DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH] usertools: fix pmdinfo parsing
@ 2020-11-03 18:39 David Marchand
  2020-11-03 19:27 ` Robin Jarry
                   ` (2 more replies)
  0 siblings, 3 replies; 15+ messages in thread
From: David Marchand @ 2020-11-03 18:39 UTC (permalink / raw)
  To: dev
  Cc: bruce.richardson, robin.jarry, Neil Horman, Rosen Xu,
	Andrew Rybchenko, Luca Boccassi

This script was using the librte_pmd prefix has a filter to follow
DT_NEEDED entries.
Now that we changed the driver names, update this heuristic with an
explicit list of device classes.

Fixes: a20b2c01a7a1 ("build: standardize component names and defines")

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 usertools/dpdk-pmdinfo.py | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/usertools/dpdk-pmdinfo.py b/usertools/dpdk-pmdinfo.py
index 1661982791..687a9fd032 100755
--- a/usertools/dpdk-pmdinfo.py
+++ b/usertools/dpdk-pmdinfo.py
@@ -450,7 +450,10 @@ def process_dt_needed_entries(self):
         for tag in dynsec.iter_tags():
             # pyelftools may return byte-strings, force decode them
             if force_unicode(tag.entry.d_tag) == 'DT_NEEDED':
-                if 'librte_pmd' in force_unicode(tag.needed):
+                words = force_unicode(tag.needed).split('_')
+                if words and len(words) >= 3 and words[0] == 'librte' and \
+                   words[1] in ['baseband', 'compress', 'crypto', 'event',
+                                'net', 'raw', 'regex', 'vdpa']:
                     library = search_file(force_unicode(tag.needed),
                                           runpath + ":" + ldlibpath +
                                           ":/usr/lib64:/lib64:/usr/lib:/lib")
-- 
2.23.0


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [dpdk-dev] [PATCH] usertools: fix pmdinfo parsing
  2020-11-03 18:39 [dpdk-dev] [PATCH] usertools: fix pmdinfo parsing David Marchand
@ 2020-11-03 19:27 ` Robin Jarry
  2020-11-03 20:20   ` David Marchand
  2020-11-04  9:40 ` [dpdk-dev] [PATCH v2] " David Marchand
  2020-11-04 15:57 ` [dpdk-dev] [PATCH v3] " David Marchand
  2 siblings, 1 reply; 15+ messages in thread
From: Robin Jarry @ 2020-11-03 19:27 UTC (permalink / raw)
  To: David Marchand
  Cc: dev, bruce.richardson, Neil Horman, Rosen Xu, Andrew Rybchenko,
	Luca Boccassi

2020-11-03, David Marchand:
> This script was using the librte_pmd prefix has a filter to follow
> DT_NEEDED entries.
> Now that we changed the driver names, update this heuristic with an
> explicit list of device classes.
> 
> Fixes: a20b2c01a7a1 ("build: standardize component names and defines")
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
>  usertools/dpdk-pmdinfo.py | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/usertools/dpdk-pmdinfo.py b/usertools/dpdk-pmdinfo.py
> index 1661982791..687a9fd032 100755
> --- a/usertools/dpdk-pmdinfo.py
> +++ b/usertools/dpdk-pmdinfo.py
> @@ -450,7 +450,10 @@ def process_dt_needed_entries(self):
>          for tag in dynsec.iter_tags():
>              # pyelftools may return byte-strings, force decode them
>              if force_unicode(tag.entry.d_tag) == 'DT_NEEDED':
> -                if 'librte_pmd' in force_unicode(tag.needed):
> +                words = force_unicode(tag.needed).split('_')
> +                if words and len(words) >= 3 and words[0] == 'librte' and \
> +                   words[1] in ['baseband', 'compress', 'crypto', 'event',
> +                                'net', 'raw', 'regex', 'vdpa']:

This code is already ugly and I don't have much better to suggest...

Acked-by: Robin Jarry <robin.jarry@6wind.com>

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [dpdk-dev] [PATCH] usertools: fix pmdinfo parsing
  2020-11-03 19:27 ` Robin Jarry
@ 2020-11-03 20:20   ` David Marchand
  2020-11-03 23:54     ` Stephen Hemminger
  2020-11-04  8:06     ` Robin Jarry
  0 siblings, 2 replies; 15+ messages in thread
From: David Marchand @ 2020-11-03 20:20 UTC (permalink / raw)
  To: Robin Jarry
  Cc: dev, Bruce Richardson, Neil Horman, Rosen Xu, Andrew Rybchenko,
	Luca Boccassi

On Tue, Nov 3, 2020 at 8:27 PM Robin Jarry <robin.jarry@6wind.com> wrote:
> 2020-11-03, David Marchand:
> > This script was using the librte_pmd prefix has a filter to follow

as*

> > DT_NEEDED entries.
> > Now that we changed the driver names, update this heuristic with an
> > explicit list of device classes.
> >
> > Fixes: a20b2c01a7a1 ("build: standardize component names and defines")
> >
> > Signed-off-by: David Marchand <david.marchand@redhat.com>
> > ---
> >  usertools/dpdk-pmdinfo.py | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/usertools/dpdk-pmdinfo.py b/usertools/dpdk-pmdinfo.py
> > index 1661982791..687a9fd032 100755
> > --- a/usertools/dpdk-pmdinfo.py
> > +++ b/usertools/dpdk-pmdinfo.py
> > @@ -450,7 +450,10 @@ def process_dt_needed_entries(self):
> >          for tag in dynsec.iter_tags():
> >              # pyelftools may return byte-strings, force decode them
> >              if force_unicode(tag.entry.d_tag) == 'DT_NEEDED':
> > -                if 'librte_pmd' in force_unicode(tag.needed):
> > +                words = force_unicode(tag.needed).split('_')
> > +                if words and len(words) >= 3 and words[0] == 'librte' and \
> > +                   words[1] in ['baseband', 'compress', 'crypto', 'event',
> > +                                'net', 'raw', 'regex', 'vdpa']:
>
> This code is already ugly and I don't have much better to suggest...

Less ugly with a regular expression?

if re.match(r"^librte_(baseband|compress|crypto|event|net|raw|regex|vdpa)_",
            force_unicode(tag.needed)):


-- 
David Marchand


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [dpdk-dev] [PATCH] usertools: fix pmdinfo parsing
  2020-11-03 20:20   ` David Marchand
@ 2020-11-03 23:54     ` Stephen Hemminger
  2020-11-04  8:04       ` Olivier Matz
  2020-11-04  8:06     ` Robin Jarry
  1 sibling, 1 reply; 15+ messages in thread
From: Stephen Hemminger @ 2020-11-03 23:54 UTC (permalink / raw)
  To: David Marchand
  Cc: Robin Jarry, dev, Bruce Richardson, Neil Horman, Rosen Xu,
	Andrew Rybchenko, Luca Boccassi

On Tue, 3 Nov 2020 21:20:43 +0100
David Marchand <david.marchand@redhat.com> wrote:

> On Tue, Nov 3, 2020 at 8:27 PM Robin Jarry <robin.jarry@6wind.com> wrote:
> > 2020-11-03, David Marchand:  
> > > This script was using the librte_pmd prefix has a filter to follow  
> 
> as*
> 
> > > DT_NEEDED entries.
> > > Now that we changed the driver names, update this heuristic with an
> > > explicit list of device classes.
> > >
> > > Fixes: a20b2c01a7a1 ("build: standardize component names and defines")
> > >
> > > Signed-off-by: David Marchand <david.marchand@redhat.com>
> > > ---
> > >  usertools/dpdk-pmdinfo.py | 5 ++++-
> > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/usertools/dpdk-pmdinfo.py b/usertools/dpdk-pmdinfo.py
> > > index 1661982791..687a9fd032 100755
> > > --- a/usertools/dpdk-pmdinfo.py
> > > +++ b/usertools/dpdk-pmdinfo.py
> > > @@ -450,7 +450,10 @@ def process_dt_needed_entries(self):
> > >          for tag in dynsec.iter_tags():
> > >              # pyelftools may return byte-strings, force decode them
> > >              if force_unicode(tag.entry.d_tag) == 'DT_NEEDED':
> > > -                if 'librte_pmd' in force_unicode(tag.needed):
> > > +                words = force_unicode(tag.needed).split('_')
> > > +                if words and len(words) >= 3 and words[0] == 'librte' and \
> > > +                   words[1] in ['baseband', 'compress', 'crypto', 'event',
> > > +                                'net', 'raw', 'regex', 'vdpa']:  
> >
> > This code is already ugly and I don't have much better to suggest...  
> 
> Less ugly with a regular expression?
> 
> if re.match(r"^librte_(baseband|compress|crypto|event|net|raw|regex|vdpa)_",
>             force_unicode(tag.needed)):
> 
> 

I prefer the list approach, much less error prone.
The code would read better if the list of drivers was in a variable
instead of a long line of code and the string processing was done separate from
the test.
Something like
                   drivers = ['baseband', 'compress'...
                   prefix = 'librte_pmd'
		   name = force_unicode(tag.needed)
                   if name.startswith(prefix):
                       suffix = name[len(prefix):]
                       if suffx in drivers:


                          

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [dpdk-dev] [PATCH] usertools: fix pmdinfo parsing
  2020-11-03 23:54     ` Stephen Hemminger
@ 2020-11-04  8:04       ` Olivier Matz
  0 siblings, 0 replies; 15+ messages in thread
From: Olivier Matz @ 2020-11-04  8:04 UTC (permalink / raw)
  To: Stephen Hemminger
  Cc: David Marchand, Robin Jarry, dev, Bruce Richardson, Neil Horman,
	Rosen Xu, Andrew Rybchenko, Luca Boccassi

On Tue, Nov 03, 2020 at 03:54:39PM -0800, Stephen Hemminger wrote:
> On Tue, 3 Nov 2020 21:20:43 +0100
> David Marchand <david.marchand@redhat.com> wrote:
> 
> > On Tue, Nov 3, 2020 at 8:27 PM Robin Jarry <robin.jarry@6wind.com> wrote:
> > > 2020-11-03, David Marchand:  
> > > > This script was using the librte_pmd prefix has a filter to follow  
> > 
> > as*
> > 
> > > > DT_NEEDED entries.
> > > > Now that we changed the driver names, update this heuristic with an
> > > > explicit list of device classes.
> > > >
> > > > Fixes: a20b2c01a7a1 ("build: standardize component names and defines")
> > > >
> > > > Signed-off-by: David Marchand <david.marchand@redhat.com>
> > > > ---
> > > >  usertools/dpdk-pmdinfo.py | 5 ++++-
> > > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/usertools/dpdk-pmdinfo.py b/usertools/dpdk-pmdinfo.py
> > > > index 1661982791..687a9fd032 100755
> > > > --- a/usertools/dpdk-pmdinfo.py
> > > > +++ b/usertools/dpdk-pmdinfo.py
> > > > @@ -450,7 +450,10 @@ def process_dt_needed_entries(self):
> > > >          for tag in dynsec.iter_tags():
> > > >              # pyelftools may return byte-strings, force decode them
> > > >              if force_unicode(tag.entry.d_tag) == 'DT_NEEDED':
> > > > -                if 'librte_pmd' in force_unicode(tag.needed):
> > > > +                words = force_unicode(tag.needed).split('_')
> > > > +                if words and len(words) >= 3 and words[0] == 'librte' and \
> > > > +                   words[1] in ['baseband', 'compress', 'crypto', 'event',
> > > > +                                'net', 'raw', 'regex', 'vdpa']:  
> > >
> > > This code is already ugly and I don't have much better to suggest...  
> > 
> > Less ugly with a regular expression?
> > 
> > if re.match(r"^librte_(baseband|compress|crypto|event|net|raw|regex|vdpa)_",
> >             force_unicode(tag.needed)):
> > 
> > 
> 
> I prefer the list approach, much less error prone.
> The code would read better if the list of drivers was in a variable
> instead of a long line of code and the string processing was done separate from
> the test.
> Something like
>                    drivers = ['baseband', 'compress'...
>                    prefix = 'librte_pmd'
> 		   name = force_unicode(tag.needed)
>                    if name.startswith(prefix):
>                        suffix = name[len(prefix):]
>                        if suffx in drivers:
> 
> 
>                           

I agree with Stephen that the list approach and an intermediate
variable is clearer than regex. However, I'd keep the split() as
in the original patch.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [dpdk-dev] [PATCH] usertools: fix pmdinfo parsing
  2020-11-03 20:20   ` David Marchand
  2020-11-03 23:54     ` Stephen Hemminger
@ 2020-11-04  8:06     ` Robin Jarry
  1 sibling, 0 replies; 15+ messages in thread
From: Robin Jarry @ 2020-11-04  8:06 UTC (permalink / raw)
  To: David Marchand
  Cc: dev, Bruce Richardson, Neil Horman, Rosen Xu, Andrew Rybchenko,
	Luca Boccassi

2020-11-03, David Marchand:
> Less ugly with a regular expression?
> 
> if re.match(r"^librte_(baseband|compress|crypto|event|net|raw|regex|vdpa)_",
>             force_unicode(tag.needed)):

No, that's worse :D

As Stephen said, maybe it would be more readable if the list of
supported classes were in a constant somewhere.

  DRIVER_CLASSES = (
      'baseband',
      'compress',
      'crypto',
      'event',
      'net',
      'raw,
      'regex',
      'vdpa',
  )

-- 
Robin

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [dpdk-dev] [PATCH v2] usertools: fix pmdinfo parsing
  2020-11-03 18:39 [dpdk-dev] [PATCH] usertools: fix pmdinfo parsing David Marchand
  2020-11-03 19:27 ` Robin Jarry
@ 2020-11-04  9:40 ` David Marchand
  2020-11-04 10:32   ` Bruce Richardson
                     ` (2 more replies)
  2020-11-04 15:57 ` [dpdk-dev] [PATCH v3] " David Marchand
  2 siblings, 3 replies; 15+ messages in thread
From: David Marchand @ 2020-11-04  9:40 UTC (permalink / raw)
  To: dev
  Cc: bruce.richardson, robin.jarry, stephen, olivier.matz,
	Neil Horman, Rosen Xu, Andrew Rybchenko, Luca Boccassi

This script inspects an ELF file (binary or shared library) and its
linked dependencies by following DT_NEEDED tags.
So far a simple librte_pmd prefix was used as a filter.
Now that we changed the driver library names, update this heuristic with
an explicit list of all driver classes.

Fixes: a20b2c01a7a1 ("build: standardize component names and defines")

Signed-off-by: David Marchand <david.marchand@redhat.com>
Acked-by: Robin Jarry <robin.jarry@6wind.com>
---
Changelog since v1:
- moved driver classes list as a class variable and did some cosmetic
  change for readibility,
- used dpdk_driver_classes variable name in the hope that someone changing
  meson will catch this script too,
- added bus, common, mempool and raw driver classes as some of them do
  carry some pmdinfo stuff and were skipped so far but I found no
  indication this skipping was intended,

---
 usertools/dpdk-pmdinfo.py | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/usertools/dpdk-pmdinfo.py b/usertools/dpdk-pmdinfo.py
index 1661982791..55a55affde 100755
--- a/usertools/dpdk-pmdinfo.py
+++ b/usertools/dpdk-pmdinfo.py
@@ -430,6 +430,20 @@ def get_dt_runpath(self, dynsec):
                 return force_unicode(tag.runpath)
         return ""
 
+    dpdk_driver_classes = (
+        'baseband',
+        'bus',
+        'common',
+        'compress',
+        'crypto',
+        'event',
+        'mempool',
+        'net',
+        'raw',
+        'regex',
+        'vdpa',
+    )
+
     def process_dt_needed_entries(self):
         """ Look to see if there are any DT_NEEDED entries in the binary
             And process those if there are
@@ -450,7 +464,11 @@ def process_dt_needed_entries(self):
         for tag in dynsec.iter_tags():
             # pyelftools may return byte-strings, force decode them
             if force_unicode(tag.entry.d_tag) == 'DT_NEEDED':
-                if 'librte_pmd' in force_unicode(tag.needed):
+                words = force_unicode(tag.needed).split('_')
+                if len(words) < 3:
+                    continue
+                prefix, drv_class = words[:2]
+                if prefix == 'librte' and drv_class in self.dpdk_driver_classes:
                     library = search_file(force_unicode(tag.needed),
                                           runpath + ":" + ldlibpath +
                                           ":/usr/lib64:/lib64:/usr/lib:/lib")
-- 
2.23.0


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [dpdk-dev] [PATCH v2] usertools: fix pmdinfo parsing
  2020-11-04  9:40 ` [dpdk-dev] [PATCH v2] " David Marchand
@ 2020-11-04 10:32   ` Bruce Richardson
  2020-11-04 10:42     ` David Marchand
  2020-11-05 11:46   ` Xu, Rosen
  2020-11-12 13:28   ` David Marchand
  2 siblings, 1 reply; 15+ messages in thread
From: Bruce Richardson @ 2020-11-04 10:32 UTC (permalink / raw)
  To: David Marchand
  Cc: dev, robin.jarry, stephen, olivier.matz, Neil Horman, Rosen Xu,
	Andrew Rybchenko, Luca Boccassi

On Wed, Nov 04, 2020 at 10:40:33AM +0100, David Marchand wrote:
> This script inspects an ELF file (binary or shared library) and its
> linked dependencies by following DT_NEEDED tags.
> So far a simple librte_pmd prefix was used as a filter.
> Now that we changed the driver library names, update this heuristic with
> an explicit list of all driver classes.
> 
> Fixes: a20b2c01a7a1 ("build: standardize component names and defines")
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> Acked-by: Robin Jarry <robin.jarry@6wind.com>
> ---
> Changelog since v1:
> - moved driver classes list as a class variable and did some cosmetic
>   change for readibility,
> - used dpdk_driver_classes variable name in the hope that someone changing
>   meson will catch this script too,

Good idea, but probably not enough. I think adding a comment to the
meson.build file to update this as well would be good.

/Bruce

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [dpdk-dev] [PATCH v2] usertools: fix pmdinfo parsing
  2020-11-04 10:32   ` Bruce Richardson
@ 2020-11-04 10:42     ` David Marchand
  0 siblings, 0 replies; 15+ messages in thread
From: David Marchand @ 2020-11-04 10:42 UTC (permalink / raw)
  To: Bruce Richardson, Robin Jarry, Olivier Matz, Thomas Monjalon
  Cc: dev, Stephen Hemminger, Neil Horman, Rosen Xu, Andrew Rybchenko,
	Luca Boccassi

On Wed, Nov 4, 2020 at 11:33 AM Bruce Richardson
<bruce.richardson@intel.com> wrote:
>
> On Wed, Nov 04, 2020 at 10:40:33AM +0100, David Marchand wrote:
> > This script inspects an ELF file (binary or shared library) and its
> > linked dependencies by following DT_NEEDED tags.
> > So far a simple librte_pmd prefix was used as a filter.
> > Now that we changed the driver library names, update this heuristic with
> > an explicit list of all driver classes.
> >
> > Fixes: a20b2c01a7a1 ("build: standardize component names and defines")
> >
> > Signed-off-by: David Marchand <david.marchand@redhat.com>
> > Acked-by: Robin Jarry <robin.jarry@6wind.com>
> > ---
> > Changelog since v1:
> > - moved driver classes list as a class variable and did some cosmetic
> >   change for readibility,
> > - used dpdk_driver_classes variable name in the hope that someone changing
> >   meson will catch this script too,
>
> Good idea, but probably not enough. I think adding a comment to the
> meson.build file to update this as well would be good.

I am reconsidering all this...
The filtering stage in the pmdinfo script is unneeded.
We need to find a PMD_INFO_STRING symbol in any case, which is good
enough to find out this is a dpdk driver without relying on the
library name.

Parsing all libraries only impact for the user is a debug message
print("Scanning %s for pmd information" % library).
We might as well remove it or hide under a verbose option.


-- 
David Marchand


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [dpdk-dev] [PATCH v3] usertools: fix pmdinfo parsing
  2020-11-03 18:39 [dpdk-dev] [PATCH] usertools: fix pmdinfo parsing David Marchand
  2020-11-03 19:27 ` Robin Jarry
  2020-11-04  9:40 ` [dpdk-dev] [PATCH v2] " David Marchand
@ 2020-11-04 15:57 ` David Marchand
  2020-11-04 16:35   ` Bruce Richardson
  2 siblings, 1 reply; 15+ messages in thread
From: David Marchand @ 2020-11-04 15:57 UTC (permalink / raw)
  To: dev
  Cc: bruce.richardson, robin.jarry, stephen, olivier.matz,
	Neil Horman, Rosen Xu, Andrew Rybchenko, Luca Boccassi

This script inspects an ELF file (binary or shared library) and its
linked dependencies by following DT_NEEDED tags.
So far a simple librte_pmd prefix was used as a filter to only parse
DPDK drivers dependencies.
While the reason is not clear from the commitlog of the patch that
introduced this filter, it was probably added for performance reasons,
since going through all dependencies can be quite long.
Testing with a DPDK built before the driver name changes:
- running the script takes ~0.3s with the filter,
- running the script takes ~9s without the filter,

Now that we changed the driver library names, it becomes more difficult
to identify only DPDK drivers, but we can just filter on the librte_
prefix to identify DPDK libraries: the script later checks for the
PMD_INFO_STRING string in .rodata and it is enough to differentiate the
DPDK drivers from the other DPDK libraries.
A debug message was logged for each inspected file, it gives no useful
information and is removed.

Fixes: a20b2c01a7a1 ("build: standardize component names and defines")

Signed-off-by: David Marchand <david.marchand@redhat.com>
Acked-by: Robin Jarry <robin.jarry@6wind.com>
---
Changelog since v2:
- revisited the issue and simplified to only filter on librte_ prefix,

Changelog since v1:
- moved driver classes list as a class variable and did some cosmetic
  change for readibility,
- used dpdk_driver_classes variable name in the hope that someone changing
  meson will catch this script too,
- added bus, common, mempool and raw driver classes as some of them do
  carry some pmdinfo stuff and were skipped so far but I found no
  indication this skipping was intended,

---
 usertools/dpdk-pmdinfo.py | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/usertools/dpdk-pmdinfo.py b/usertools/dpdk-pmdinfo.py
index 1661982791..95fb0111d8 100755
--- a/usertools/dpdk-pmdinfo.py
+++ b/usertools/dpdk-pmdinfo.py
@@ -434,7 +434,6 @@ def process_dt_needed_entries(self):
         """ Look to see if there are any DT_NEEDED entries in the binary
             And process those if there are
         """
-        global raw_output
         runpath = ""
         ldlibpath = os.environ.get('LD_LIBRARY_PATH')
         if ldlibpath is None:
@@ -450,13 +449,11 @@ def process_dt_needed_entries(self):
         for tag in dynsec.iter_tags():
             # pyelftools may return byte-strings, force decode them
             if force_unicode(tag.entry.d_tag) == 'DT_NEEDED':
-                if 'librte_pmd' in force_unicode(tag.needed):
+                if 'librte_' in force_unicode(tag.needed):
                     library = search_file(force_unicode(tag.needed),
                                           runpath + ":" + ldlibpath +
                                           ":/usr/lib64:/lib64:/usr/lib:/lib")
                     if library is not None:
-                        if raw_output is False:
-                            print("Scanning %s for pmd information" % library)
                         with io.open(library, 'rb') as file:
                             try:
                                 libelf = ReadElf(file, sys.stdout)
-- 
2.23.0


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [dpdk-dev] [PATCH v3] usertools: fix pmdinfo parsing
  2020-11-04 15:57 ` [dpdk-dev] [PATCH v3] " David Marchand
@ 2020-11-04 16:35   ` Bruce Richardson
  2020-11-04 16:48     ` David Marchand
  0 siblings, 1 reply; 15+ messages in thread
From: Bruce Richardson @ 2020-11-04 16:35 UTC (permalink / raw)
  To: David Marchand
  Cc: dev, robin.jarry, stephen, olivier.matz, Neil Horman, Rosen Xu,
	Andrew Rybchenko, Luca Boccassi

On Wed, Nov 04, 2020 at 04:57:21PM +0100, David Marchand wrote:
> This script inspects an ELF file (binary or shared library) and its
> linked dependencies by following DT_NEEDED tags.
> So far a simple librte_pmd prefix was used as a filter to only parse
> DPDK drivers dependencies.
> While the reason is not clear from the commitlog of the patch that
> introduced this filter, it was probably added for performance reasons,
> since going through all dependencies can be quite long.
> Testing with a DPDK built before the driver name changes:
> - running the script takes ~0.3s with the filter,
> - running the script takes ~9s without the filter,
> 
> Now that we changed the driver library names, it becomes more difficult
> to identify only DPDK drivers, but we can just filter on the librte_
> prefix to identify DPDK libraries: the script later checks for the
> PMD_INFO_STRING string in .rodata and it is enough to differentiate the
> DPDK drivers from the other DPDK libraries.
> A debug message was logged for each inspected file, it gives no useful
> information and is removed.
> 
> Fixes: a20b2c01a7a1 ("build: standardize component names and defines")
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> Acked-by: Robin Jarry <robin.jarry@6wind.com>
> ---
> Changelog since v2:
> - revisited the issue and simplified to only filter on librte_ prefix,
> 

Given you provided some original perf numbers of 0.3 vs 9 seconds above,
rather than leave us all in suspense :-), can you perhaps provide the
numbers for this version too.

/Bruce

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [dpdk-dev] [PATCH v3] usertools: fix pmdinfo parsing
  2020-11-04 16:35   ` Bruce Richardson
@ 2020-11-04 16:48     ` David Marchand
  2020-11-05 11:49       ` Bruce Richardson
  0 siblings, 1 reply; 15+ messages in thread
From: David Marchand @ 2020-11-04 16:48 UTC (permalink / raw)
  To: Bruce Richardson
  Cc: dev, Robin Jarry, Stephen Hemminger, Olivier Matz, Neil Horman,
	Rosen Xu, Andrew Rybchenko, Luca Boccassi

On Wed, Nov 4, 2020 at 5:36 PM Bruce Richardson
<bruce.richardson@intel.com> wrote:
>
> On Wed, Nov 04, 2020 at 04:57:21PM +0100, David Marchand wrote:
> > This script inspects an ELF file (binary or shared library) and its
> > linked dependencies by following DT_NEEDED tags.
> > So far a simple librte_pmd prefix was used as a filter to only parse
> > DPDK drivers dependencies.
> > While the reason is not clear from the commitlog of the patch that
> > introduced this filter, it was probably added for performance reasons,
> > since going through all dependencies can be quite long.
> > Testing with a DPDK built before the driver name changes:
> > - running the script takes ~0.3s with the filter,
> > - running the script takes ~9s without the filter,
> >
> > Now that we changed the driver library names, it becomes more difficult
> > to identify only DPDK drivers, but we can just filter on the librte_
> > prefix to identify DPDK libraries: the script later checks for the
> > PMD_INFO_STRING string in .rodata and it is enough to differentiate the
> > DPDK drivers from the other DPDK libraries.
> > A debug message was logged for each inspected file, it gives no useful
> > information and is removed.
> >
> > Fixes: a20b2c01a7a1 ("build: standardize component names and defines")
> >
> > Signed-off-by: David Marchand <david.marchand@redhat.com>
> > Acked-by: Robin Jarry <robin.jarry@6wind.com>
> > ---
> > Changelog since v2:
> > - revisited the issue and simplified to only filter on librte_ prefix,
> >
>
> Given you provided some original perf numbers of 0.3 vs 9 seconds above,
> rather than leave us all in suspense :-), can you perhaps provide the
> numbers for this version too.

Ah ah, yes, thought I had put it.
I have values in the 0.40/0.50s range.


-- 
David Marchand


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [dpdk-dev] [PATCH v2] usertools: fix pmdinfo parsing
  2020-11-04  9:40 ` [dpdk-dev] [PATCH v2] " David Marchand
  2020-11-04 10:32   ` Bruce Richardson
@ 2020-11-05 11:46   ` Xu, Rosen
  2020-11-12 13:28   ` David Marchand
  2 siblings, 0 replies; 15+ messages in thread
From: Xu, Rosen @ 2020-11-05 11:46 UTC (permalink / raw)
  To: David Marchand, dev
  Cc: Richardson, Bruce, robin.jarry, stephen, olivier.matz,
	Neil Horman, Andrew Rybchenko, Luca Boccassi

Hi,

> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: Wednesday, November 04, 2020 17:41
> To: dev@dpdk.org
> Cc: Richardson, Bruce <bruce.richardson@intel.com>;
> robin.jarry@6wind.com; stephen@networkplumber.org;
> olivier.matz@6wind.com; Neil Horman <nhorman@tuxdriver.com>; Xu,
> Rosen <rosen.xu@intel.com>; Andrew Rybchenko
> <andrew.rybchenko@oktetlabs.ru>; Luca Boccassi <bluca@debian.org>
> Subject: [PATCH v2] usertools: fix pmdinfo parsing
> 
> This script inspects an ELF file (binary or shared library) and its linked
> dependencies by following DT_NEEDED tags.
> So far a simple librte_pmd prefix was used as a filter.
> Now that we changed the driver library names, update this heuristic with an
> explicit list of all driver classes.
> 
> Fixes: a20b2c01a7a1 ("build: standardize component names and defines")
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> Acked-by: Robin Jarry <robin.jarry@6wind.com>
> ---
> Changelog since v1:
> - moved driver classes list as a class variable and did some cosmetic
>   change for readibility,
> - used dpdk_driver_classes variable name in the hope that someone
> changing
>   meson will catch this script too,
> - added bus, common, mempool and raw driver classes as some of them do
>   carry some pmdinfo stuff and were skipped so far but I found no
>   indication this skipping was intended,
> 
> ---
>  usertools/dpdk-pmdinfo.py | 20 +++++++++++++++++++-
>  1 file changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/usertools/dpdk-pmdinfo.py b/usertools/dpdk-pmdinfo.py index
> 1661982791..55a55affde 100755
> --- a/usertools/dpdk-pmdinfo.py
> +++ b/usertools/dpdk-pmdinfo.py
> @@ -430,6 +430,20 @@ def get_dt_runpath(self, dynsec):
>                  return force_unicode(tag.runpath)
>          return ""
> 
> +    dpdk_driver_classes = (
> +        'baseband',
> +        'bus',
> +        'common',
> +        'compress',
> +        'crypto',
> +        'event',
> +        'mempool',
> +        'net',
> +        'raw',
> +        'regex',
> +        'vdpa',
> +    )
> +
>      def process_dt_needed_entries(self):
>          """ Look to see if there are any DT_NEEDED entries in the binary
>              And process those if there are @@ -450,7 +464,11 @@ def
> process_dt_needed_entries(self):
>          for tag in dynsec.iter_tags():
>              # pyelftools may return byte-strings, force decode them
>              if force_unicode(tag.entry.d_tag) == 'DT_NEEDED':
> -                if 'librte_pmd' in force_unicode(tag.needed):
> +                words = force_unicode(tag.needed).split('_')
> +                if len(words) < 3:
> +                    continue
> +                prefix, drv_class = words[:2]
> +                if prefix == 'librte' and drv_class in self.dpdk_driver_classes:
>                      library = search_file(force_unicode(tag.needed),
>                                            runpath + ":" + ldlibpath +
>                                            ":/usr/lib64:/lib64:/usr/lib:/lib")
> --
> 2.23.0

Acked-by: Rosen Xu <rosen.xu@intel.com>

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [dpdk-dev] [PATCH v3] usertools: fix pmdinfo parsing
  2020-11-04 16:48     ` David Marchand
@ 2020-11-05 11:49       ` Bruce Richardson
  0 siblings, 0 replies; 15+ messages in thread
From: Bruce Richardson @ 2020-11-05 11:49 UTC (permalink / raw)
  To: David Marchand
  Cc: dev, Robin Jarry, Stephen Hemminger, Olivier Matz, Neil Horman,
	Rosen Xu, Andrew Rybchenko, Luca Boccassi

On Wed, Nov 04, 2020 at 05:48:27PM +0100, David Marchand wrote:
> On Wed, Nov 4, 2020 at 5:36 PM Bruce Richardson
> <bruce.richardson@intel.com> wrote:
> >
> > On Wed, Nov 04, 2020 at 04:57:21PM +0100, David Marchand wrote:
> > > This script inspects an ELF file (binary or shared library) and its
> > > linked dependencies by following DT_NEEDED tags.
> > > So far a simple librte_pmd prefix was used as a filter to only parse
> > > DPDK drivers dependencies.
> > > While the reason is not clear from the commitlog of the patch that
> > > introduced this filter, it was probably added for performance reasons,
> > > since going through all dependencies can be quite long.
> > > Testing with a DPDK built before the driver name changes:
> > > - running the script takes ~0.3s with the filter,
> > > - running the script takes ~9s without the filter,
> > >
> > > Now that we changed the driver library names, it becomes more difficult
> > > to identify only DPDK drivers, but we can just filter on the librte_
> > > prefix to identify DPDK libraries: the script later checks for the
> > > PMD_INFO_STRING string in .rodata and it is enough to differentiate the
> > > DPDK drivers from the other DPDK libraries.
> > > A debug message was logged for each inspected file, it gives no useful
> > > information and is removed.
> > >
> > > Fixes: a20b2c01a7a1 ("build: standardize component names and defines")
> > >
> > > Signed-off-by: David Marchand <david.marchand@redhat.com>
> > > Acked-by: Robin Jarry <robin.jarry@6wind.com>
> > > ---
> > > Changelog since v2:
> > > - revisited the issue and simplified to only filter on librte_ prefix,
> > >
> >
> > Given you provided some original perf numbers of 0.3 vs 9 seconds above,
> > rather than leave us all in suspense :-), can you perhaps provide the
> > numbers for this version too.
> 
> Ah ah, yes, thought I had put it.
> I have values in the 0.40/0.50s range.
> 
Sounds good.

Acked-by: Bruce Richardson <bruce.richardson@intel.com>

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [dpdk-dev] [PATCH v2] usertools: fix pmdinfo parsing
  2020-11-04  9:40 ` [dpdk-dev] [PATCH v2] " David Marchand
  2020-11-04 10:32   ` Bruce Richardson
  2020-11-05 11:46   ` Xu, Rosen
@ 2020-11-12 13:28   ` David Marchand
  2 siblings, 0 replies; 15+ messages in thread
From: David Marchand @ 2020-11-12 13:28 UTC (permalink / raw)
  To: dev
  Cc: Bruce Richardson, Robin Jarry, Stephen Hemminger, Olivier Matz,
	Neil Horman, Rosen Xu, Andrew Rybchenko, Luca Boccassi

On Wed, Nov 4, 2020 at 10:41 AM David Marchand
<david.marchand@redhat.com> wrote:
>
> This script inspects an ELF file (binary or shared library) and its
> linked dependencies by following DT_NEEDED tags.
> So far a simple librte_pmd prefix was used as a filter.
> Now that we changed the driver library names, update this heuristic with
> an explicit list of all driver classes.
>
> Fixes: a20b2c01a7a1 ("build: standardize component names and defines")
>
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> Acked-by: Robin Jarry <robin.jarry@6wind.com>
Acked-by: Bruce Richardson <bruce.richardson@intel.com>

Applied, thanks.


-- 
David Marchand


^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2020-11-12 13:29 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-03 18:39 [dpdk-dev] [PATCH] usertools: fix pmdinfo parsing David Marchand
2020-11-03 19:27 ` Robin Jarry
2020-11-03 20:20   ` David Marchand
2020-11-03 23:54     ` Stephen Hemminger
2020-11-04  8:04       ` Olivier Matz
2020-11-04  8:06     ` Robin Jarry
2020-11-04  9:40 ` [dpdk-dev] [PATCH v2] " David Marchand
2020-11-04 10:32   ` Bruce Richardson
2020-11-04 10:42     ` David Marchand
2020-11-05 11:46   ` Xu, Rosen
2020-11-12 13:28   ` David Marchand
2020-11-04 15:57 ` [dpdk-dev] [PATCH v3] " David Marchand
2020-11-04 16:35   ` Bruce Richardson
2020-11-04 16:48     ` David Marchand
2020-11-05 11:49       ` Bruce Richardson

DPDK patches and discussions

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://inbox.dpdk.org/dev/0 dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dev dev/ https://inbox.dpdk.org/dev \
		dev@dpdk.org
	public-inbox-index dev

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git