Re: [dpdk-dev] [PATCH v2 2/6] ethdev: add iterator to match devargs input

[Andrew Rybchenko <arybchenko@solarflare.com>]

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;