From mboxrd@z Thu Jan 1 00:00:00 1970 From: Shreyansh Jain Subject: Re: [PATCH v11 11/25] eal/dev: implement device iteration Date: Thu, 12 Jul 2018 16:28:27 +0530 Message-ID: <1a96a4a1-71fd-74ef-f599-22dc21134ed4@nxp.com> References: Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Cc: dev@dpdk.org To: Gaetan Rivet Return-path: Received: from EUR01-DB5-obe.outbound.protection.outlook.com (mail-db5eur01on0045.outbound.protection.outlook.com [104.47.2.45]) by dpdk.org (Postfix) with ESMTP id B92BD1B45E for ; Thu, 12 Jul 2018 12:58:55 +0200 (CEST) In-Reply-To: Content-Language: en-US List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On Thursday 12 July 2018 03:15 AM, Gaetan Rivet wrote: > Use the iteration hooks in the abstraction layers to perform the > requested filtering on the internal device lists. > > Signed-off-by: Gaetan Rivet > --- > lib/librte_eal/common/eal_common_dev.c | 168 ++++++++++++++++++++++++ > lib/librte_eal/common/include/rte_dev.h | 26 ++++ > lib/librte_eal/rte_eal_version.map | 1 + > 3 files changed, 195 insertions(+) > > diff --git a/lib/librte_eal/common/eal_common_dev.c b/lib/librte_eal/common/eal_common_dev.c > index 63e329bd8..b78845f02 100644 > --- a/lib/librte_eal/common/eal_common_dev.c > +++ b/lib/librte_eal/common/eal_common_dev.c > @@ -45,6 +45,28 @@ static struct dev_event_cb_list dev_event_cbs; > /* spinlock for device callbacks */ > static rte_spinlock_t dev_event_lock = RTE_SPINLOCK_INITIALIZER; > > +struct dev_next_ctx { > + struct rte_dev_iterator *it; > + const char *bus_str; > + const char *cls_str; > +}; > + > +#define CTX(it, bus_str, cls_str) \ > + (&(const struct dev_next_ctx){ \ > + .it = it, \ > + .bus_str = bus_str, \ > + .cls_str = cls_str, \ > + }) > + > +#define ITCTX(ptr) \ > + (((struct dev_next_ctx *)(intptr_t)ptr)->it) > + > +#define BUSCTX(ptr) \ > + (((struct dev_next_ctx *)(intptr_t)ptr)->bus_str) > + > +#define CLSCTX(ptr) \ > + (((struct dev_next_ctx *)(intptr_t)ptr)->cls_str) > + > static int cmp_detached_dev_name(const struct rte_device *dev, > const void *_name) > { > @@ -398,3 +420,149 @@ rte_dev_iterator_init(struct rte_dev_iterator *it, > get_out: > return -rte_errno; > } > + > +static char * > +dev_str_sane_copy(const char *str) > +{ > + size_t end; > + char *copy; > + > + end = strcspn(str, ",/"); > + if (str[end] == ',') { > + copy = strdup(&str[end + 1]); > + } else { > + /* '/' or '\0' */ > + copy = strdup(""); > + } Though it doesn't change anything functionally, if you can separate blocks of if-else with new lines, it really makes it easier to read. Like here... > + if (copy == NULL) { > + rte_errno = ENOMEM; > + } else { > + char *slash; > + > + slash = strchr(copy, '/'); > + if (slash != NULL) > + slash[0] = '\0'; > + } > + return copy; > +} > + > +static int > +class_next_dev_cmp(const struct rte_class *cls, > + const void *ctx) > +{ > + struct rte_dev_iterator *it; > + const char *cls_str = NULL; > + void *dev; > + > + if (cls->dev_iterate == NULL) > + return 1; > + it = ITCTX(ctx); > + cls_str = CLSCTX(ctx); > + dev = it->class_device; > + /* it->cls_str != NULL means a class > + * was specified in the devstr. > + */ > + if (it->cls_str != NULL && cls != it->cls) > + return 1; > + /* If an error occurred previously, > + * no need to test further. > + */ > + if (rte_errno != 0) > + return -1; I am guessing here by '..error occurred previously..' you mean sane_copy. If so, why wait until this point to return? Anyway the caller (rte_bus_find, probably) would only look for '0' or non-zero. > + dev = cls->dev_iterate(dev, cls_str, it); > + it->class_device = dev; > + return dev == NULL; > +} > + > +static int > +bus_next_dev_cmp(const struct rte_bus *bus, > + const void *ctx) > +{ > + struct rte_device *dev = NULL; > + struct rte_class *cls = NULL; > + struct rte_dev_iterator *it; > + const char *bus_str = NULL; > + > + if (bus->dev_iterate == NULL) > + return 1; > + it = ITCTX(ctx); > + bus_str = BUSCTX(ctx); > + dev = it->device; > + /* it->bus_str != NULL means a bus > + * was specified in the devstr. > + */ > + if (it->bus_str != NULL && bus != it->bus) > + return 1; > + /* If an error occurred previously, > + * no need to test further. > + */ > + if (rte_errno != 0) > + return -1; > + if (it->cls_str == NULL) { > + dev = bus->dev_iterate(dev, bus_str, it); > + goto end; This is slightly confusing. If it->cls_str == NULL, you do bus->dev_iterate... but > + } > + /* cls_str != NULL */ > + if (dev == NULL) { > +next_dev_on_bus: > + dev = bus->dev_iterate(dev, bus_str, it); When cls_str!=NULL, you still do bus->dev_iterate... So, maybe they are OR case resulting in check of dev==NULL and return (as being done right now by jumping to out)...? And, how can bus iterate over a 'null' device? > + it->device = dev; > + } > + if (dev == NULL) > + return 1; Maybe, this check should move in the if(dev==NULL) above - that way, it can in path of 'next_dev_on_bus' yet do the same as function as its current location. > + if (it->cls != NULL) In what case would (it->cls_str == NULL) but (it->cls != NULL)? > + cls = TAILQ_PREV(it->cls, rte_class_list, next); > + cls = rte_class_find(cls, class_next_dev_cmp, ctx); > + if (cls != NULL) { > + it->cls = cls; > + goto end; > + } > + goto next_dev_on_bus; Maybe I have completely mixed your intention of this function. So, if you still find the above comments naive - maybe you can tell me what you are attempting here? Is it: find next bus and stop if no class specified. find next class as well, iff that too was specified? Reason I am confused is that bus_next_dev_cmp attempts to compare both - bus and class, yet class_next_dev_cmp simply stops by comparing class only. > +end: > + it->device = dev; > + return dev == NULL; > +} A new line should be added here - start of a new function. > +__rte_experimental > +struct rte_device * > +rte_dev_iterator_next(struct rte_dev_iterator *it) > +{ > + struct rte_bus *bus = NULL; > + int old_errno = rte_errno; > + char *bus_str = NULL; > + char *cls_str = NULL; > + > + rte_errno = 0; > + if (it->bus_str == NULL && it->cls_str == NULL) { > + /* Invalid iterator. */ > + rte_errno = EINVAL; > + return NULL; > + } > + if (it->bus != NULL) > + bus = TAILQ_PREV(it->bus, rte_bus_list, next); > + if (it->bus_str != NULL) { > + bus_str = dev_str_sane_copy(it->bus_str); > + if (bus_str == NULL) > + goto out; > + } > + if (it->cls_str != NULL) { > + cls_str = dev_str_sane_copy(it->cls_str); > + if (cls_str == NULL) > + goto out; > + } > + while ((bus = rte_bus_find(bus, bus_next_dev_cmp, > + CTX(it, bus_str, cls_str)))) { > + if (it->device != NULL) { > + it->bus = bus; > + goto out; > + } > + if (it->bus_str != NULL || > + rte_errno != 0) > + break; > + } > + if (rte_errno == 0) > + rte_errno = old_errno; > +out: > + free(bus_str); > + free(cls_str); > + return it->device; > +} > diff --git a/lib/librte_eal/common/include/rte_dev.h b/lib/librte_eal/common/include/rte_dev.h > index fdc812ff8..8638a2bbd 100644 > --- a/lib/librte_eal/common/include/rte_dev.h > +++ b/lib/librte_eal/common/include/rte_dev.h > @@ -355,6 +355,32 @@ __rte_experimental > int > rte_dev_iterator_init(struct rte_dev_iterator *it, const char *str); > > +/** > + * Iterates on a device iterator. > + * > + * Generates a new rte_device handle corresponding to the next element > + * in the list described in comprehension by the iterator. > + * > + * The next object is returned, and the iterator is updated. > + * > + * @param it > + * Device iterator handle. > + * > + * @return > + * An rte_device handle if found. > + * NULL if an error occurred (rte_errno is set). > + * NULL if no device could be found (rte_errno is not set). > + */ > +__rte_experimental > +struct rte_device * > +rte_dev_iterator_next(struct rte_dev_iterator *it); > + > +#define RTE_DEV_FOREACH(dev, devstr, it) \ > + for (rte_dev_iterator_init(it, devstr), \ > + dev = rte_dev_iterator_next(it); \ > + dev != NULL; \ > + dev = rte_dev_iterator_next(it)) > + > #ifdef __cplusplus > } > #endif > diff --git a/lib/librte_eal/rte_eal_version.map b/lib/librte_eal/rte_eal_version.map > index ac04120d6..4cd5ab3df 100644 > --- a/lib/librte_eal/rte_eal_version.map > +++ b/lib/librte_eal/rte_eal_version.map > @@ -252,6 +252,7 @@ EXPERIMENTAL { > rte_dev_event_monitor_start; > rte_dev_event_monitor_stop; > rte_dev_iterator_init; > + rte_dev_iterator_next; > rte_devargs_add; > rte_devargs_dump; > rte_devargs_insert; >