[dpdk-dev] [PATCH v3 10/20] eal/dev: implement device iteration initialization

[Neil Horman]

On Tue, Mar 27, 2018 at 10:23:21PM +0200, Gaëtan Rivet wrote:
> On Tue, Mar 27, 2018 at 03:04:13PM +0200, Gaëtan Rivet wrote:
> > On Tue, Mar 27, 2018 at 02:40:00PM +0200, Gaëtan Rivet wrote:
> > > On Tue, Mar 27, 2018 at 07:47:50AM -0400, Neil Horman wrote:
> > > > On Tue, Mar 27, 2018 at 01:18:34AM +0200, Gaetan Rivet wrote:
> > > > > Parse a device description.
> > > > > Split this description in their relevant part for each layers.
> > > > > No dynamic allocation is performed.
> > > > > 
> > > > > Cc: Neil Horman <nhorman at tuxdriver.com>
> > > > > Cc: "Wiles, Keith" <keith.wiles at intel.com>
> > > > > Signed-off-by: Gaetan Rivet <gaetan.rivet at 6wind.com>
> > > > > ---
> > > > > 
> > > > > This version uses librte_kvargs.
> > > > > 
> > > > >  lib/Makefile                            |   1 +
> > > > >  lib/librte_eal/bsdapp/eal/Makefile      |   1 +
> > > > >  lib/librte_eal/common/eal_common_dev.c  | 147 ++++++++++++++++++++++++++++++++
> > > > >  lib/librte_eal/common/include/rte_dev.h |  23 +++++
> > > > >  lib/librte_eal/linuxapp/eal/Makefile    |   1 +
> > > > >  lib/librte_eal/rte_eal_version.map      |   1 +
> > > > >  6 files changed, 174 insertions(+)
> > > > > 
> > > > > diff --git a/lib/Makefile b/lib/Makefile
> > > > > index fc7a55a37..1b17526f7 100644
> > > > > --- a/lib/Makefile
> > > > > +++ b/lib/Makefile
> > > > > @@ -6,6 +6,7 @@ include $(RTE_SDK)/mk/rte.vars.mk
> > > > >  DIRS-y += librte_compat
> > > > >  DIRS-$(CONFIG_RTE_LIBRTE_KVARGS) += librte_kvargs
> > > > >  DIRS-$(CONFIG_RTE_LIBRTE_EAL) += librte_eal
> > > > > +DEPDIRS-librte_eal := librte_kvargs
> > > > >  DIRS-$(CONFIG_RTE_LIBRTE_PCI) += librte_pci
> > > > >  DEPDIRS-librte_pci := librte_eal
> > > > >  DIRS-$(CONFIG_RTE_LIBRTE_RING) += librte_ring
> > > > > diff --git a/lib/librte_eal/bsdapp/eal/Makefile b/lib/librte_eal/bsdapp/eal/Makefile
> > > > > index 17ff1ac45..f6cea7fc2 100644
> > > > > --- a/lib/librte_eal/bsdapp/eal/Makefile
> > > > > +++ b/lib/librte_eal/bsdapp/eal/Makefile
> > > > > @@ -18,6 +18,7 @@ CFLAGS += $(WERROR_FLAGS) -O3
> > > > >  LDLIBS += -lexecinfo
> > > > >  LDLIBS += -lpthread
> > > > >  LDLIBS += -lgcc_s
> > > > > +LDLIBS += -lrte_kvargs
> > > > >  
> > > > >  EXPORT_MAP := ../../rte_eal_version.map
> > > > >  
> > > > > diff --git a/lib/librte_eal/common/eal_common_dev.c b/lib/librte_eal/common/eal_common_dev.c
> > > > > index cd071442f..1f6df2351 100644
> > > > > --- a/lib/librte_eal/common/eal_common_dev.c
> > > > > +++ b/lib/librte_eal/common/eal_common_dev.c
> > > > > @@ -10,9 +10,12 @@
> > > > >  
> > > > >  #include <rte_compat.h>
> > > > >  #include <rte_bus.h>
> > > > > +#include <rte_class.h>
> > > > >  #include <rte_dev.h>
> > > > >  #include <rte_devargs.h>
> > > > >  #include <rte_debug.h>
> > > > > +#include <rte_errno.h>
> > > > > +#include <rte_kvargs.h>
> > > > >  #include <rte_log.h>
> > > > >  
> > > > >  #include "eal_private.h"
> > > > > @@ -207,3 +210,147 @@ rte_eal_hotplug_remove(const char *busname, const char *devname)
> > > > >  	rte_eal_devargs_remove(busname, devname);
> > > > >  	return ret;
> > > > >  }
> > > > > +
> > > > > +static size_t
> > > > > +dev_layer_count(const char *s)
> > > > > +{
> > > > > +	size_t i = s ? 1 : 0;
> > > > > +
> > > > > +	while (s != NULL && s[0] != '\0') {
> > > > > +		i += s[0] == '/';
> > > > > +		s++;
> > > > > +	}
> > > > > +	return i;
> > > > > +}
> > > > > +
> > > > So the above code really just counts the number characters in the string,
> > > > omitting the delimiter character '/', right?  If thats all you want to do, you can just
> > > > use strtok and strnlen for that, cant you?
> > > 
> > > Will do.
> > > 
> > 
> > Answered too quickly.
> > No, this function only counts the number of occurences of '/' in the
> > text.
> > 
> > strtok could be used however in the main function.
> > Will see for a simpler implementation using it.
> > 
> 
> After a few tries, I see no benefit in using strtok.
> The original const text must still be tokenized, strtok would modify it.
> 
> Duplicating would force finding the references in the original text,
> which would make the code as complex as it is now.
> 
Oh, if you don't want to modify the string, then index() is what you want.  see
man 3 index, or man 3 rindex

Neil

> > > > 
> > > > Otherwise, this looks pretty good to me
> > > 
> > > Please look into the librte_kvargs compatibility patch as well (quite
> > > short). I'm very unhappy about the logging hack.
> > > There is always the solution of setting a function pointer on rte_log
> > > with the proper loglevels and so on.
> > > Ideally rte_log could be made independent (starting skimming EAL from
> > > all the fat), but this is much less trivial.
> > > 
> > > This implementation relies on librte_kvargs being available. If this is
> > > not the case, there isn't much point to it.
> > > 
> > > -- 
> > > Gaëtan Rivet
> > > 6WIND
> > 
> > -- 
> > Gaëtan Rivet
> > 6WIND
> 
> -- 
> Gaëtan Rivet
> 6WIND
>