From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp.tuxdriver.com (charlotte.tuxdriver.com [70.61.120.58]) by dpdk.org (Postfix) with ESMTP id B1D735961 for ; Fri, 11 Apr 2014 17:48:30 +0200 (CEST) Received: from hmsreliant.think-freely.org ([2001:470:8:a08:7aac:c0ff:fec2:933b] helo=localhost) by smtp.tuxdriver.com with esmtpsa (TLSv1:AES128-SHA:128) (Exim 4.63) (envelope-from ) id 1WYdiL-0000mT-Lv; Fri, 11 Apr 2014 11:50:06 -0400 Date: Fri, 11 Apr 2014 11:50:01 -0400 From: Neil Horman To: Thomas Monjalon Message-ID: <20140411155000.GD911@hmsreliant.think-freely.org> References: <1460632.jOzC6OEr8u@xps13> <1397201813-26627-1-git-send-email-olivier.matz@6wind.com> <20140411104929.GA911@hmsreliant.think-freely.org> <8367341.P2CbmhJR7D@xps13> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <8367341.P2CbmhJR7D@xps13> User-Agent: Mutt/1.5.21 (2010-09-15) X-Spam-Score: -2.9 (--) X-Spam-Status: No Cc: dev@dpdk.org Subject: Re: [dpdk-dev] [PATCH v2 07/11 1/2] vdev: new registration API X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 11 Apr 2014 15:48:31 -0000 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 > > > > 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 >