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 5188A594F for ; Wed, 21 May 2014 17:23:35 +0200 (CEST) Received: from nat-pool-rdu-u.redhat.com ([66.187.233.203] helo=localhost) by smtp.tuxdriver.com with esmtpsa (TLSv1:AES128-SHA:128) (Exim 4.63) (envelope-from ) id 1Wn8Ml-00026M-P8; Wed, 21 May 2014 11:23:43 -0400 Date: Wed, 21 May 2014 11:23:33 -0400 From: Neil Horman To: "Richardson, Bruce" Message-ID: <20140521152333.GA5829@localhost.localdomain> References: <1400580057-30155-1-git-send-email-bruce.richardson@intel.com> <1400580057-30155-3-git-send-email-bruce.richardson@intel.com> <20140520181844.GF6648@hmsreliant.think-freely.org> <59AF69C657FD0841A61C55336867B5B01AA1C540@IRSMSX103.ger.corp.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <59AF69C657FD0841A61C55336867B5B01AA1C540@IRSMSX103.ger.corp.intel.com> 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 2/4] distributor: new packet distributor library 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: Wed, 21 May 2014 15:23:35 -0000 On Wed, May 21, 2014 at 10:21:26AM +0000, Richardson, Bruce wrote: > > -----Original Message----- > > From: Neil Horman [mailto:nhorman@tuxdriver.com] > > Sent: Tuesday, May 20, 2014 7:19 PM > > To: Richardson, Bruce > > Cc: dev@dpdk.org > > Subject: Re: [dpdk-dev] [PATCH 2/4] distributor: new packet distributor library > > > > On Tue, May 20, 2014 at 11:00:55AM +0100, Bruce Richardson wrote: > > > This adds the code for a new Intel DPDK library for packet distribution. > > > The distributor is a component which is designed to pass packets > > > one-at-a-time to workers, with dynamic load balancing. Using the RSS > > > field in the mbuf as a tag, the distributor tracks what packet tag is > > > being processed by what worker and then ensures that no two packets with > > > the same tag are in-flight simultaneously. Once a tag is not in-flight, > > > then the next packet with that tag will be sent to the next available > > > core. > > > > > > Signed-off-by: Bruce Richardson > > > > > > > > > > Don't need to reserve an extra argument here. You're not ABI safe currently, > > and if DPDK becomes ABI safe in the future, we will use a linker script to > > provide versions with backward compatibility easily enough. > We may not have ABI compatibility between releases, but on the other hand we try to reduce the amount of code changes that need to be made by our customers who are compiling their code against the libraries - generally linking against static rather than shared libraries. Since we have a reasonable expectation that this field will be needed in a future release, we want to include it now so that when we do need it, no code changes need to be made to upgrade this particular library to a new Intel DPDK version. I understand why you added the reserved argument, but I still don't think its a good idea, especially since you're not ABI safe/stable at the moment. By adding this argument, you're forcing early users to declare a variable to pass into your library that they know is unused, and as such likely uninitalized (or at least initilized to an unknown value). When you do in the future make use of this unknown value, your internal implementation will have to support being called by both 'old' applications that just pass in any old value, and 'new' users who pass in valid data, and the implementation wont have any way to differentiate between the two. You can certainly document a reserved value that current users must initilize that variable too, so that you can make that differentiation, but you have to hope they do that correctly and consistently. It seems to me it would be better to do something like: 1) Not include the reserved parameter 2) When you do add the extra parameter, rename the function as well, and 3) provide a compatibility function that preserves the old API and passes the reserved value as the new parameter to the renamed function in (2) That way old applications will run transparently, and you don't have to hope they code the reserved values properly (note you can also do this with a macro if you want to save the call instruction) Ideally, you would just do this with a version script during linking, so that you could include 2 versions of the same function name (v1 without the extra paramter and v2 with the extra parameter), and old applications linked against v1 would just continue to work, but dpdk isn't there yet :) Neil