From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dispatch1-us1.ppe-hosted.com (dispatch1-us1.ppe-hosted.com [148.163.129.52]) by dpdk.org (Postfix) with ESMTP id 8870B1DB8 for ; Mon, 8 Oct 2018 09:07:20 +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 33F53B40061; Mon, 8 Oct 2018 07:07:18 +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; Mon, 8 Oct 2018 08:07:11 +0100 To: Thomas Monjalon , CC: , , References: <20181007222554.4886-1-thomas@monjalon.net> <20181007222554.4886-3-thomas@monjalon.net> From: Andrew Rybchenko Message-ID: <8689ac9f-c5dd-aa29-8f8b-e32674338160@solarflare.com> Date: Mon, 8 Oct 2018 10:06:30 +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: <20181007222554.4886-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-24142.003 X-TM-AS-Result: No-23.396700-8.000000-10 X-TMASE-MatchedRID: u6ojmU07PKwOwH4pD14DsPHkpkyUphL9X5TqQagR07eHlDUCu0I+XEUW SNUeoUM2IV4u8YKdeKsBtjkcfRMmqUfr6WG4Th9aj5hLPCX3ZdO/yN2q8U674h0bKcI3i70lMq+ en+OaNuuxMCJcxAohFkdgwneopewYw+noLBygYTjJ1E39jKDimAGZ/+APXW9k9MKrUsgs1FUb29 WAvZS6W2lN2Y+u+3g04P7jwUgIrrh72dn2ylCK1kf49ONH0RaSSg85P8ZMy4YtxWNuz6R5rPBYR o06eVj3gML9UOgCBPdiHeCFOuGHOoYCBZzro74fGFMYlDUzwr0pWss5kPUFdGaDo8g8jAvL2yTT 9iCpP0fe5Q7kEmiRVgm1LFAkOT1AKxWw1zPDv68wiJTf3kjwfTGZtPrBBPZrmKInQ61iaANvkON K/M1of7zpx5Afjtnea6yV/w/mblhg5AA4n45LbWwbuvhCHs3cFTmqwD90nsIG2HMvWEJenvRm0k mqtH+DFx+Yjf5GQdhmwbChf+ccp1Fej9EwCdZzdPuue3cRiRgXjfGTDxEzLQ2jblY+VZC3807t+ VEWpeYgZIcq73TEpZJTzjwz+GzoOhEq4R2it0Evj6wHfIGxyQhRCJFb9cus5x99q4kVOUh5RA29 TjvO6H4MKWqwjUmzierhrTyYFA/fX2sZfyOJoZ4CIKY/Hg3AtOt1ofVlaoLUHQeTVDUrItRnEQC UU+jz4SYwERccte20414PQsB701G52SG4FjEmhkgwURgXNKKEqjDmdqe2cQ== X-TM-AS-User-Approved-Sender: Yes X-TM-AS-User-Blocked-Sender: No X-TMASE-Result: 10--23.396700-8.000000 X-TMASE-Version: SMEX-12.5.0.1300-8.5.1010-24142.003 X-MDID: 1538982439-RRD5RN9zNcaP 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 2/5] ethdev: add an iterator to match some 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: Mon, 08 Oct 2018 07:07:20 -0000 On 10/8/18 1:25 AM, Thomas Monjalon wrote: > Signed-off-by: Thomas Monjalon > --- > 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_ethdev.c | 87 ++++++++++++++++++++++ > lib/librte_ethdev/rte_ethdev.h | 56 ++++++++++++++ > lib/librte_ethdev/rte_ethdev_version.map | 2 + > 6 files changed, 166 insertions(+), 1 deletion(-) > > diff --git a/lib/librte_eal/common/include/rte_common.h b/lib/librte_eal/common/include/rte_common.h > index 069c13ec7..2269e5456 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) \ Missing space after each comma. > + (*(type*)((uintptr_t)(var) + offsetof(typeof(*(var)), field))) > + In general it is bad symptom that we need it. I'd think more that twice before adding it :) > /*********** 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_ethdev.c b/lib/librte_ethdev/rte_ethdev.c > index ef99f7068..83ab28c23 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,91 @@ enum { > STAT_QMAP_RX > }; > > +int __rte_experimental > +rte_eth_iterator_init(struct rte_dev_iterator *iter, const char *devargs_str) > +{ > +#define iter_anybus_str "class=eth," > + int ret; > + struct rte_devargs devargs; > + const char *bus_param_key; > + char *bus_str; > + size_t bus_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); rte_devargs_parse() does strdup() for args. It requires free() in the function if devargs parsed successfully, but init fails. In the case of success it is moved to cls_str which is 'const' and I see not code which frees it as well. > + if (ret != 0) > + return ret; > + > + /* Assume parameters of old syntax can match only at ethdev level. */ > + iter->cls_str = devargs.args; > + > + iter->bus = devargs.bus; > + if (iter->bus->dev_iterate == NULL) > + ret = -ENOTSUP; /* share error log with below */ > + > + /* 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"; > + else > + ret = -ENOTSUP; > + if (ret < 0) { > + RTE_LOG(ERR, EAL, "Bus %s does not support iterating.\n", > + iter->bus->name); > + return -ENOTSUP; > + } > + bus_str_size = strlen(bus_param_key) + strlen(devargs.name) + 2; > + bus_str = malloc(bus_str_size); > + if (bus_str == NULL) > + return -ENOMEM; > + ret = snprintf(bus_str, bus_str_size, "%s=%s", > + bus_param_key, devargs.name); > + if (ret < 0) { > + free(bus_str); > + return -EINVAL; > + } > + iter->bus_str = bus_str; > + > + iter->cls = rte_class_find_by_name("eth"); > + return 0; > +} > + > +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 for matching rte_device */ > + if (iter->class_device == NULL) { > + iter->device = iter->bus->dev_iterate( > + iter->device, iter->bus_str, iter); > + if (iter->device == NULL) > + break; > + } > + 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); > + } while (iter->class_device == NULL); It is non-obvious what is happening above. It would be very useful to add comments which explains it. May be just hints. > + > + /* No more ethdev port to iterate. */ > + free(RTE_CAST_FIELD(iter, bus_str, char *)); /* workaround const */ > + iter->bus_str = NULL; Who does it if RTE_ETH_FOREACH_MATCHING_DEV caller breaks the loop before reaching maximum? > + return RTE_MAX_ETHPORTS; > +} > + > 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..d5a457216 100644 > --- a/lib/librte_ethdev/rte_ethdev.h > +++ b/lib/librte_ethdev/rte_ethdev.h > @@ -166,6 +166,62 @@ 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 field bus_str is dynamically allocated and must be freed. > + * > + * @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(). > + * The field bus_str is freed when no more port is found. > + * > + * @return > + * A port id if found, RTE_MAX_ETHPORTS otherwise. > + */ > +__rte_experimental > +uint16_t rte_eth_iterator_next(struct rte_dev_iterator *iter); > + > +/** > + * Macro to iterate over all ethdev ports matching some devargs. > + * > + * @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)) > + Such iterators are very convenient, but in this particular case it is a source of non-obvious memory leaks and necessity of the hack to discard 'const'. May be iterator free callback with its own opaque data can help to avoid these hacks with const discard? I.e. rte_eth_iterator_free() which does the job and mentioned in the rte_eth_iterator_init() and the macro description. > /** > * 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_switch_domain_alloc; > rte_eth_switch_domain_free; > rte_flow_expand_rss;