[dpdk-dev] [PATCH v11 11/25] eal/dev: implement device iteration

[Shreyansh Jain]

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 <gaetan.rivet at 6wind.com>
> ---
>   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;
>