Re: [dpdk-dev] [PATCH] ethdev: additional parameter in RX callback

[Bruce Richardson <bruce.richardson@intel.com>]

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?