2014-04-11 11:50, Neil Horman: > There are still problems with the libraries after Oliviers patch series: > > [nhorman at 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 at 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
