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

[Neil Horman <nhorman@tuxdriver.com>]

On Fri, Apr 11, 2014 at 03:11:56PM +0200, Thomas Monjalon wrote:
> Hi Neil,
> 
> 2014-04-11 06:49, Neil Horman:
> > On Fri, Apr 11, 2014 at 09:36:52AM +0200, Olivier Matz wrote:
> > > Instead of having a list of virtual device drivers in EAL code, add an
> > > API to register drivers. Thanks to this new registration method, we can
> > > remove the references to pmd_ring, pmd_pcap and pmd_xenvirt in EAL code.
> > > This also enables the ability to register a virtual device driver as
> > > a shared library.
> > > 
> > > The registration is done in an init function flaged with
> > > __attribute__((constructor)). The new convention is to name this
> > > function rte_pmd_xyz_init(). The per-device init function is renamed
> > > rte_pmd_xyz_devinit().
> > > 
> > > By the way the internal PMDs are now also .so/standalone ready. Let's do
> > > it later on. It will be required to ease maintenance.
> > > 
> > > Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
> > 
> > I just posted a patch to separate pmd linkage from the applications
> > yesterday, which will work with the existing API.  See my series on the
> > introduction of the PMD_INIT and PMD_INIT_NONPCI macros.
> 
> This patch serie was posted weeks ago and has been reviewed.

When and where?  You are correct, this series was posted about 1.5 months ago:
http://dpdk.org/ml/archives/dev/2014-February/001466.html

To which there was a single comment, and no other traffic until last night, when
a few bits of the series were posted to git, followed by the remaining bits
several hours later.  I don't see any acks or indications that, after 1.5
months, this was still an active patch series.

Note also that whatever review was done, was not particularly through.  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

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.

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

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

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.

As for this patch series, I'll rebase on top of Oliviers, fixing the things that
are still broken and resubmit.

Regards
Neil

> Thanks
> -- 
> Thomas
>