Re: [dpdk-dev] [PATCH v2 07/11 1/2] vdev: new registration API

[Neil Horman <nhorman@tuxdriver.com>]

On Fri, Apr 11, 2014 at 06:18:08PM +0200, Thomas Monjalon wrote:
> 2014-04-11 11:50, Neil Horman:
> > There are still problems with the libraries after Oliviers patch series:
> > 
> > [nhorman@hmsreliant lib]$ ldd librte_pmd_pcap.so
> >         statically linked
> > 
> > The DSO's built by dpdk are not actually build as shared libraries but
> > rather statically linked together.  My patch series corrects that
> 
> Yes, that's why I said that "your patches have to be reviewed carefully 
> because there are other insteresting changes"
> 
Ok, But you also said that the previous patchset was reviewed carefully.
Apologies for any confusion.

> > Also, the build system fails to recognize the fact applications no longer
> > need to link against specific pmd libraries.  testpmd for example still
> > links to every single pmd libraryin dpdk, despite the fact that doesn't
> > need to happen (at least not with my patch series)
> > [nhorman@hmsreliant app]$ ldd testpmd
> > 	...
> >         librte_pmd_e1000.so
> >         librte_pmd_ixgbe.so
> >         librte_pmd_virtio_uio.so
> >         librte_pmd_vmxnet3_uio.so
> > 	...
> > 
> > Thats just a sample, but none of the pmds need to be explicitly referenced
> > at link time when using DSO's.  They can be specified at run time with the
> > -d option. They only need to be linked when you want to avoid having to use
> > the -d option at run time.  My patch series corrects that.
> 
> I agree.
> 
> > > It's a good approach because code is simpler like this and it's a first
> > > step before removing rte_pmd_init_all().
> > > Your patches are using rte_pmd_init_all() which is an useless function.
> > 
> > Thats not at all true.  rte_pmd_init_all is adventageous because it is
> > agnostic toward the pmd that is in use.
> 
> Not having rte_pmd_init_all() at all is a bit more agnostic :)
> 
Well, that would be great, I presume you are proposing tucking the driver
initalization into the eal library as part of the eal library init, so as to
protect it from the application?

> > If you use constructors in the
> > pmds (which is good), you don't need to reference the specific libraries in
> > your application code, but Oliviers approach will require that you do so. 
> > Needing to specify a specific pmd init function in your application
> > destroys the usefulness of the -d option in the eal option parsing code.
> 
> Could you point where we need to call a specific pmd init function?
> If it's the case, it must simply be fixed.
> 
Sorry, I misread here.  I was convoluted the previous approach in which some of
the sample apps called pmd initalization functions directly.  Looking at the
patches in a web browser, I thought that was still being done in the test app.

> > That is to say, if I have an application that wants to use the ring_pmd, it
> > needs to call the ring_pmd init function. Once that is coded into the
> > application, no other pmd can be loaded, because the application doesn't
> > call that pmds init function (i.e. -d is made useless).
> 
> I agree that ring_pmd init should be fixed.
> 
> > Conversely, if an
> > application uses rte_pmd_init_all (the way my patch series codes it), any
> > pmd that is either statically/dynamically loaded by the application, or
> > opened via a dlopen call, will be registered and initalized, allowing any
> > application to control which pmd is used during the build process or at run
> > time, without having to hard code any initalization.
> 
> It seems that your patch is not removing 
> rte_eth_ring_pair_create/rte_eth_ring_pair_attach so I'm not sure you can 
> dynamically change the PMD in this case.
> 
Ew, I had missed those calls.  Yes, those should be encapsulated as some driver
ops or some such.  I'll look at that when I rebase.  Regardless however, I
didn't mean to state that pmds could be switched while running, only that the
pmd to use could be specified at run time.  Though, you're correct, pmd_ring
doesn't seem to hold in line with the other pmds in their isolation.

> > > Neil, next time, don't hesitate to discuss the design approaches before
> > > doing such big changes. By the way, your patches have to be reviewed
> > > carefully because there are other insteresting changes.
> > 
> > I didn't hesitate, Im happy to reach out and discuss large changes, I just
> > thought I would do so with code in hand (this change set only took a few
> > days to write).  I think my major failing was to assume that the git tree
> > was reasonably up to date with the mailing list.  I joined the mailing list
> > about a month ago (2 weeks after this patch series went up).  About a week
> > ago I started tinkering with this, under the impression that what was in
> > the git tree (having seem my assembly patch go in) was reasonably current
> > with the mailing list. Having a patch sit on a mailing list without comment
> > for a month is not condusive to doing upstream development.  Checking the
> > mailing list for prior work isn't helpful either, as mailman doesn't allow
> > for the easy extraction of patches (they're all served as html), and
> > patches that sit for that long begin to look suspect (are they going in or
> > not?).
> 
> You're right. It shouldn't stay so long without being integrated.
> Using patchwork should help us here.
> 
Patchwork would be good, a git branch would really be better. Anything other
than git means developers have a step 0 to undertake when starting development
in whcih they have to fetch uncomitted patches from patchwork to make sure their
code it up to date, and that introduces the possibility of errors during
appilcation (due to different developers applying patches in different oders,
etc).  Using a git branch gives you a canonical location with the latest code
that every developer sees in the same way, which is very helpful to concurrent
development.

> > If I can make a suggestion: If you're planning on delaying patches like this
> > for that length of time, create a devel branch for unstable changes, which
> > accepts changes from the mailing list in a timely fashion.  Allow public
> > testing to weed out the problems, not off-list review.  Set stabilization
> > periods for said devel-branch and merge them to the latest release branch
> > during those times. Also, please don't push larger series piecemeal out to
> > the git tree.  Stage them all on your copy of the repo and push them at
> > once.  As it is, my patch series is rebased to a point that is halfway
> > through Oliviers patch series.  I know John Linville is on this list that
> > maintains the wireless tree, and I'm sure he can offer lots of good git
> > workflow pointers.
> 
> I understand your comment. This workflow worked well in the first year because 
> there was few contributors. Now we need another organization. And yes I'm 
> thinking about a development branch.
> 
> > As for this patch series, I'll rebase on top of Oliviers, fixing the things
> > that are still broken and resubmit.
> 
> OK, thank you.
> 
Thank you!  I'll repost sometime next week

Best
Neil

> -- 
> Thomas
>