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

[Pavan Nikhilesh Bhagavatula <pbhagavatula@marvell.com>]

>> From: dev <dev-bounces@dpdk.org> On Behalf Of bugzilla@dpdk.org
>> Sent: Wednesday, May 26, 2021 9:24 AM
>> To: dev@dpdk.org
>> Subject: [dpdk-dev] [Bug 721] Wrong event pointer in rx adapter
>>
>> https://urldefense.proofpoint.com/v2/url?u=https-
>3A__bugs.dpdk.org_show-5Fbug.cgi-3Fid-
>3D721&d=DwIGaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=E3SgYMjtKCMVsB
>-fmvgGV3o-
>g_fjLhk5Pupi9ijohpc&m=P7yStZJEWQK5Q4wQOaFcDFtRT_BqBTTBJSfc-
>d8-qUs&s=AqT8tJKXHif2z3dq6jjWuj00vlMwfzWGGv9LICSVwjY&e=
>>
>>             Bug ID: 721
>>            Summary: Wrong event pointer in rx adapter
>>            Product: DPDK
>>            Version: 20.11
>>           Hardware: All
>>                 OS: All
>>             Status: UNCONFIRMED
>>           Severity: major
>>           Priority: Normal
>>          Component: eventdev
>>           Assignee: dev@dpdk.org
>>           Reporter: heng.wang@ericsson.com
>>   Target Milestone: ---
>>
>> Problem:
>> In the function rxa_buffer_mbufs the ev is not reset to the array's
>first
>> element before passing to a user call back function.
>>
>> Workaround:
>> In the user callback, we must decrement ev by number of event
>before we can use
>> it.
>>
>> Fix:
>> I think we should pass &buf->events[buf->count] to dev_info->cb_fn
>instead of
>> ev.
>>
>>  773         for (i = 0; i < num; i++) {
>>  774                 m = mbufs[i];
>>  775
>>  776                 rss = do_rss ?
>>  777                         rxa_do_softrss(m, rx_adapter->rss_key_be) :
>>  778                         m->hash.rss;
>>  779                 ev->event = event;
>>  780                 ev->flow_id = (rss & ~flow_id_mask) |
>>  781                                 (ev->flow_id & flow_id_mask);
>>  782                 ev->mbuf = m;
>>  783                 ev++;
>>  784         }
>>  785
>>  786         if (dev_info->cb_fn) {
>>  787
>>  788                 dropped = 0;
>>  789                 nb_cb = dev_info->cb_fn(eth_dev_id, rx_queue_id,
>>  790                                         ETH_EVENT_BUFFER_SIZE, buf->count,
>> ev,
>>  791                                         num, dev_info->cb_arg, &dropped);
>>  792                 if (unlikely(nb_cb > num))
>>  793                         RTE_EDEV_LOG_ERR("Rx CB returned %d (> %d)
>> events",
>>  794                                 nb_cb, num);
>>  795                 else
>>  796                         num = nb_cb;
>>  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..

>
>Regards, -Harry

Cheers,
Pavan.