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

[Thomas Monjalon <thomas.monjalon-pdR9zngts4EAvxtiuMwx3w@public.gmane.org>]

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"

> 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 :)

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

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

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

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

-- 
Thomas