From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: 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 ; 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 , CC: , , References: <20181007222554.4886-1-thomas@monjalon.net> <20181009001616.10497-1-thomas@monjalon.net> <20181009001616.10497-3-thomas@monjalon.net> From: Andrew Rybchenko 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 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-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 Few minor notes below, in any case: Reviewed-by: Andrew Rybchenko > --- > 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 > #include > #include > +#include > > #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;