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

[Neil Horman <nhorman@tuxdriver.com>]

On Fri, Mar 13, 2015 at 04:26:52PM +0000, Mcnamara, John wrote:
> 
> 
> > -----Original Message-----
> > From: Neil Horman [mailto:nhorman@tuxdriver.com]
> > Sent: Friday, March 13, 2015 3:09 PM
> > To: Richardson, Bruce
> > Cc: Mcnamara, John; dev@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH] ethdev: additional parameter in RX
> > callback
> > 
> > Plese set asside the ABI issue for a moment.  I get that you're trying to
> > get it in prior to needing to version it.  Thats not the argument.  The
> > argument is how best to codify the new information you want to express in
> > the callback.  For this specific case, I think there are better ways to do
> > this than to just blindly add a new parameter.
> 
> Hi Neil,
> 
> I think that is good advice is the general case but this is a very specific case. The modified callback is only used in rte_eth_rx_burst(). For context here is the function in its entirety (without #defs). The substantive change (the addition of nb_pkts) is on the line with an asterisk:
> 
> 
>     static inline uint16_t
>     rte_eth_rx_burst(uint8_t port_id, uint16_t queue_id,
>              struct rte_mbuf **rx_pkts, const uint16_t nb_pkts)
>     {
>         struct rte_eth_dev *dev;
> 
>         dev = &rte_eth_devices[port_id];
> 
>         int16_t nb_rx = (*dev->rx_pkt_burst)(dev->data->rx_queues[queue_id],
>                 rx_pkts, nb_pkts);
> 
>         struct rte_eth_rxtx_callback *cb = dev->post_rx_burst_cbs[queue_id];
> 
>         if (unlikely(cb != NULL)) {
>             do {
>                 nb_rx = cb->fn.rx(port_id, queue_id, rx_pkts, nb_rx,
>     *						nb_pkts, cb->param);
>                 cb = cb->next;
>             } while (cb != NULL);
>         }
> 
>         return nb_rx;
>     }
> 

Not sure I grok your point here.  Why impact does the number of internal
callouts for the callback api have on how we structure the API to external
consumers?


> > Encoding the array size
> > implicitly with a terminating marker lets you use this equally well with
> > the tx and rx callbacks (should you ever need it on the latter)
> 
> 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?  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

Neil

> John
> 
> 
> 
> 
> 
> 
>