Re: [dpdk-dev] [Bug 721] Wrong event pointer in rx adapter

["Jayatheerthan, Jay" <jay.jayatheerthan@intel.com>]

> -----Original Message-----
> From: Van Haaren, Harry <harry.van.haaren@intel.com>
> Sent: Thursday, May 27, 2021 1:49 PM
> To: Pavan Nikhilesh Bhagavatula <pbhagavatula@marvell.com>; bugzilla@dpdk.org; dev@dpdk.org; Rao, Nikhil <nikhil.rao@intel.com>;
> Jayatheerthan, Jay <jay.jayatheerthan@intel.com>; heng.wang@ericsson.com
> Subject: RE: [dpdk-dev] [Bug 721] Wrong event pointer in rx adapter
> 
> > -----Original Message-----
> > From: Pavan Nikhilesh Bhagavatula <pbhagavatula@marvell.com>
> > Sent: Wednesday, May 26, 2021 2:02 PM
> > To: Van Haaren, Harry <harry.van.haaren@intel.com>; bugzilla@dpdk.org;
> > dev@dpdk.org; Rao, Nikhil <nikhil.rao@intel.com>; Jayatheerthan, Jay
> > <jay.jayatheerthan@intel.com>; heng.wang@ericsson.com
> > Subject: RE: [dpdk-dev] [Bug 721] Wrong event pointer in rx adapter
> >
> > >> From: dev <dev-bounces@dpdk.org> On Behalf Of bugzilla@dpdk.org
> <snip original bug report for brevity>
> > >>  797                 if (dropped)
> > >>  798                         rx_adapter->stats.rx_dropped += dropped;
> > >>  799         }
> > >
> > >+CC RX Adapter Maintainer, and Pavan as this code has been changed
> > >recently.
> > >
> > >I've done a quick review of the above report, and believe it to be a
> > >genuine bug,
> > >however this code has been refactored in commit d7c428e557b as part
> > >of series
> > >to " eventdev: support Rx adapter event vector".
> > >
> > >The relevant diff here for reference:
> > >Note "ev" pointer replaced with "&buf->events[buf->count]"
> > >
> > >                dropped = 0;
> > >                nb_cb = dev_info->cb_fn(eth_dev_id, rx_queue_id,
> > >-                                       ETH_EVENT_BUFFER_SIZE, buf->count, ev,
> > >-                                       num, dev_info->cb_arg, &dropped);
> > >+                                       ETH_EVENT_BUFFER_SIZE, buf->count,
> > >+                                       &buf->events[buf->count], num,
> > >+                                       dev_info->cb_arg, &dropped);
> > >                if (unlikely(nb_cb > num))
> > >
> > >
> > >Based on this investigation, although the code changed, the same bug
> > >seems present,
> > >as really &buf->events[0] should be passed to the callback?
> >
> > The callback semantics are pretty ambiguous. There are two cases:
> > 1. Is the callback allowed to modify the entire event buffer?
> > Or
> > 2. Is it only allowed to modify the events received in the current Rx cycle?
> >
> > The current code only takes into account case 2 as handling case 1 requires
> > additional modification of nb_cb etc..
> 
> Ah, I'm not actually familiar with the rx-adapter code at all, or the callback
> semantics/design... I presumed it was the same as Ethdev RX/TX callbacks.
> 
> Jay, would you mind taking a look and identifying what the desired behavior is?
> 
> It would be good to fix this ASAP as it seems like a genuine issue, and the workaround
> of "moving the pointer to where it should be in App code" is... a workaround :)

The design intent here is for the call back to get the current batch of packet and make drop decisions based on enqueue_buf_size (total enqueue buffer size), enqueue_buf_count (current buffer count) and nb_event (num events in current batch). If we pass full event buffer, are we trying to make another decision on packet that has already been admitted in earlier iteration. Besides, it may require change in the callback function implementation.

The latest code after Pavan's change fixes the bug reported here. Pavan, do you want to disposition this bug appropriately?

> 
> > Cheers,
> > Pavan.
> 
> Thanks for the input Pavan! Regards, -Harry

Thanks a bunch Harry and Pavan for your consideration!

-Jay