* [dpdk-dev] [Bug 721] Wrong event pointer in rx adapter
@ 2021-05-26 8:23 bugzilla
2021-05-26 9:54 ` Van Haaren, Harry
2021-06-21 9:20 ` bugzilla
0 siblings, 2 replies; 6+ messages in thread
From: bugzilla @ 2021-05-26 8:23 UTC (permalink / raw)
To: dev
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: 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 }
--
You are receiving this mail because:
You are the assignee for the bug.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [dpdk-dev] [Bug 721] Wrong event pointer in rx adapter 2021-05-26 8:23 [dpdk-dev] [Bug 721] Wrong event pointer in rx adapter bugzilla @ 2021-05-26 9:54 ` Van Haaren, Harry 2021-05-26 13:02 ` Pavan Nikhilesh Bhagavatula 2021-06-21 9:20 ` bugzilla 1 sibling, 1 reply; 6+ messages in thread From: Van Haaren, Harry @ 2021-05-26 9:54 UTC (permalink / raw) To: bugzilla, dev, Rao, Nikhil, Jayatheerthan, Jay, pbhagavatula, heng.wang > 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://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: 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? Regards, -Harry ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [dpdk-dev] [Bug 721] Wrong event pointer in rx adapter 2021-05-26 9:54 ` Van Haaren, Harry @ 2021-05-26 13:02 ` Pavan Nikhilesh Bhagavatula 2021-05-27 8:18 ` Van Haaren, Harry 0 siblings, 1 reply; 6+ messages in thread From: Pavan Nikhilesh Bhagavatula @ 2021-05-26 13:02 UTC (permalink / raw) To: Van Haaren, Harry, bugzilla, dev, Rao, Nikhil, Jayatheerthan, Jay, heng.wang >> 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. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [dpdk-dev] [Bug 721] Wrong event pointer in rx adapter 2021-05-26 13:02 ` Pavan Nikhilesh Bhagavatula @ 2021-05-27 8:18 ` Van Haaren, Harry 2021-06-01 6:23 ` Jayatheerthan, Jay 0 siblings, 1 reply; 6+ messages in thread From: Van Haaren, Harry @ 2021-05-27 8:18 UTC (permalink / raw) To: Pavan Nikhilesh Bhagavatula, bugzilla, dev, Rao, Nikhil, Jayatheerthan, Jay, heng.wang > -----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 :) > Cheers, > Pavan. Thanks for the input Pavan! Regards, -Harry ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [dpdk-dev] [Bug 721] Wrong event pointer in rx adapter 2021-05-27 8:18 ` Van Haaren, Harry @ 2021-06-01 6:23 ` Jayatheerthan, Jay 0 siblings, 0 replies; 6+ messages in thread From: Jayatheerthan, Jay @ 2021-06-01 6:23 UTC (permalink / raw) To: Van Haaren, Harry, Pavan Nikhilesh Bhagavatula, bugzilla, dev, Rao, Nikhil, heng.wang > -----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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [dpdk-dev] [Bug 721] Wrong event pointer in rx adapter 2021-05-26 8:23 [dpdk-dev] [Bug 721] Wrong event pointer in rx adapter bugzilla 2021-05-26 9:54 ` Van Haaren, Harry @ 2021-06-21 9:20 ` bugzilla 1 sibling, 0 replies; 6+ messages in thread From: bugzilla @ 2021-06-21 9:20 UTC (permalink / raw) To: dev https://bugs.dpdk.org/show_bug.cgi?id=721 Pavan Nikhilesh Bhagavatula (pbhagavatula@marvell.com) changed: What |Removed |Added ---------------------------------------------------------------------------- Status|UNCONFIRMED |RESOLVED Resolution|--- |INVALID CC| |pbhagavatula@marvell.com --- Comment #7 from Pavan Nikhilesh Bhagavatula (pbhagavatula@marvell.com) --- 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. -- You are receiving this mail because: You are the assignee for the bug. ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-06-21 9:20 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-05-26 8:23 [dpdk-dev] [Bug 721] Wrong event pointer in rx adapter bugzilla 2021-05-26 9:54 ` Van Haaren, Harry 2021-05-26 13:02 ` Pavan Nikhilesh Bhagavatula 2021-05-27 8:18 ` Van Haaren, Harry 2021-06-01 6:23 ` Jayatheerthan, Jay 2021-06-21 9:20 ` bugzilla
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).