Wrong event pointer in rx adapter
https://bugs.dpdk.org/show_bug.cgi?id=721
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: [email protected]
Reporter: [email protected]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 }
Signature · 2 lines
You are receiving this mail because: You are the assignee for the bug.
Quoted 54 lines
From: dev <[email protected]> On Behalf Of [email protected]
Sent: Wednesday, May 26, 2021 9:24 AM
To: [email protected]
Subject: [dpdk-dev] [Bug 721] Wrong event pointer in rx adapterhttps://bugs.dpdk.org/show_bug.cgi?id=721
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: [email protected] Reporter: [email protected]Target Milestone: ---
Problem:
In the functionrxa_buffer_mbufsthe 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] todev_info->cb_fninstead of ev.773 for (i = 0; i < num; i++) {
774 m = mbufs[i];
775
776 rss =do_rss?
777rxa_do_softrss(m, rx_adapter->rss_key_be):
778m->hash.rss;
779ev->event= event;
780ev->flow_id= (rss & ~flow_id_mask) |
781 (ev->flow_id&flow_id_mask);
782ev->mbuf= m;
783 ev++;
784 }
785
786 if (dev_info->cb_fn) {
787
788 dropped = 0;
789nb_cb= dev_info->cb_fn(eth_dev_id,rx_queue_id,
790ETH_EVENT_BUFFER_SIZE,buf->count,
ev,
791 num,dev_info->cb_arg, &dropped);
792 if (unlikely(nb_cb > num))
Quoted 8 lines
793 RTE_EDEV_LOG_ERR("Rx CB returned %d (> %d)
events",
794nb_cb, num);
795 else
796 num =nb_cb;
797 if (dropped)
798rx_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?
Regards, -Harry
Quoted 93 lines
Quoted 6 lines
From: dev <[email protected]> On Behalf Of [email protected]
Sent: Wednesday, May 26, 2021 9:24 AM
To: [email protected]
Subject: [dpdk-dev] [Bug 721] Wrong event pointer in rx adapter3A__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=
Quoted 13 lines
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: [email protected] Reporter: [email protected]Quoted 4 lines
Target Milestone: ---
Problem:
In the functionrxa_buffer_mbufsthe ev is not reset to the array'sfirst
Quoted 4 lines
element before passing to a user call back function.
Workaround:
In the user callback, we must decrement ev by number of eventbefore we can use
Quoted 4 lines
it.
Fix:
I think we should pass &buf->events[buf->count] todev_info->cb_fninstead of
Quoted 23 lines
ev.
773 for (i = 0; i < num; i++) {
774 m = mbufs[i];
775
776 rss =do_rss?
777rxa_do_softrss(m, rx_adapter->rss_key_be):
778m->hash.rss;
779ev->event= event;
780ev->flow_id= (rss & ~flow_id_mask) |
781 (ev->flow_id&flow_id_mask);
782ev->mbuf= m;
783 ev++;
784 }
785
786 if (dev_info->cb_fn) {
787
788 dropped = 0;
789nb_cb= dev_info->cb_fn(eth_dev_id,rx_queue_id,
790ETH_EVENT_BUFFER_SIZE,buf->count,
ev,
791 num,dev_info->cb_arg, &dropped);
792 if (unlikely(nb_cb > num))Quoted 8 lines
793 RTE_EDEV_LOG_ERR("Rx CB returned %d (> %d)
events",
794nb_cb, num);
795 else
796 num =nb_cb;
797 if (dropped)
798rx_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 commitd7c428e557bas 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,(quoted diff) +3
-2- 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))
Quoted 3 lines
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..
Quoted 2 lines
Regards, -Harry
Cheers,
Pavan.
Quoted 9 lines
-----Original Message-----
From: Pavan Nikhilesh Bhagavatula <[email protected]>
Sent: Wednesday, May 26, 2021 2:02 PM
To: Van Haaren, Harry <[email protected]>; [email protected]; [email protected]; Rao, Nikhil <[email protected]>; Jayatheerthan, Jay <[email protected]>; [email protected]
Subject: RE: [dpdk-dev] [Bug 721] Wrong event pointer in rx adapterQuoted 1 line
Quoted 1 line
From: dev <[email protected]> On Behalf Of [email protected]
<snip original bug report for brevity>
Quoted 30 lines
Quoted 26 lines
Quoted 3 lines
797 if (dropped)
798rx_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 commitd7c428e557bas 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,(quoted diff) +3
-2- 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))Quoted 3 lines
Based on this investigation, although the code changed, the same bug seems present,
as really &buf->events[0] should be passed to the callback?
Quoted 7 lines
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_cbetc..
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 :)
Quoted 2 lines
Cheers,
Pavan.
Thanks for the input Pavan! Regards, -Harry
Quoted 55 lines
-----Original Message-----
From: Van Haaren, Harry <[email protected]>
Sent: Thursday, May 27, 2021 1:49 PM
To: Pavan Nikhilesh Bhagavatula <[email protected]>; [email protected]; [email protected]; Rao, Nikhil <[email protected]>; Jayatheerthan, Jay <[email protected]>; [email protected]
Subject: RE: [dpdk-dev] [Bug 721] Wrong event pointer in rx adapterQuoted 9 lines
-----Original Message-----
From: Pavan Nikhilesh Bhagavatula <[email protected]>
Sent: Wednesday, May 26, 2021 2:02 PM
To: Van Haaren, Harry <[email protected]>; [email protected]; [email protected]; Rao, Nikhil <[email protected]>; Jayatheerthan, Jay <[email protected]>; [email protected]
Subject: RE: [dpdk-dev] [Bug 721] Wrong event pointer in rx adapterQuoted 1 line
Quoted 1 line
From: dev <[email protected]> On Behalf Of [email protected]
<snip original bug report for brevity>
Quoted 30 lines
Quoted 26 lines
Quoted 3 lines
797 if (dropped)
798rx_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 commitd7c428e557bas 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,(quoted diff) +3
-2- 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))Quoted 3 lines
Based on this investigation, although the code changed, the same bug seems present,
as really &buf->events[0] should be passed to the callback?Quoted 7 lines
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_cbetc..
Quoted 7 lines
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?
Quoted 5 lines
Quoted 3 lines
Cheers,
Pavan.Thanks for the input Pavan! Regards, -Harry
Thanks a bunch Harry and Pavan for your consideration!
-Jay
https://bugs.dpdk.org/show_bug.cgi?id=721
Pavan Nikhilesh Bhagavatula ([email protected]) changed:
What |Removed |Added
----------------------------------------------------------------------------
Status|UNCONFIRMED |RESOLVED
Resolution|--- |INVALID
CC| |[email protected]--- Comment #7 from Pavan Nikhilesh Bhagavatula ([email protected]) --- As Jay mentioned, the intent is to give the callback events generated in the current Rx cycle which the current code already does.
Marking the bug as resolved.
Pavan.
Signature · 2 lines
You are receiving this mail because: You are the assignee for the bug.