From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wg0-f49.google.com (mail-wg0-f49.google.com [74.125.82.49]) by dpdk.org (Postfix) with ESMTP id 13362307 for ; Fri, 11 Apr 2014 18:16:32 +0200 (CEST) Received: by mail-wg0-f49.google.com with SMTP id a1so5590604wgh.8 for ; Fri, 11 Apr 2014 09:18:10 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:from:to:cc:subject:date:message-id:organization :user-agent:in-reply-to:references:mime-version :content-transfer-encoding:content-type; bh=DPnq6oWw2wKJ1MHItDvJSr+d9DOlCB2P/Zptpw59mqQ=; b=P1yFo85Q66Gi/6w5OdIaZ4LqX3mXt3CEcMyGN10BE5PvIlM5o6RL7g/t+5rF3mdI0Y UUaJl9DCgkj1xNnhe8ma9QdM5LR/iDm/nEceGsxJsB5H83HSlTRXet7dVMhY6ZitNiVf 0j8sY5Hr6CDIJJ1lKXV+PfspnCdgeYfC3nA7QCdSTVHjjKmVSOkVz780lkf4IF9NiFaQ +7Ivnjjf8/Fp36l09JxULmc34sUZARM9RcH3yX01ocm0MY3y9Isu6qNI68jmHS/UQJbT RQmmr7IulLRsXkMs9vSdunDkZ3GqiChGSSduoULzT6eropSAutluDUTmBRcPkqdKdwY7 C2iQ== X-Gm-Message-State: ALoCoQlHSU0nB+gSn3t8Tcsdy4E07oIu8ehiCBK+1gIUp1pRIrZrduRt0EuGcjRTgY9HRpTEAGfP X-Received: by 10.194.238.231 with SMTP id vn7mr2791984wjc.76.1397233090121; Fri, 11 Apr 2014 09:18:10 -0700 (PDT) Received: from xps13.localnet (6wind.net2.nerim.net. [213.41.180.237]) by mx.google.com with ESMTPSA id vh3sm11905471wjc.18.2014.04.11.09.18.08 for (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 11 Apr 2014 09:18:09 -0700 (PDT) From: Thomas Monjalon To: Neil Horman Date: Fri, 11 Apr 2014 18:18:08 +0200 Message-ID: <4074308.XK7oTvuRSP@xps13> Organization: 6WIND User-Agent: KMail/4.12.3 (Linux/3.13.7-1-ARCH; KDE/4.12.3; x86_64; ; ) In-Reply-To: <20140411155000.GD911@hmsreliant.think-freely.org> References: <1460632.jOzC6OEr8u@xps13> <8367341.P2CbmhJR7D@xps13> <20140411155000.GD911@hmsreliant.think-freely.org> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" 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 16:16:33 -0000 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