Re: [dpdk-dev] [PATCH v1 05/13] ethdev: add private generic device iterator

["Gaëtan Rivet" <gaetan.rivet@6wind.com>]

On Fri, Aug 31, 2018 at 01:09:34PM +0300, Andrew Rybchenko wrote:
> On 08/30/2018 04:41 PM, Gaetan Rivet wrote:
> > This iterator can be customized with a comparison function that will
> > trigger a stopping condition.
> > 
> > It can be leveraged to write several different iterators that have
> > similar but non-identical purposes.
> > 
> > It is private to librte_ethdev.
> > 
> > Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
> > ---
> >   lib/librte_ethdev/Makefile      |  1 +
> >   lib/librte_ethdev/eth_private.c | 31 +++++++++++++++++++++++++++++++
> >   lib/librte_ethdev/eth_private.h | 26 ++++++++++++++++++++++++++
> >   lib/librte_ethdev/meson.build   |  3 ++-
> >   4 files changed, 60 insertions(+), 1 deletion(-)
> >   create mode 100644 lib/librte_ethdev/eth_private.c
> >   create mode 100644 lib/librte_ethdev/eth_private.h
> > 
> > diff --git a/lib/librte_ethdev/Makefile b/lib/librte_ethdev/Makefile
> > index 0935a275e..3c1c92cb9 100644
> > --- a/lib/librte_ethdev/Makefile
> > +++ b/lib/librte_ethdev/Makefile
> > @@ -18,6 +18,7 @@ EXPORT_MAP := rte_ethdev_version.map
> >   LIBABIVER := 10
> > +SRCS-y += eth_private.c
> >   SRCS-y += rte_ethdev.c
> >   SRCS-y += rte_flow.c
> >   SRCS-y += rte_tm.c
> > diff --git a/lib/librte_ethdev/eth_private.c b/lib/librte_ethdev/eth_private.c
> > new file mode 100644
> > index 000000000..d565568a0
> > --- /dev/null
> > +++ b/lib/librte_ethdev/eth_private.c
> 
> Just a nit
> I think it is better to name it ethdev_private.c since we already
> have ethdev_profile.[ch] and it is about ethdev, not Ethernet.
> 

Agreed.

> > @@ -0,0 +1,31 @@
> > +/* SPDX-License-Identifier: BSD-3-Clause
> > + * Copyright(c) 2018 Gaëtan Rivet
> > + */
> > +
> > +#include "rte_ethdev.h"
> > +#include "eth_private.h"
> > +
> > +struct rte_eth_dev *
> > +eth_find_device(const struct rte_eth_dev *start, rte_eth_cmp_t cmp,
> > +		const void *data)
> > +{
> > +	struct rte_eth_dev *edev;
> > +	ptrdiff_t idx;
> > +
> > +	/* Avoid Undefined Behaviour */
> > +	if (start != NULL &&
> > +	    (start < &rte_eth_devices[0] ||
> > +	     start > &rte_eth_devices[RTE_MAX_ETHPORTS]))
> > +		return NULL;
> > +	if (start != NULL)
> > +		idx = start - &rte_eth_devices[0] + 1;
> > +	else
> > +		idx = 0;
> > +	for (; idx < RTE_MAX_ETHPORTS; idx++) {
> > +		edev = &rte_eth_devices[idx];
> 
> Shouldn't we limit it to valid ports only?
> If no, I think it would be useful to highlight it in the function
> description that it iterates over all devices including unused.
> 

I'm undecided about it, I wanted eth_find_device to be and internal
operator in the most generic way, allowing ethdev layer to move from
manually iterating to using this, to introduce the probable future
evolution of not using an array of rte_eth_devices.

So I thought of this operator as something stateless, only looking at
currently available memory and letting the comparison select what is
useful. Maybe for example this operator would be used to find the next
unused device, etc.

My doubts is that this is kind of future-proofing the design, something
that I don't think is good practice.

So, if you prefer something that takes care of state, as far as devargs
parsing is concerned, I'm ok with that. Otherwise it can stay that way,
UNUSED devices are taken care of afterward.

> > +		if (cmp(edev, data) == 0)
> > +			return edev;
> > +	}
> > +	return NULL;
> > +}
> > +
> > diff --git a/lib/librte_ethdev/eth_private.h b/lib/librte_ethdev/eth_private.h
> > new file mode 100644
> > index 000000000..0f5c6d5c4
> > --- /dev/null
> > +++ b/lib/librte_ethdev/eth_private.h
> 
> ethdev_private.h

sure.

> 
> <...>

-- 
Gaëtan Rivet
6WIND