From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by dpdk.org (Postfix) with ESMTP id 95211B62 for ; Mon, 23 Mar 2015 16:29:37 +0100 (CET) Received: from orsmga002.jf.intel.com ([10.7.209.21]) by fmsmga102.fm.intel.com with ESMTP; 23 Mar 2015 08:29:35 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.11,452,1422950400"; d="scan'208";a="702852203" Received: from bricha3-mobl3.ger.corp.intel.com ([10.243.20.28]) by orsmga002.jf.intel.com with SMTP; 23 Mar 2015 08:29:34 -0700 Received: by (sSMTP sendmail emulation); Mon, 23 Mar 2015 15:29:32 +0025 Date: Mon, 23 Mar 2015 15:29:32 +0000 From: Bruce Richardson To: Thomas Monjalon Message-ID: <20150323152932.GC12720@bricha3-MOBL3> References: <1426179268-22164-1-git-send-email-john.mcnamara@intel.com> <20150313231526.GA14641@neilslaptop.think-freely.org> <2572007.3HCLxGSxJD@xps13> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <2572007.3HCLxGSxJD@xps13> Organization: Intel Shannon Ltd. User-Agent: Mutt/1.5.23 (2014-03-12) Cc: dev@dpdk.org Subject: Re: [dpdk-dev] [PATCH] ethdev: additional parameter in RX callback 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: Mon, 23 Mar 2015 15:29:38 -0000 On Mon, Mar 23, 2015 at 04:16:36PM +0100, Thomas Monjalon wrote: > 2015-03-13 19:15, Neil Horman: > > On Fri, Mar 13, 2015 at 06:28:31PM +0000, Mcnamara, John wrote: > > > From: Neil Horman [mailto:nhorman@tuxdriver.com] > > > > > > > > > Is encoding the information in the array really a better solution here? > > > > The cb->param already exists for passing in user defined information to > > > > the callback. The proposed patch merely transmits the parent function > > > > arguments to the enclosed callback. > > > > > > > > > The cb->param can't be used here, because its opaque to the internals of > > > > the DPDK. rte_eth_rx_burst doesn't (and can't) know where in the cb- > > > > >params pointer to store that information. Thats why you added an > > > > additional parameter in the first place, isn't it? > > > > > > Yes. That is correct. > > > > > Then why did you suggest doing so? > > > > > > My point is that using > > > > an array terminator keeps us out of this habbit of just adding parameters > > > > to communicate more information (as thats an ABI breaking method, and not > > > > particularly scalable if there is more information to be transmitted in > > > > the future). Using a context sensitive API set goes beyond even that, and > > > > allows to retrieve arbitrary information form callbacks as needed in an > > > > ABI safe manner > > > > > > Again I can agree with this in the general case, but it isn't necessary, > > > in this case, to encode the information in the array since it is already > > > local to and available in the function. It seems artificial, at this point, > > > to implement an array terminator solution to protect an API that, > > > effectively, hasn't been published yet. > > > > > You indicate that you agree an alternate solution is preferable in the general > > case, so as to provide an API that is extensible in a way that isn't subject to > > ABI breakage, correct? If so, why do assert that its not necessecary in this > > specific case? If you feel you need to add information so that callbacks can be > > more flexible (in this case specifying the size of a passed in array), why > > immediately shoehorn another parmeter in place, and break the consistency > > between rx and tx callbacks, when you don't have to? I don't care if you break > > ABI today (although to call it unpublished I think is disingenuous, as lots of > > testing and development has already taken place with the ABI as it currently > > stands). I care, as I noted above about not getting into the habbit of just > > assuming a change like this requires that you invaliate ABI somehow. You don't > > have to, you can create an API that is fairly invariant to it here if you like. > > The question in my mind is, why don't you? > > I think John is saying that the API of rte_eth_rx_burst() already includes > the nb_pkts parameter. So it's natural to push it to the callback. > I also think Neil is saying that this parameter is useless in the callback > and in rte_eth_rx_burst() if the array was null terminated. > In any case, having a mix (null termination + parameter in rte_eth_rx_burst) > is not acceptable. > Moreover, I wonder how efficient are the compiler optimizations in each loop > case (index and null termination). Compiler can't optimize/unroll the loop in the null termination case. For the passing-the-size through option, in any app where the RX buffer size is constant, i.e. probably a lot of them - like in our examples, the compiler can do loop unrolling, and possibly other optimizations on the known value. [Whether it choses too or not, is not something we have tested :-)] /Bruce > > As the API was using an integer count, my opinion is to keep it and push it to > the callback for 2.0. > If null termination is validated to be better, it could be a later rework. > > Is there something I'm missing? > Thoughts?