[dpdk-dev] [PATCH 1/3] eal/drivers: prefix driver REGISTER macros with EAL

[Neil Horman]

On Tue, Oct 11, 2016 at 12:06:27PM +0530, Shreyansh Jain wrote:
> On Monday 10 October 2016 06:26 PM, Neil Horman wrote:
> > On Sat, Oct 08, 2016 at 01:00:59PM +0000, Shreyansh Jain wrote:
> > > Hi Thomas,
> > > 
> > > > -----Original Message-----
> > > > From: Thomas Monjalon [mailto:thomas.monjalon at 6wind.com]
> > > > Sent: Friday, October 07, 2016 7:22 PM
> > > > To: Shreyansh Jain <shreyansh.jain at nxp.com>
> > > > Cc: david.marchand at 6wind.com; dev at dpdk.org
> > > > Subject: Re: [PATCH 1/3] eal/drivers: prefix driver REGISTER macros with EAL
> > > > 
> > > > 2016-10-07 19:11, Shreyansh Jain:
> > > > > --- a/mk/internal/rte.compile-pre.mk
> > > > > +++ b/mk/internal/rte.compile-pre.mk
> > > > > @@ -87,7 +87,7 @@ endif
> > > > >   PMDINFO_GEN = $(RTE_SDK_BIN)/app/dpdk-pmdinfogen $@ $@.pmd.c
> > > > >   PMDINFO_CC = $(CC) $(CPPFLAGS) $(CFLAGS) -c -o $@.pmd.o $@.pmd.c
> > > > >   PMDINFO_LD = $(CROSS)ld $(LDFLAGS) -r -o $@.o $@.pmd.o $@
> > > > > -PMDINFO_TO_O = if grep -q 'DRIVER_REGISTER_.*(.*)' $<; then \
> > > > > +PMDINFO_TO_O = if grep 'EAL_REGISTER_.*(.*)' $<; then \
> > > > >          echo "$(if $V,$(PMDINFO_GEN),  PMDINFO $@.pmd.c)" && \
> > > > >          $(PMDINFO_GEN) && \
> > > > >          echo "$(if $V,$(PMDINFO_CC),  CC $@.pmd.o)" && \
> > > > > 
> > > > > --->8---
> > > > >    CC eal_pci_vfio.o
> > > > >    PMDINFO eal_pci_vfio.o.pmd.c
> > > > > /bin/sh: 1:
> > > > > /home/shreyansh/build/DPDK/02_dpdk/x86_64-native-linuxapp-gcc/app/dpdk-
> > > > pmdinfogen:
> > > > > not found
> > > > > /home/shreyansh/build/DPDK/02_dpdk/mk/internal/rte.compile-pre.mk:138:
> > > > > recipe for target 'eal_pci_vfio.o' failed
> > > > > --->8---
> > > > > 
> > > > > I don't think PMDINFO should be running on eal_pci_vfio file. Isn't it?
> > > > 
> > > > Every files are scanned for the pattern.
> > > 
> > > Sorry, I should have been clearer in my statement.
> > > I meant, I didn't think eal_pci_vfio.o had anything of interest for the PMD tool and hence the mk files would have skipped over it in absence of a match.
> > > I understand that PMDINFO would run on all files.
> > > 
> > Thats incorrect, the Makefile does a REGEX search for appropriate registration
> > macros that imply the need for pmdinfo to run.  If its running on an
> > inappropriate file its because your new macros inadvertently match the current
> > regex, hence my suggestion that the regex should be tuned to be more specific
> 
> Agree. Thats is what I wanted to clarify as stated below: "...EAL_REGISTER_*
> (macro name has changed since) is matching EAL_REGISTER_TAILQ..".
> 
> As for 'more specific' match - I did suggest [2] a longer more specific
> version but Thomas had a different view point [1]. You can have a look at
> [2] and let me know your suggestion or if that is wrong.
> 
> [1] http://dpdk.org/ml/archives/dev/2016-October/048425.html
> [2] http://dpdk.org/ml/archives/dev/2016-October/048407.html
> 
Right, so I'm on the same page as Thomas, in that the "RTE_PMD_REGISTER_.*("
regex is sufficient in the current state.  However, your patches are changing
the registration macros from something that is sufficiently unique for pmdinfo
to detect to something that collides with an inappropriate match (the
EAL_REGISTER_TAILQ macro), and so you have to make the regex more specific.

This also begs the question in my mind, is it really worth changing the macro?
I really don't think it is.  The registration macros are pretty descriptive as
they stand, and have already changed 3 or 4 times in the last 6 months, which
suggests to me that any change here is really just churn more than meaningful
change.  You can make the argument that the name might be more in line with the
library its implemented in or what not, but in truth, its easy to understand
what the macros do (in their previous or current incantations), and any change
that just makes them the same as other macros in their naming is really more
trouble than its worth.

Neil


Neil

> > 
> > Neil
> > 
> > > > 
> > > > > Is it because EAL_REGISTER_* is matching EAL_REGISTER_TAILQ like macro
> > > > > as well?
> > > > 
> > > > Probably.
> > > > That's another argument in favor of good prefixes.
> > > > I think you should use RTE_DRIVER_REGISTER_ or better, RTE_PMD_REGISTER_
> > > 
> > > I thought of EAL_PMD_REGISTER_* but dropped it because for PCI_TABLE like macros, the name got really long (EAL_PMD_REGISTER_PCI_TABLE()).
> > > Anyways, I will use RTE_PMD_REGISTER_* now and send another version.
> > > 
> > > > > 
> > > > > I am not very well versed with how PMDINFO is linking with the build
> > > > > system so any hints/insight into this would be really helpful.
> > > > > 
> > > > > One I get more clarity on this, I will push a new version of this patch.
> > > > 
> > > > Thanks
> > > 
> > > -
> > > Shreyansh
> > > 
> > 
> 
> -
> Shreyansh
>