From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <arybchenko@solarflare.com>
Received: from dispatch1-us1.ppe-hosted.com (dispatch1-us1.ppe-hosted.com
[67.231.154.164]) by dpdk.org (Postfix) with ESMTP id 27E771B1F1
for <dev@dpdk.org>; Tue, 9 Oct 2018 11:32:37 +0200 (CEST)
X-Virus-Scanned: Proofpoint Essentials engine
Received: from webmail.solarflare.com (uk.solarflare.com [193.34.186.16])
(using TLSv1.2 with cipher ECDHE-RSA-AES256-SHA384 (256/256 bits))
(No client certificate requested)
by mx1-us3.ppe-hosted.com (Proofpoint Essentials ESMTP Server) with ESMTPS id
59BBB9C005B; Tue, 9 Oct 2018 09:32:35 +0000 (UTC)
Received: from [192.168.38.17] (91.220.146.112) by ukex01.SolarFlarecom.com
(10.17.10.4) with Microsoft SMTP Server (TLS) id 15.0.1395.4; Tue, 9 Oct 2018
10:32:29 +0100
To: Thomas Monjalon <thomas@monjalon.net>, <dev@dpdk.org>
CC: <gaetan.rivet@6wind.com>, <ophirmu@mellanox.com>, <ferruh.yigit@intel.com>
References: <20181007222554.4886-1-thomas@monjalon.net>
<20181009001616.10497-1-thomas@monjalon.net>
<20181009001616.10497-3-thomas@monjalon.net>
From: Andrew Rybchenko <arybchenko@solarflare.com>
Message-ID: <8b6df6a2-93ea-396f-c35e-31ae710c30c9@solarflare.com>
Date: Tue, 9 Oct 2018 12:31:47 +0300
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
Thunderbird/60.0
MIME-Version: 1.0
In-Reply-To: <20181009001616.10497-3-thomas@monjalon.net>
Content-Language: en-GB
X-Originating-IP: [91.220.146.112]
X-ClientProxiedBy: ocex03.SolarFlarecom.com (10.20.40.36) To
ukex01.SolarFlarecom.com (10.17.10.4)
X-TM-AS-Product-Ver: SMEX-12.5.0.1300-8.5.1010-24144.003
X-TM-AS-Result: No-17.916200-8.000000-10
X-TMASE-MatchedRID: 0lhM5bBmjEMOwH4pD14DsPHkpkyUphL9dSHY4x0UjQnMB0kPsl40wwn9
ofQNoBQF4oFHJm9sCKlbKf/6AFZqwYBoGyFs+F1MiVYleIBIrY8S12tj9Zvd83zlhuYw8JsTtW1
Wj9Lt9iKpHhuvR/33oM3jfPQqNI3UhUgo3TqXokH1RUeLAvHT8ilayzmQ9QV0d9Bdd/Oygj27Sa
dIEA5aG9PTUDa8SKRWAbY5HH0TJqlH6+lhuE4fWo+YSzwl92XT5Y0kb0hqatx3vUA6/Pi03HY3T
cMnBmkfuBv/D8BB2s36fu2oNjvIqHcna+FgAjo0VU3yVpaj3QwJDfFL7Mvp7aMP1fF+gQ2O7q5C
4hc2gUJrJo8P2lJSR+ZGr412mQtci9Om4SO4imS4jAucHcCqnZh/evhkKmjqzQ05wK4VbrWOaw6
mJwKBPOlIl0vueSZqXi6O8BfIMIKIT1Xviano0TCaEJt46PppWU9oJdIKhsqNTrsIz649BK0EWc
sWoNWb00Mueu1NQPDmnUOunTqnrE3cZYMqE45IQpxiLlDD9FUYrnuRn/aZqTDJ9a3KikGoaza5e
RS5azQ8ivFkadaiTeXs3RL/C7f1Vvw4hLZdTvN0+657dxGJGBeN8ZMPETMtDaNuVj5VkLfzTu35
URal5iBkhyrvdMSlklPOPDP4bOjsxr0ZiyQynz5bo3ZLMFMHB+dff70WbjlCI7pNLiSjfbSDm1Y
HCToety25oFQme1X/voIzhn8wZuVHGbcDbAq6OX/V8P8ail1ZDL1gLmoa/PoA9r2LThYYvJqH1o
JhQIewYOdumIOriJUo2zqk3bYOrhRsQA01y0ePHpfnxuArxrQP4ImCyJ3WSUeoZcZQ8GVnogHS4
m59FxHt3U8a3BzJwJheUhYd3dU=
X-TM-AS-User-Approved-Sender: Yes
X-TM-AS-User-Blocked-Sender: No
X-TMASE-Result: 10--17.916200-8.000000
X-TMASE-Version: SMEX-12.5.0.1300-8.5.1010-24144.003
X-MDID: 1539077556-Vpj8dtEw_5pm
Content-Type: text/plain; charset="utf-8"; format=flowed
Content-Transfer-Encoding: 7bit
X-Content-Filtered-By: Mailman/MimeDel 2.1.15
Subject: Re: [dpdk-dev] [PATCH v2 2/6] ethdev: add iterator to match devargs
input
X-BeenThere: dev@dpdk.org
X-Mailman-Version: 2.1.15
Precedence: list
List-Id: DPDK patches and discussions <dev.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/dev>,
<mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/dev>,
<mailto:dev-request@dpdk.org?subject=subscribe>
X-List-Received-Date: Tue, 09 Oct 2018 09:32:37 -0000
On 10/9/18 3:16 AM, Thomas Monjalon wrote:
> The iterator will return the ethdev port ids matching a devargs string.
> It is recommended to use the macro RTE_ETH_FOREACH_MATCHING_DEV()
> for usage convenience.
>
> The class string is prefixed with '+' in order to skip the validation
> of the parameter keys. It is tolerated for the compatibility with
> the old (current) syntax where all parameters (bus, class and driver)
> are mixed in the same string without any delimiter.
> Thanks to this compatibility prefix, the driver parameters will be
> skipped during the ethdev parsing, and not considered invalid.
>
> A macro is introduced in rte_common.h to workaround a const field.
> This hack is needed to free const strings in the iterator.
> It is preferred to keep the const for these fields, because it gives
> a hint that they are not changed at each iteration.
>
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
Few minor notes below, in any case:
Reviewed-by: Andrew Rybchenko <arybchenko@solarflare.com>
> ---
> lib/librte_eal/common/include/rte_common.h | 6 +
> lib/librte_ethdev/ethdev_private.c | 10 +-
> lib/librte_ethdev/ethdev_private.h | 6 +
> lib/librte_ethdev/rte_class_eth.c | 7 +-
> lib/librte_ethdev/rte_ethdev.c | 124 +++++++++++++++++++++
> lib/librte_ethdev/rte_ethdev.h | 77 +++++++++++++
> lib/librte_ethdev/rte_ethdev_version.map | 2 +
> 7 files changed, 230 insertions(+), 2 deletions(-)
>
> diff --git a/lib/librte_eal/common/include/rte_common.h b/lib/librte_eal/common/include/rte_common.h
> index 069c13ec7..e3c0407a9 100644
> --- a/lib/librte_eal/common/include/rte_common.h
> +++ b/lib/librte_eal/common/include/rte_common.h
> @@ -164,6 +164,12 @@ static void __attribute__((destructor(RTE_PRIO(prio)), used)) func(void)
> */
> #define RTE_PTR_DIFF(ptr1, ptr2) ((uintptr_t)(ptr1) - (uintptr_t)(ptr2))
>
> +/**
> + * Workaround to cast a const field of a structure to non-const type.
> + */
> +#define RTE_CAST_FIELD(var, field, type) \
> + (*(type *)((uintptr_t)(var) + offsetof(typeof(*(var)), field)))
> +
> /*********** Macros/static functions for doing alignment ********/
>
>
> diff --git a/lib/librte_ethdev/ethdev_private.c b/lib/librte_ethdev/ethdev_private.c
> index 768c8b2ed..acc787dba 100644
> --- a/lib/librte_ethdev/ethdev_private.c
> +++ b/lib/librte_ethdev/ethdev_private.c
> @@ -5,6 +5,14 @@
> #include "rte_ethdev.h"
> #include "ethdev_private.h"
>
> +uint16_t
> +eth_dev_to_id(const struct rte_eth_dev *dev)
> +{
> + if (dev == NULL)
> + return RTE_MAX_ETHPORTS;
> + return dev - rte_eth_devices;
> +}
> +
> struct rte_eth_dev *
> eth_find_device(const struct rte_eth_dev *start, rte_eth_cmp_t cmp,
> const void *data)
> @@ -18,7 +26,7 @@ eth_find_device(const struct rte_eth_dev *start, rte_eth_cmp_t cmp,
> start > &rte_eth_devices[RTE_MAX_ETHPORTS]))
> return NULL;
> if (start != NULL)
> - idx = start - &rte_eth_devices[0] + 1;
> + idx = eth_dev_to_id(start) + 1;
> else
> idx = 0;
> for (; idx < RTE_MAX_ETHPORTS; idx++) {
> diff --git a/lib/librte_ethdev/ethdev_private.h b/lib/librte_ethdev/ethdev_private.h
> index 0f5c6d5c4..e67cf6831 100644
> --- a/lib/librte_ethdev/ethdev_private.h
> +++ b/lib/librte_ethdev/ethdev_private.h
> @@ -11,6 +11,12 @@
> extern "C" {
> #endif
>
> +/*
> + * Convert rte_eth_dev pointer to port id.
> + * NULL will be translated to RTE_MAX_ETHPORTS.
> + */
> +uint16_t eth_dev_to_id(const struct rte_eth_dev *dev);
> +
> /* Generic rte_eth_dev comparison function. */
> typedef int (*rte_eth_cmp_t)(const struct rte_eth_dev *, const void *);
>
> diff --git a/lib/librte_ethdev/rte_class_eth.c b/lib/librte_ethdev/rte_class_eth.c
> index 84b646291..532080a58 100644
> --- a/lib/librte_ethdev/rte_class_eth.c
> +++ b/lib/librte_ethdev/rte_class_eth.c
> @@ -57,9 +57,14 @@ eth_dev_iterate(const void *start,
> {
> struct rte_kvargs *kvargs = NULL;
> struct rte_eth_dev *edev = NULL;
> + const char * const *valid_keys = NULL;
>
> if (str != NULL) {
> - kvargs = rte_kvargs_parse(str, eth_params_keys);
> + if (str[0] == '+') /* no validation of keys */
> + str ++;
As I understand it should be no space before ++
> + else
> + valid_keys = eth_params_keys;
> + kvargs = rte_kvargs_parse(str, valid_keys);
> if (kvargs == NULL) {
> RTE_LOG(ERR, EAL, "cannot parse argument list\n");
> rte_errno = EINVAL;
> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> index ef99f7068..da20ab81f 100644
> --- a/lib/librte_ethdev/rte_ethdev.c
> +++ b/lib/librte_ethdev/rte_ethdev.c
> @@ -36,11 +36,13 @@
> #include <rte_spinlock.h>
> #include <rte_string_fns.h>
> #include <rte_kvargs.h>
> +#include <rte_class.h>
>
> #include "rte_ether.h"
> #include "rte_ethdev.h"
> #include "rte_ethdev_driver.h"
> #include "ethdev_profile.h"
> +#include "ethdev_private.h"
>
> int rte_eth_dev_logtype;
>
> @@ -181,6 +183,128 @@ enum {
> STAT_QMAP_RX
> };
>
> +int __rte_experimental
> +rte_eth_iterator_init(struct rte_dev_iterator *iter, const char *devargs_str)
> +{
> + int ret;
> + struct rte_devargs devargs = {.args = NULL};
> + const char *bus_param_key;
> + char *bus_str = NULL;
> + char *cls_str = NULL;
> + size_t str_size;
> +
> + memset(iter, 0, sizeof(*iter));
> +
> + /*
> + * The devargs string may use various syntaxes:
> + * - 0000:08:00.0,representor=[1-3]
> + * - pci:0000:06:00.0,representor=[0,5]
> + * A new syntax is in development (not yet supported):
> + * - bus=X,paramX=x/class=Y,paramY=y/driver=Z,paramZ=z
> + */
> +
> + /* Split bus, device and parameters. */
> + ret = rte_devargs_parse(&devargs, devargs_str);
> + if (ret != 0)
> + goto error;
> +
> + /*
> + * Assume parameters of old syntax can match only at ethdev level.
> + * Extra parameters will be ignored, thanks to "+" prefix.
> + */
> + str_size = strlen(devargs.args) + 2;
> + cls_str = malloc(str_size);
> + if (cls_str == NULL) {
> + ret = -ENOMEM;
> + goto error;
> + }
> + ret = snprintf(cls_str, str_size, "+%s", devargs.args);
> + if (ret < 0) {
As I understand we expect ret == str_size - 1 here. May be it makes sent
to harden the check.
> + ret = -EINVAL;
> + goto error;
> + }
> + iter->cls_str = cls_str;
> + free(devargs.args); /* allocated by rte_devargs_parse() */
> + devargs.args = NULL;
> +
> + iter->bus = devargs.bus;
> + if (iter->bus->dev_iterate == NULL) {
> + ret = -ENOTSUP;
> + goto error;
> + }
> +
> + /* Convert bus args to new syntax for use with new API dev_iterate. */
> + if (strcmp(iter->bus->name, "vdev") == 0)
> + bus_param_key = "name";
> + else if (strcmp(iter->bus->name, "pci") == 0)
> + bus_param_key = "addr";
I thought that if one branch has curly brackets other branches should
have curly brackets as well.
> + else {
> + ret = -ENOTSUP;
> + goto error;
> + }
> + str_size = strlen(bus_param_key) + strlen(devargs.name) + 2;
> + bus_str = malloc(str_size);
> + if (bus_str == NULL) {
> + ret = -ENOMEM;
> + goto error;
> + }
> + ret = snprintf(bus_str, str_size, "%s=%s",
> + bus_param_key, devargs.name);
> + if (ret < 0) {
May be it makes sense to make the check more strict: ret != str_size - 1
> + ret = -EINVAL;
> + goto error;
> + }
> + iter->bus_str = bus_str;
> +
> + iter->cls = rte_class_find_by_name("eth");
> + return 0;
> +
> +error:
> + if (ret == -ENOTSUP)
> + RTE_LOG(ERR, EAL, "Bus %s does not support iterating.\n",
> + iter->bus->name);
> + free(devargs.args);
> + free(bus_str);
> + free(cls_str);
> + return ret;
> +}
> +
> +uint16_t __rte_experimental
> +rte_eth_iterator_next(struct rte_dev_iterator *iter)
> +{
> + if (iter->cls == NULL) /* invalid ethdev iterator */
> + return RTE_MAX_ETHPORTS;
> +
> + do { /* loop to try all matching rte_device */
> + /* If not in middle of rte_eth_dev iteration, */
> + if (iter->class_device == NULL) {
> + /* get next rte_device to try. */
> + iter->device = iter->bus->dev_iterate(
> + iter->device, iter->bus_str, iter);
> + if (iter->device == NULL)
> + break; /* no more rte_device candidate */
> + }
> + /* A device is matching bus part, need to check ethdev part. */
> + iter->class_device = iter->cls->dev_iterate(
> + iter->class_device, iter->cls_str, iter);
> + if (iter->class_device != NULL)
> + return eth_dev_to_id(iter->class_device); /* match */
> + } while (1); /* need to try next rte_device */
> +
> + /* No more ethdev port to iterate. */
> + rte_eth_iterator_free(iter);
> + return RTE_MAX_ETHPORTS;
> +}
> +
> +void __rte_experimental
> +rte_eth_iterator_free(struct rte_dev_iterator *iter)
Yes, I know that the name is suggested by me, but maybe
it should be rte_eth_iterator_fini() or _cleanup() as pair to _init.
> +{
> + free(RTE_CAST_FIELD(iter, bus_str, char *)); /* workaround const */
> + iter->bus_str = NULL;
> + free(RTE_CAST_FIELD(iter, cls_str, char *)); /* workaround const */
> + iter->cls_str = NULL;
> +}
> +
> uint16_t
> rte_eth_find_next(uint16_t port_id)
> {
> diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
> index 012577b0a..9c12c8cdf 100644
> --- a/lib/librte_ethdev/rte_ethdev.h
> +++ b/lib/librte_ethdev/rte_ethdev.h
> @@ -166,6 +166,83 @@ extern int rte_eth_dev_logtype;
>
> struct rte_mbuf;
>
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice.
> + *
> + * Initializes a device iterator.
> + *
> + * This iterator allows accessing a list of devices matching some devargs.
> + *
> + * @param iter
> + * Device iterator handle initialized by the function.
> + * The fields bus_str and cls_str might be dynamically allocated,
> + * and could be freed by calling rte_eth_iterator_free().
> + *
> + * @param devargs
> + * Device description string.
> + *
> + * @return
> + * 0 on successful initialization, negative otherwise.
> + */
> +__rte_experimental
> +int rte_eth_iterator_init(struct rte_dev_iterator *iter, const char *devargs);
> +
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice.
> + *
> + * Iterates on devices with devargs filter.
> + * The ownership is not checked.
> + *
> + * The next port id is returned, and the iterator is updated.
> + *
> + * @param iter
> + * Device iterator handle initialized by rte_eth_iterator_init().
> + * Some fields bus_str and cls_str might be freed when no more port is found,
> + * by calling rte_eth_iterator_free().
> + *
> + * @return
> + * A port id if found, RTE_MAX_ETHPORTS otherwise.
> + */
> +__rte_experimental
> +uint16_t rte_eth_iterator_next(struct rte_dev_iterator *iter);
> +
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice.
> + *
> + * Free some allocated fields of the iterator.
> + *
> + * This function is automatically called by rte_eth_iterator_next()
> + * on the last iteration (i.e. when no more matching port is found).
> + *
May be it makes sense to mention that it is safe to call it twice.
It could be simpler to use it taking the possibility into account.
> + * @param iter
> + * Device iterator handle initialized by rte_eth_iterator_init().
> + * The fields bus_str and cls_str are freed if needed.
> + */
> +__rte_experimental
> +void rte_eth_iterator_free(struct rte_dev_iterator *iter);
> +
> +/**
> + * Macro to iterate over all ethdev ports matching some devargs.
> + *
> + * If a break is done before the end of the loop,
> + * the function rte_eth_iterator_free() must be called.
> + *
> + * @param id
> + * Iterated port id of type uint16_t.
> + * @param devargs
> + * Device parameters input as string of type char*.
> + * @param iter
> + * Iterator handle of type struct rte_dev_iterator, used internally.
> + */
> +#define RTE_ETH_FOREACH_MATCHING_DEV(id, devargs, iter) \
> + for (rte_eth_iterator_init(iter, devargs), \
> + id = rte_eth_iterator_next(iter); \
> + id != RTE_MAX_ETHPORTS; \
> + id = rte_eth_iterator_next(iter))
> +
> /**
> * A structure used to retrieve statistics for an Ethernet port.
> * Not all statistics fields in struct rte_eth_stats are supported
> diff --git a/lib/librte_ethdev/rte_ethdev_version.map b/lib/librte_ethdev/rte_ethdev_version.map
> index 38f117f01..e009988fd 100644
> --- a/lib/librte_ethdev/rte_ethdev_version.map
> +++ b/lib/librte_ethdev/rte_ethdev_version.map
> @@ -237,6 +237,8 @@ EXPERIMENTAL {
> rte_eth_dev_owner_unset;
> rte_eth_dev_rx_offload_name;
> rte_eth_dev_tx_offload_name;
> + rte_eth_iterator_init;
> + rte_eth_iterator_next;
rte_eth_iterator_free() or renamed
> rte_eth_switch_domain_alloc;
> rte_eth_switch_domain_free;
> rte_flow_expand_rss;