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 AEAC8A6A for ; Mon, 23 Mar 2015 17:01:14 +0100 (CET) Received: from cpe-098-026-070-093.nc.res.rr.com ([98.26.70.93] helo=localhost) by smtp.tuxdriver.com with esmtpsa (TLSv1:AES128-SHA:128) (Exim 4.63) (envelope-from ) id 1Ya4mm-00058K-3c; Mon, 23 Mar 2015 12:01:09 -0400 Date: Mon, 23 Mar 2015 12:00:58 -0400 From: Neil Horman To: Thomas Monjalon Message-ID: <20150323160058.GA5661@hmsreliant.think-freely.org> 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> User-Agent: Mutt/1.5.23 (2014-03-12) X-Spam-Score: -2.9 (--) X-Spam-Status: No 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 16:01:15 -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). > > 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. > I'm fine with this if thats the consensus, I'm more interested in making sure we think about these problems in such a way that we're not just running from ABI issues, because we're eventually going to have to deal with them Neil > Is there something I'm missing? > Thoughts? >