Re: [dpdk-dev] [PATCH 2/5] ethdev: add an iterator to match some devargs input

[Andrew Rybchenko <arybchenko@solarflare.com>]

On 10/8/18 1:25 AM, Thomas Monjalon wrote:
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> ---
>   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 <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,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;