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 at 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?
