diff mbox

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

Message ID f7f7115526378e8d83e03cf5e6bc3bead6571f24.1522105876.git.gaetan.rivet@6wind.com (mailing list archive)
State Superseded, archived
Headers

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation fail Compilation issues

Commit Message

Gaëtan Rivet March 26, 2018, 11:18 p.m. UTC 
  Parse a device description.
Split this description in their relevant part for each layers.
No dynamic allocation is performed.

Cc: Neil Horman <nhorman@tuxdriver.com>
Cc: "Wiles, Keith" <keith.wiles@intel.com>
Signed-off-by: Gaetan Rivet <gaetan.rivet@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(+)

Comments

Neil Horman March 27, 2018, 11:47 a.m. UTC | #1 
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@tuxdriver.com>
> Cc: "Wiles, Keith" <keith.wiles@intel.com>
> Signed-off-by: Gaetan Rivet <gaetan.rivet@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?

Otherwise, this looks pretty good to me
Neil
Gaëtan Rivet March 27, 2018, 12:40 p.m. UTC | #2 
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@tuxdriver.com>
> > Cc: "Wiles, Keith" <keith.wiles@intel.com>
> > Signed-off-by: Gaetan Rivet <gaetan.rivet@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.

> 
> 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 March 27, 2018, 1:04 p.m. UTC | #3 
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@tuxdriver.com>
> > > Cc: "Wiles, Keith" <keith.wiles@intel.com>
> > > Signed-off-by: Gaetan Rivet <gaetan.rivet@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.

> > 
> > 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
Wiles, Keith March 27, 2018, 1:08 p.m. UTC | #4 
> On Mar 27, 2018, at 7:40 AM, Gaëtan Rivet <gaetan.rivet@6wind.com> 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@tuxdriver.com>

>>> Cc: "Wiles, Keith" <keith.wiles@intel.com>

>>> Signed-off-by: Gaetan Rivet <gaetan.rivet@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.

> 

>> 

>> 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.


I thought RTE_LOG was setup correctly using defaults, some changes after I looked at must have changed that behavior I guess. That needs to be address at some point IMO, but not in this patch set.

> 

> This implementation relies on librte_kvargs being available. If this is

> not the case, there isn't much point to it.


Looked at the kvargs change and does look correct, but the above needs to be addressed or it will not be logged for normal usage of kvargs. I would have expected RTE_LOG to be able to handle the case of being called before it is inited and just use stderr. It needs to be looked at.

> 

> -- 

> Gaëtan Rivet

> 6WIND


Regards,
Keith
Neil Horman March 27, 2018, 6:26 p.m. UTC | #5 
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@tuxdriver.com>
> > > Cc: "Wiles, Keith" <keith.wiles@intel.com>
> > > Signed-off-by: Gaetan Rivet <gaetan.rivet@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.
> 
> > 
> > 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.
> 
just posted about that.  I agree with Keith, I don't think you should need that
patch.  RTE_LOG just calls rte_vlog which contains this code:

if (f == NULL) {
                f = default_log_stream;
                if (f == NULL) {
                        /*
                         * Grab the current value of stderr here, rather than
                         * just initializing default_log_stream to stderr. This
                         * ensures that we will always use the current value
                         * of stderr, even if the application closes and
                         * reopens it.
                         */
                        f = stderr;
                }
        }
}

Which I read as saying that the logging library should back off to stderr if its
not initialized yet.  If you've encountered a problem that made you need that
logging patch, it seems like you should be able to drop it, and we need to fix
the logging library.  Can you elaborate on what you ran into here?

Neil

> 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 March 27, 2018, 8:20 p.m. UTC | #6 
On Tue, Mar 27, 2018 at 02:26:33PM -0400, Neil Horman 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@tuxdriver.com>
> > > > Cc: "Wiles, Keith" <keith.wiles@intel.com>
> > > > Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
> > > > ---
> > > > 
> > > > This version uses librte_kvargs.
> > > > 
> > > 
> > > 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.
> > 
> just posted about that.  I agree with Keith, I don't think you should need that
> patch.  RTE_LOG just calls rte_vlog which contains this code:
> 
> if (f == NULL) {
>                 f = default_log_stream;
>                 if (f == NULL) {
>                         /*
>                          * Grab the current value of stderr here, rather than
>                          * just initializing default_log_stream to stderr. This
>                          * ensures that we will always use the current value
>                          * of stderr, even if the application closes and
>                          * reopens it.
>                          */
>                         f = stderr;
>                 }
>         }
> }
> 
> Which I read as saying that the logging library should back off to stderr if its
> not initialized yet.  If you've encountered a problem that made you need that
> logging patch, it seems like you should be able to drop it, and we need to fix
> the logging library.  Can you elaborate on what you ran into here?
> 
> Neil

Neat. The issue is that rte_log.h is not symlink-ed until librte_eal is
processed. rte_log cannot be included.

I know Keith and Bruce discussed a few months back the possibility of
having an initial linking pass. Personally I am still against this kind of
solution, hiding design issues under the rug.
Gaëtan Rivet March 27, 2018, 8:23 p.m. UTC | #7 
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@tuxdriver.com>
> > > > Cc: "Wiles, Keith" <keith.wiles@intel.com>
> > > > Signed-off-by: Gaetan Rivet <gaetan.rivet@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.

> > > 
> > > 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
Bruce Richardson March 27, 2018, 8:28 p.m. UTC | #8 
On Tue, Mar 27, 2018 at 10:20:40PM +0200, Gaëtan Rivet wrote:
> On Tue, Mar 27, 2018 at 02:26:33PM -0400, Neil Horman 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@tuxdriver.com>
> > > > > Cc: "Wiles, Keith" <keith.wiles@intel.com>
> > > > > Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
> > > > > ---
> > > > > 
> > > > > This version uses librte_kvargs.
> > > > > 
> > > > 
> > > > 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.
> > > 
> > just posted about that.  I agree with Keith, I don't think you should need that
> > patch.  RTE_LOG just calls rte_vlog which contains this code:
> > 
> > if (f == NULL) {
> >                 f = default_log_stream;
> >                 if (f == NULL) {
> >                         /*
> >                          * Grab the current value of stderr here, rather than
> >                          * just initializing default_log_stream to stderr. This
> >                          * ensures that we will always use the current value
> >                          * of stderr, even if the application closes and
> >                          * reopens it.
> >                          */
> >                         f = stderr;
> >                 }
> >         }
> > }
> > 
> > Which I read as saying that the logging library should back off to stderr if its
> > not initialized yet.  If you've encountered a problem that made you need that
> > logging patch, it seems like you should be able to drop it, and we need to fix
> > the logging library.  Can you elaborate on what you ran into here?
> > 
> > Neil
> 
> Neat. The issue is that rte_log.h is not symlink-ed until librte_eal is
> processed. rte_log cannot be included.
> 
Sure it can - just pass -I/path/to/eal as a cflag.

/Bruce
Gaëtan Rivet March 27, 2018, 8:35 p.m. UTC | #9 
On Tue, Mar 27, 2018 at 09:28:07PM +0100, Bruce Richardson wrote:
> On Tue, Mar 27, 2018 at 10:20:40PM +0200, Gaëtan Rivet wrote:
> > On Tue, Mar 27, 2018 at 02:26:33PM -0400, Neil Horman 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@tuxdriver.com>
> > > > > > Cc: "Wiles, Keith" <keith.wiles@intel.com>
> > > > > > Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
> > > > > > ---
> > > > > > 
> > > > > > This version uses librte_kvargs.
> > > > > > 
> > > > > 
> > > > > 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.
> > > > 
> > > just posted about that.  I agree with Keith, I don't think you should need that
> > > patch.  RTE_LOG just calls rte_vlog which contains this code:
> > > 
> > > if (f == NULL) {
> > >                 f = default_log_stream;
> > >                 if (f == NULL) {
> > >                         /*
> > >                          * Grab the current value of stderr here, rather than
> > >                          * just initializing default_log_stream to stderr. This
> > >                          * ensures that we will always use the current value
> > >                          * of stderr, even if the application closes and
> > >                          * reopens it.
> > >                          */
> > >                         f = stderr;
> > >                 }
> > >         }
> > > }
> > > 
> > > Which I read as saying that the logging library should back off to stderr if its
> > > not initialized yet.  If you've encountered a problem that made you need that
> > > logging patch, it seems like you should be able to drop it, and we need to fix
> > > the logging library.  Can you elaborate on what you ran into here?
> > > 
> > > Neil
> > 
> > Neat. The issue is that rte_log.h is not symlink-ed until librte_eal is
> > processed. rte_log cannot be included.
> > 
> Sure it can - just pass -I/path/to/eal as a cflag.
> 
> /Bruce

When you put it this way... :)

Sure, this is a solution, of which early symlink was a genericization.
I'm just not a fan of having co-dependent libraries.

But this will probably come to that.
Bruce Richardson March 27, 2018, 8:48 p.m. UTC | #10 
> -----Original Message-----
> From: Gaëtan Rivet [mailto:gaetan.rivet@6wind.com]
> Sent: Tuesday, March 27, 2018 9:35 PM
> To: Richardson, Bruce <bruce.richardson@intel.com>
> Cc: Neil Horman <nhorman@tuxdriver.com>; dev@dpdk.org; Wiles, Keith
> <keith.wiles@intel.com>
> Subject: Re: [dpdk-dev] [PATCH v3 10/20] eal/dev: implement device
> iteration initialization
> 
> On Tue, Mar 27, 2018 at 09:28:07PM +0100, Bruce Richardson wrote:
> > On Tue, Mar 27, 2018 at 10:20:40PM +0200, Gaëtan Rivet wrote:
> > > On Tue, Mar 27, 2018 at 02:26:33PM -0400, Neil Horman 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@tuxdriver.com>
> > > > > > > Cc: "Wiles, Keith" <keith.wiles@intel.com>
> > > > > > > Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
> > > > > > > ---
> > > > > > >
> > > > > > > This version uses librte_kvargs.
> > > > > > >
> > > > > >
> > > > > > 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.
> > > > >
> > > > just posted about that.  I agree with Keith, I don't think you
> > > > should need that patch.  RTE_LOG just calls rte_vlog which contains
> this code:
> > > >
> > > > if (f == NULL) {
> > > >                 f = default_log_stream;
> > > >                 if (f == NULL) {
> > > >                         /*
> > > >                          * Grab the current value of stderr here,
> rather than
> > > >                          * just initializing default_log_stream to
> stderr. This
> > > >                          * ensures that we will always use the
> current value
> > > >                          * of stderr, even if the application closes
> and
> > > >                          * reopens it.
> > > >                          */
> > > >                         f = stderr;
> > > >                 }
> > > >         }
> > > > }
> > > >
> > > > Which I read as saying that the logging library should back off to
> > > > stderr if its not initialized yet.  If you've encountered a
> > > > problem that made you need that logging patch, it seems like you
> > > > should be able to drop it, and we need to fix the logging library.
> Can you elaborate on what you ran into here?
> > > >
> > > > Neil
> > >
> > > Neat. The issue is that rte_log.h is not symlink-ed until librte_eal
> > > is processed. rte_log cannot be included.
> > >
> > Sure it can - just pass -I/path/to/eal as a cflag.
> >
> > /Bruce
> 
> When you put it this way... :)
> 
> Sure, this is a solution, of which early symlink was a genericization.
> I'm just not a fan of having co-dependent libraries.
> 
> But this will probably come to that.
> 

The other alternative is what was done with rte_compat.h - create a new lib
just with a single header file in it. Works ok too, and may seem less hacky
to some folks.

/Bruce
Neil Horman March 27, 2018, 11:26 p.m. UTC | #11 
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@tuxdriver.com>
> > > > > Cc: "Wiles, Keith" <keith.wiles@intel.com>
> > > > > Signed-off-by: Gaetan Rivet <gaetan.rivet@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
>
Neil Horman March 27, 2018, 11:53 p.m. UTC | #12 
On Tue, Mar 27, 2018 at 08:48:01PM +0000, Richardson, Bruce wrote:
> 
> 
> > -----Original Message-----
> > From: Gaëtan Rivet [mailto:gaetan.rivet@6wind.com]
> > Sent: Tuesday, March 27, 2018 9:35 PM
> > To: Richardson, Bruce <bruce.richardson@intel.com>
> > Cc: Neil Horman <nhorman@tuxdriver.com>; dev@dpdk.org; Wiles, Keith
> > <keith.wiles@intel.com>
> > Subject: Re: [dpdk-dev] [PATCH v3 10/20] eal/dev: implement device
> > iteration initialization
> > 
> > On Tue, Mar 27, 2018 at 09:28:07PM +0100, Bruce Richardson wrote:
> > > On Tue, Mar 27, 2018 at 10:20:40PM +0200, Gaëtan Rivet wrote:
> > > > On Tue, Mar 27, 2018 at 02:26:33PM -0400, Neil Horman 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@tuxdriver.com>
> > > > > > > > Cc: "Wiles, Keith" <keith.wiles@intel.com>
> > > > > > > > Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
> > > > > > > > ---
> > > > > > > >
> > > > > > > > This version uses librte_kvargs.
> > > > > > > >
> > > > > > >
> > > > > > > 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.
> > > > > >
> > > > > just posted about that.  I agree with Keith, I don't think you
> > > > > should need that patch.  RTE_LOG just calls rte_vlog which contains
> > this code:
> > > > >
> > > > > if (f == NULL) {
> > > > >                 f = default_log_stream;
> > > > >                 if (f == NULL) {
> > > > >                         /*
> > > > >                          * Grab the current value of stderr here,
> > rather than
> > > > >                          * just initializing default_log_stream to
> > stderr. This
> > > > >                          * ensures that we will always use the
> > current value
> > > > >                          * of stderr, even if the application closes
> > and
> > > > >                          * reopens it.
> > > > >                          */
> > > > >                         f = stderr;
> > > > >                 }
> > > > >         }
> > > > > }
> > > > >
> > > > > Which I read as saying that the logging library should back off to
> > > > > stderr if its not initialized yet.  If you've encountered a
> > > > > problem that made you need that logging patch, it seems like you
> > > > > should be able to drop it, and we need to fix the logging library.
> > Can you elaborate on what you ran into here?
> > > > >
> > > > > Neil
> > > >
> > > > Neat. The issue is that rte_log.h is not symlink-ed until librte_eal
> > > > is processed. rte_log cannot be included.
> > > >
> > > Sure it can - just pass -I/path/to/eal as a cflag.
> > >
> > > /Bruce
> > 
> > When you put it this way... :)
> > 
> > Sure, this is a solution, of which early symlink was a genericization.
> > I'm just not a fan of having co-dependent libraries.
> > 
> > But this will probably come to that.
> > 
> 
> The other alternative is what was done with rte_compat.h - create a new lib
> just with a single header file in it. Works ok too, and may seem less hacky
> to some folks.
> 
> /Bruce
> 
This seems like a good alternative to me.  I'm not entirely sure why the logging
functions are integrated to EAL anyway.  

Neil
Gaëtan Rivet March 28, 2018, 8:10 a.m. UTC | #13 
On Tue, Mar 27, 2018 at 07:53:46PM -0400, Neil Horman wrote:
> On Tue, Mar 27, 2018 at 08:48:01PM +0000, Richardson, Bruce wrote:
> > 
> > 
> > > -----Original Message-----
> > > From: Gaëtan Rivet [mailto:gaetan.rivet@6wind.com]
> > > Sent: Tuesday, March 27, 2018 9:35 PM
> > > To: Richardson, Bruce <bruce.richardson@intel.com>
> > > Cc: Neil Horman <nhorman@tuxdriver.com>; dev@dpdk.org; Wiles, Keith
> > > <keith.wiles@intel.com>
> > > Subject: Re: [dpdk-dev] [PATCH v3 10/20] eal/dev: implement device
> > > iteration initialization
> > > 
> > > On Tue, Mar 27, 2018 at 09:28:07PM +0100, Bruce Richardson wrote:
> > > > On Tue, Mar 27, 2018 at 10:20:40PM +0200, Gaëtan Rivet wrote:
> > > > > On Tue, Mar 27, 2018 at 02:26:33PM -0400, Neil Horman 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@tuxdriver.com>
> > > > > > > > > Cc: "Wiles, Keith" <keith.wiles@intel.com>
> > > > > > > > > Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
> > > > > > > > > ---
> > > > > > > > >
> > > > > > > > > This version uses librte_kvargs.
> > > > > > > > >
> > > > > > > >
> > > > > > > > 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.
> > > > > > >
> > > > > > just posted about that.  I agree with Keith, I don't think you
> > > > > > should need that patch.  RTE_LOG just calls rte_vlog which contains
> > > this code:
> > > > > >
> > > > > > if (f == NULL) {
> > > > > >                 f = default_log_stream;
> > > > > >                 if (f == NULL) {
> > > > > >                         /*
> > > > > >                          * Grab the current value of stderr here,
> > > rather than
> > > > > >                          * just initializing default_log_stream to
> > > stderr. This
> > > > > >                          * ensures that we will always use the
> > > current value
> > > > > >                          * of stderr, even if the application closes
> > > and
> > > > > >                          * reopens it.
> > > > > >                          */
> > > > > >                         f = stderr;
> > > > > >                 }
> > > > > >         }
> > > > > > }
> > > > > >
> > > > > > Which I read as saying that the logging library should back off to
> > > > > > stderr if its not initialized yet.  If you've encountered a
> > > > > > problem that made you need that logging patch, it seems like you
> > > > > > should be able to drop it, and we need to fix the logging library.
> > > Can you elaborate on what you ran into here?
> > > > > >
> > > > > > Neil
> > > > >
> > > > > Neat. The issue is that rte_log.h is not symlink-ed until librte_eal
> > > > > is processed. rte_log cannot be included.
> > > > >
> > > > Sure it can - just pass -I/path/to/eal as a cflag.
> > > >
> > > > /Bruce
> > > 
> > > When you put it this way... :)
> > > 
> > > Sure, this is a solution, of which early symlink was a genericization.
> > > I'm just not a fan of having co-dependent libraries.
> > > 
> > > But this will probably come to that.
> > > 
> > 
> > The other alternative is what was done with rte_compat.h - create a new lib
> > just with a single header file in it. Works ok too, and may seem less hacky
> > to some folks.
> > 
> > /Bruce
> > 
> This seems like a good alternative to me.  I'm not entirely sure why the logging
> functions are integrated to EAL anyway.  
> 
> Neil
>  

As I said earlier:

> > > > > > > Ideally rte_log could be made independent (starting skimming EAL
> > > > > > > from all the fat), but this is much less trivial.

rte_log could certainly stand on its own. I quickly attempted to make it
a library, but this is too much work at this point. I think the EAL
should be broken down, it is too monolithic. Problem is that many
other libraries / applications, now relies on parts of the EAL that
would need to be moved.

So any effort in this direction will be difficult to undertake (and
badly received by the community, with good reasons), especially when
the workaround is an additional -I cflag.
Neil Horman March 28, 2018, 11:17 a.m. UTC | #14 
On Wed, Mar 28, 2018 at 10:10:07AM +0200, Gaëtan Rivet wrote:
> On Tue, Mar 27, 2018 at 07:53:46PM -0400, Neil Horman wrote:
> > On Tue, Mar 27, 2018 at 08:48:01PM +0000, Richardson, Bruce wrote:
> > > 
> > > 
> > > > -----Original Message-----
> > > > From: Gaëtan Rivet [mailto:gaetan.rivet@6wind.com]
> > > > Sent: Tuesday, March 27, 2018 9:35 PM
> > > > To: Richardson, Bruce <bruce.richardson@intel.com>
> > > > Cc: Neil Horman <nhorman@tuxdriver.com>; dev@dpdk.org; Wiles, Keith
> > > > <keith.wiles@intel.com>
> > > > Subject: Re: [dpdk-dev] [PATCH v3 10/20] eal/dev: implement device
> > > > iteration initialization
> > > > 
> > > > On Tue, Mar 27, 2018 at 09:28:07PM +0100, Bruce Richardson wrote:
> > > > > On Tue, Mar 27, 2018 at 10:20:40PM +0200, Gaëtan Rivet wrote:
> > > > > > On Tue, Mar 27, 2018 at 02:26:33PM -0400, Neil Horman 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@tuxdriver.com>
> > > > > > > > > > Cc: "Wiles, Keith" <keith.wiles@intel.com>
> > > > > > > > > > Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
> > > > > > > > > > ---
> > > > > > > > > >
> > > > > > > > > > This version uses librte_kvargs.
> > > > > > > > > >
> > > > > > > > >
> > > > > > > > > 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.
> > > > > > > >
> > > > > > > just posted about that.  I agree with Keith, I don't think you
> > > > > > > should need that patch.  RTE_LOG just calls rte_vlog which contains
> > > > this code:
> > > > > > >
> > > > > > > if (f == NULL) {
> > > > > > >                 f = default_log_stream;
> > > > > > >                 if (f == NULL) {
> > > > > > >                         /*
> > > > > > >                          * Grab the current value of stderr here,
> > > > rather than
> > > > > > >                          * just initializing default_log_stream to
> > > > stderr. This
> > > > > > >                          * ensures that we will always use the
> > > > current value
> > > > > > >                          * of stderr, even if the application closes
> > > > and
> > > > > > >                          * reopens it.
> > > > > > >                          */
> > > > > > >                         f = stderr;
> > > > > > >                 }
> > > > > > >         }
> > > > > > > }
> > > > > > >
> > > > > > > Which I read as saying that the logging library should back off to
> > > > > > > stderr if its not initialized yet.  If you've encountered a
> > > > > > > problem that made you need that logging patch, it seems like you
> > > > > > > should be able to drop it, and we need to fix the logging library.
> > > > Can you elaborate on what you ran into here?
> > > > > > >
> > > > > > > Neil
> > > > > >
> > > > > > Neat. The issue is that rte_log.h is not symlink-ed until librte_eal
> > > > > > is processed. rte_log cannot be included.
> > > > > >
> > > > > Sure it can - just pass -I/path/to/eal as a cflag.
> > > > >
> > > > > /Bruce
> > > > 
> > > > When you put it this way... :)
> > > > 
> > > > Sure, this is a solution, of which early symlink was a genericization.
> > > > I'm just not a fan of having co-dependent libraries.
> > > > 
> > > > But this will probably come to that.
> > > > 
> > > 
> > > The other alternative is what was done with rte_compat.h - create a new lib
> > > just with a single header file in it. Works ok too, and may seem less hacky
> > > to some folks.
> > > 
> > > /Bruce
> > > 
> > This seems like a good alternative to me.  I'm not entirely sure why the logging
> > functions are integrated to EAL anyway.  
> > 
> > Neil
> >  
> 
> As I said earlier:
> 
> > > > > > > > Ideally rte_log could be made independent (starting skimming EAL
> > > > > > > > from all the fat), but this is much less trivial.
> 
> rte_log could certainly stand on its own. I quickly attempted to make it
> a library, but this is too much work at this point. I think the EAL
> should be broken down, it is too monolithic. Problem is that many
> other libraries / applications, now relies on parts of the EAL that
> would need to be moved.
> 
> So any effort in this direction will be difficult to undertake (and
> badly received by the community, with good reasons), especially when
> the workaround is an additional -I cflag.
> 
I'm fine with just adding another include path to the CFLAGS for this purpose in
the short term, just saying I'm not sure why the log library got integrated into
EAL in the first place.

Neil

> -- 
> Gaëtan Rivet
> 6WIND
>
Gaëtan Rivet March 28, 2018, 12:48 p.m. UTC | #15 
On Tue, Mar 27, 2018 at 07:26:41PM -0400, Neil Horman wrote:
> 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@tuxdriver.com>
> > > > > > Cc: "Wiles, Keith" <keith.wiles@intel.com>
> > > > > > Signed-off-by: Gaetan Rivet <gaetan.rivet@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
> 

strchr is already used when necessary, and index is removed since
POSIX.1-2008.

Thanks for the suggestions, it was still helpful to go over the code and
see if there were possible improvements.
Thomas Monjalon April 22, 2018, 10:29 p.m. UTC | #16 
28/03/2018 13:17, Neil Horman:
> On Wed, Mar 28, 2018 at 10:10:07AM +0200, Gaëtan Rivet wrote:
> > On Tue, Mar 27, 2018 at 07:53:46PM -0400, Neil Horman wrote:
> > > On Tue, Mar 27, 2018 at 08:48:01PM +0000, Richardson, Bruce wrote:
> > > > From: Gaëtan Rivet [mailto:gaetan.rivet@6wind.com]
> > > > > On Tue, Mar 27, 2018 at 09:28:07PM +0100, Bruce Richardson wrote:
> > > > > > On Tue, Mar 27, 2018 at 10:20:40PM +0200, Gaëtan Rivet wrote:
> > > > > > > Neat. The issue is that rte_log.h is not symlink-ed until librte_eal
> > > > > > > is processed. rte_log cannot be included.
> > > > > > >
> > > > > > Sure it can - just pass -I/path/to/eal as a cflag.
> > > > > >
> > > > > > /Bruce
> > > > > 
> > > > > When you put it this way... :)
> > > > > 
> > > > > Sure, this is a solution, of which early symlink was a genericization.
> > > > > I'm just not a fan of having co-dependent libraries.
> > > > > 
> > > > > But this will probably come to that.
> > > > > 
> > > > 
> > > > The other alternative is what was done with rte_compat.h - create a new lib
> > > > just with a single header file in it. Works ok too, and may seem less hacky
> > > > to some folks.
> > > > 
> > > > /Bruce
> > > > 
> > > This seems like a good alternative to me.  I'm not entirely sure why the logging
> > > functions are integrated to EAL anyway.  
> > > 
> > > Neil
> > >  
> > 
> > As I said earlier:
> > 
> > > > > > > > > Ideally rte_log could be made independent (starting skimming EAL
> > > > > > > > > from all the fat), but this is much less trivial.
> > 
> > rte_log could certainly stand on its own. I quickly attempted to make it
> > a library, but this is too much work at this point. I think the EAL
> > should be broken down, it is too monolithic. Problem is that many
> > other libraries / applications, now relies on parts of the EAL that
> > would need to be moved.
> > 
> > So any effort in this direction will be difficult to undertake (and
> > badly received by the community, with good reasons), especially when
> > the workaround is an additional -I cflag.
> > 
> I'm fine with just adding another include path to the CFLAGS for this purpose in
> the short term, just saying I'm not sure why the log library got integrated into
> EAL in the first place.
> 
> Neil

The log functions must be in EAL because they are different per-environment.
The basic functions are the reason of EAL.
I think the problem is somewhere else: we are trying to implemement the complex
device logic in EAL.
We should move all the bus and device logics outside of EAL, in my opinion.
We should also move all the options parsing outside of EAL, and make it optional.

Anyway, these are considerations for the long run.
diff mbox

Patch

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;
+}
+
+int __rte_experimental
+rte_dev_iterator_init(struct rte_dev_iterator *it,
+		      const char *devstr)
+{
+	struct {
+		const char *key;
+		const char *str;
+		struct rte_kvargs *kvlist;
+	} layers[] = {
+		{ "bus=",    NULL, NULL, },
+		{ "class=",  NULL, NULL, },
+		{ "driver=", NULL, NULL, },
+	};
+	struct rte_kvargs_pair *kv = NULL;
+	struct rte_class *cls = NULL;
+	struct rte_bus *bus = NULL;
+	const char *s = devstr;
+	size_t nblayer;
+	size_t i = 0;
+
+	/* Having both busstr and clsstr NULL is illegal,
+	 * marking this iterator as invalid unless
+	 * everything goes well.
+	 */
+	it->busstr = NULL;
+	it->clsstr = NULL;
+	/* Split each sub-lists. */
+	nblayer = dev_layer_count(devstr);
+	if (nblayer > RTE_DIM(layers)) {
+		RTE_LOG(ERR, EAL, "Invalid query: too many layers (%zu)\n",
+			nblayer);
+		rte_errno = EINVAL;
+		goto get_out;
+	}
+	while (s != NULL) {
+		char *copy;
+		char *end;
+
+		if (strncmp(layers[i].key, s,
+			    strlen(layers[i].key)))
+			goto next_layer;
+		layers[i].str = s;
+		copy = strdup(s);
+		if (copy == NULL) {
+			RTE_LOG(ERR, EAL, "OOM\n");
+			rte_errno = ENOMEM;
+			goto get_out;
+		}
+		end = strchr(copy, '/');
+		end = end ? end : strchr(copy, '\0');
+		end[0] = '\0';
+		layers[i].kvlist = rte_kvargs_parse(copy, NULL);
+		free(copy);
+		if (layers[i].kvlist == NULL) {
+			RTE_LOG(ERR, EAL, "Could not parse %s\n", s);
+			rte_errno = EINVAL;
+			goto get_out;
+		}
+		s = strchr(s, '/');
+		if (s != NULL)
+			s++;
+next_layer:
+		if (i >= RTE_DIM(layers)) {
+			RTE_LOG(ERR, EAL, "Unrecognized layer %s\n", s);
+			rte_errno = EINVAL;
+			goto get_out;
+		}
+		i++;
+	}
+	/* Parse each sub-list. */
+	for (i = 0; i < RTE_DIM(layers); i++) {
+		if (layers[i].kvlist == NULL)
+			continue;
+		kv = &layers[i].kvlist->pairs[0];
+		if (strcmp(kv->key, "bus") == 0) {
+			bus = rte_bus_find_by_name(kv->value);
+			if (bus == NULL) {
+				RTE_LOG(ERR, EAL, "Could not find bus \"%s\"\n",
+					kv->value);
+				rte_errno = EFAULT;
+				goto get_out;
+			}
+		} else if (strcmp(kv->key, "class") == 0) {
+			cls = rte_class_find_by_name(kv->value);
+			if (cls == NULL) {
+				RTE_LOG(ERR, EAL, "Could not find class \"%s\"\n",
+					kv->value);
+				rte_errno = EFAULT;
+				goto get_out;
+			}
+		} else if (strcmp(kv->key, "driver") == 0) {
+			/* Ignore */
+			continue;
+		}
+	}
+	/* The string should have at least
+	 * one layer specified.
+	 */
+	if (bus == NULL && cls == NULL) {
+		RTE_LOG(ERR, EAL,
+			"Either bus or class must be specified.\n");
+		rte_errno = EINVAL;
+		goto get_out;
+	}
+	if (bus != NULL && bus->dev_iterate == NULL) {
+		RTE_LOG(ERR, EAL, "Bus %s not supported\n", bus->name);
+		rte_errno = ENOTSUP;
+		goto get_out;
+	}
+	if (cls != NULL && cls->dev_iterate == NULL) {
+		RTE_LOG(ERR, EAL, "Class %s not supported\n", cls->name);
+		rte_errno = ENOTSUP;
+		goto get_out;
+	}
+	/* Fill iterator fields. */
+	if (bus != NULL)
+		it->busstr = layers[0].str;
+	if (cls != NULL)
+		it->clsstr = layers[1].str;
+	it->devstr = devstr;
+	it->bus = bus;
+	it->cls = cls;
+	it->device = NULL;
+	it->class_device = NULL;
+get_out:
+	for (i = 0; i < RTE_DIM(layers); i++) {
+		if (layers[i].kvlist)
+			rte_kvargs_free(layers[i].kvlist);
+	}
+	return -rte_errno;
+}
diff --git a/lib/librte_eal/common/include/rte_dev.h b/lib/librte_eal/common/include/rte_dev.h
index 937ff6079..7ce13e068 100644
--- a/lib/librte_eal/common/include/rte_dev.h
+++ b/lib/librte_eal/common/include/rte_dev.h
@@ -310,6 +310,29 @@  typedef void *(*rte_dev_iterate_t)(const void *start,
 				   const char *devstr,
 				   const struct rte_dev_iterator *it);
 
+/**
+ * Initializes a device iterator.
+ *
+ * This iterator allows accessing a list of devices matching a criteria.
+ * The device matching is made among all buses and classes currently registered,
+ * filtered by the device description given as parameter.
+ *
+ * This function will not allocate any memory. It is safe to stop the
+ * iteration at any moment and let the iterator go out of context.
+ *
+ * @param it
+ *   Device iterator handle.
+ *
+ * @param str
+ *   Device description string.
+ *
+ * @return
+ *   0 on successful initialization.
+ *   <0 on error.
+ */
+int __rte_experimental
+rte_dev_iterator_init(struct rte_dev_iterator *it, const char *str);
+
 #ifdef __cplusplus
 }
 #endif
diff --git a/lib/librte_eal/linuxapp/eal/Makefile b/lib/librte_eal/linuxapp/eal/Makefile
index a3edbbe76..87caa23a1 100644
--- a/lib/librte_eal/linuxapp/eal/Makefile
+++ b/lib/librte_eal/linuxapp/eal/Makefile
@@ -27,6 +27,7 @@  LDLIBS += -lrt
 ifeq ($(CONFIG_RTE_EAL_NUMA_AWARE_HUGEPAGES),y)
 LDLIBS += -lnuma
 endif
+LDLIBS += -lrte_kvargs
 
 # specific to linuxapp exec-env
 SRCS-$(CONFIG_RTE_EXEC_ENV_LINUXAPP) := eal.c
diff --git a/lib/librte_eal/rte_eal_version.map b/lib/librte_eal/rte_eal_version.map
index 910cb23c9..921da3075 100644
--- a/lib/librte_eal/rte_eal_version.map
+++ b/lib/librte_eal/rte_eal_version.map
@@ -228,6 +228,7 @@  EXPERIMENTAL {
 	rte_mp_sendmsg;
 	rte_mp_request;
 	rte_mp_reply;
+	rte_dev_iterator_init;
 	rte_service_attr_get;
 	rte_service_attr_reset_all;
 	rte_service_component_register;