[v4] mbuf: fix reset on mbuf free
Checks
Commit Message
m->nb_seg must be reset on mbuf free whatever the value of m->next,
because it can happen that m->nb_seg is != 1. For instance in this
case:
m1 = rte_pktmbuf_alloc(mp);
rte_pktmbuf_append(m1, 500);
m2 = rte_pktmbuf_alloc(mp);
rte_pktmbuf_append(m2, 500);
rte_pktmbuf_chain(m1, m2);
m0 = rte_pktmbuf_alloc(mp);
rte_pktmbuf_append(m0, 500);
rte_pktmbuf_chain(m0, m1);
As rte_pktmbuf_chain() does not reset nb_seg in the initial m1
segment (this is not required), after this code the mbuf chain
have 3 segments:
- m0: next=m1, nb_seg=3
- m1: next=m2, nb_seg=2
- m2: next=NULL, nb_seg=1
Then split this chain between m1 and m2, it would result in 2 packets:
- first packet
- m0: next=m1, nb_seg=2
- m1: next=NULL, nb_seg=2
- second packet
- m2: next=NULL, nb_seg=1
Freeing the first packet will not restore nb_seg=1 in the second
segment. This is an issue because it is expected that mbufs stored
in pool have their nb_seg field set to 1.
Fixes: 8f094a9ac5d7 ("mbuf: set mbuf fields while in pool")
Cc: stable@dpdk.org
Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
Acked-by: Morten Brørup <mb@smartsharesystems.com>
Acked-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
---
v4
* add a unit test (suggested by David)
v3
* fix commit log again (thanks Morten for spotting it)
v2
* avoid write access if uneeded (suggested by Konstantin)
* enhance comments in mbuf header file (suggested by Morten)
* fix commit log
app/test/test_mbuf.c | 69 +++++++++++++++++++++++++++++++++
lib/librte_mbuf/rte_mbuf.c | 4 +-
lib/librte_mbuf/rte_mbuf.h | 8 ++--
lib/librte_mbuf/rte_mbuf_core.h | 13 ++++++-
4 files changed, 86 insertions(+), 8 deletions(-)
Comments
On Wed, Jan 13, 2021 at 2:28 PM Olivier Matz <olivier.matz@6wind.com> wrote:
>
> m->nb_seg must be reset on mbuf free whatever the value of m->next,
> because it can happen that m->nb_seg is != 1. For instance in this
> case:
>
> m1 = rte_pktmbuf_alloc(mp);
> rte_pktmbuf_append(m1, 500);
> m2 = rte_pktmbuf_alloc(mp);
> rte_pktmbuf_append(m2, 500);
> rte_pktmbuf_chain(m1, m2);
> m0 = rte_pktmbuf_alloc(mp);
> rte_pktmbuf_append(m0, 500);
> rte_pktmbuf_chain(m0, m1);
>
> As rte_pktmbuf_chain() does not reset nb_seg in the initial m1
> segment (this is not required), after this code the mbuf chain
> have 3 segments:
> - m0: next=m1, nb_seg=3
> - m1: next=m2, nb_seg=2
> - m2: next=NULL, nb_seg=1
>
> Then split this chain between m1 and m2, it would result in 2 packets:
> - first packet
> - m0: next=m1, nb_seg=2
> - m1: next=NULL, nb_seg=2
> - second packet
> - m2: next=NULL, nb_seg=1
>
> Freeing the first packet will not restore nb_seg=1 in the second
> segment. This is an issue because it is expected that mbufs stored
> in pool have their nb_seg field set to 1.
>
> Fixes: 8f094a9ac5d7 ("mbuf: set mbuf fields while in pool")
> Cc: stable@dpdk.org
>
> Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
> Acked-by: Morten Brørup <mb@smartsharesystems.com>
> Acked-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
> Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> ---
>
> v4
> * add a unit test (suggested by David)
Olivier,
Thanks, for adding it.
Ali,
You reported some performance regression, did you confirm it?
If I get no reply by monday, I'll proceed with this patch.
Thanks.
Hi,
Adding Ferruh and Zhaoyan,
> Ali,
>
> You reported some performance regression, did you confirm it?
> If I get no reply by monday, I'll proceed with this patch.
Sure I'll confirm by Monday.
Doesn't the regression also reproduce on the Lab's Intel servers?
Even though the check iol-intel-Performance isn't failing, I can see that the throughput differences from expected for this patch are less than those of another patch that was tested only 20 minutes earlier. Both patches were applied to the same tree:
https://mails.dpdk.org/archives/test-report/2021-January/173927.html
> | 64 | 512 | 1.571 |
https://mails.dpdk.org/archives/test-report/2021-January/173919.html
> | 64 | 512 | 2.698 |
Assuming that pw86457 doesn't have an effect on this test, it looks to me that this patch caused a regression in Intel hardware as well.
Can someone update the baseline's expected values for the Intel NICs and rerun the test on this patch?
Thanks,
Ali
Hi,
(Sorry had to resend this to some recipients due to mail server problems).
Just confirming that I can still reproduce the regression with single core and 64B frames on other servers.
- Ali
> -----Original Message-----
> From: Ali Alnubani <alialnu@nvidia.com>
> Sent: Friday, January 15, 2021 8:39 PM
> To: David Marchand <david.marchand@redhat.com>; Olivier Matz
> <olivier.matz@6wind.com>; Ferruh Yigit <ferruh.yigit@intel.com>;
> zhaoyan.chen@intel.com
> Cc: dev <dev@dpdk.org>; Andrew Rybchenko
> <andrew.rybchenko@oktetlabs.ru>; Ananyev, Konstantin
> <konstantin.ananyev@intel.com>; Morten Brørup
> <mb@smartsharesystems.com>; ajitkhaparde@gmail.com; dpdk stable
> <stable@dpdk.org>; Ajit Khaparde <ajit.khaparde@broadcom.com>
> Subject: Re: [dpdk-stable] [PATCH v4] mbuf: fix reset on mbuf free
>
> Hi,
> Adding Ferruh and Zhaoyan,
>
> > Ali,
> >
> > You reported some performance regression, did you confirm it?
> > If I get no reply by monday, I'll proceed with this patch.
>
> Sure I'll confirm by Monday.
>
> Doesn't the regression also reproduce on the Lab's Intel servers?
> Even though the check iol-intel-Performance isn't failing, I can see that the
> throughput differences from expected for this patch are less than those of
> another patch that was tested only 20 minutes earlier. Both patches were
> applied to the same tree:
>
> https://mails.dpdk.org/archives/test-report/2021-January/173927.html
> > | 64 | 512 | 1.571 |
>
> https://mails.dpdk.org/archives/test-report/2021-January/173919.html
> > | 64 | 512 | 2.698 |
>
> Assuming that pw86457 doesn't have an effect on this test, it looks to me
> that this patch caused a regression in Intel hardware as well.
>
> Can someone update the baseline's expected values for the Intel NICs and
> rerun the test on this patch?
>
> Thanks,
> Ali
Hi Ali,
On Mon, Jan 18, 2021 at 05:52:32PM +0000, Ali Alnubani wrote:
> Hi,
> (Sorry had to resend this to some recipients due to mail server problems).
>
> Just confirming that I can still reproduce the regression with single core and 64B frames on other servers.
Many thanks for the feedback. Can you please detail what is the amount
of performance loss in percent, and confirm the test case? (I suppose it
is testpmd io forward).
Unfortunatly, I won't be able to spend a lot of time on this soon (sorry
for that). So I see at least these 2 options:
- postpone the patch again, until I can find more time to analyze
and optimize
- apply the patch if the performance loss is acceptable compared to
the added value of fixing a bug
Regards,
Olivier
>
> - Ali
>
> > -----Original Message-----
> > From: Ali Alnubani <alialnu@nvidia.com>
> > Sent: Friday, January 15, 2021 8:39 PM
> > To: David Marchand <david.marchand@redhat.com>; Olivier Matz
> > <olivier.matz@6wind.com>; Ferruh Yigit <ferruh.yigit@intel.com>;
> > zhaoyan.chen@intel.com
> > Cc: dev <dev@dpdk.org>; Andrew Rybchenko
> > <andrew.rybchenko@oktetlabs.ru>; Ananyev, Konstantin
> > <konstantin.ananyev@intel.com>; Morten Brørup
> > <mb@smartsharesystems.com>; ajitkhaparde@gmail.com; dpdk stable
> > <stable@dpdk.org>; Ajit Khaparde <ajit.khaparde@broadcom.com>
> > Subject: Re: [dpdk-stable] [PATCH v4] mbuf: fix reset on mbuf free
> >
> > Hi,
> > Adding Ferruh and Zhaoyan,
> >
> > > Ali,
> > >
> > > You reported some performance regression, did you confirm it?
> > > If I get no reply by monday, I'll proceed with this patch.
> >
> > Sure I'll confirm by Monday.
> >
> > Doesn't the regression also reproduce on the Lab's Intel servers?
> > Even though the check iol-intel-Performance isn't failing, I can see that the
> > throughput differences from expected for this patch are less than those of
> > another patch that was tested only 20 minutes earlier. Both patches were
> > applied to the same tree:
> >
> > https://mails.dpdk.org/archives/test-report/2021-January/173927.html
> > > | 64 | 512 | 1.571 |
> >
> > https://mails.dpdk.org/archives/test-report/2021-January/173919.html
> > > | 64 | 512 | 2.698 |
> >
> > Assuming that pw86457 doesn't have an effect on this test, it looks to me
> > that this patch caused a regression in Intel hardware as well.
> >
> > Can someone update the baseline's expected values for the Intel NICs and
> > rerun the test on this patch?
> >
> > Thanks,
> > Ali
Could someone at Intel please update the test script to provide output according to the test plan? Or delegate to the right person.
According to the test plan, the information requested by Olivier should be in the test output already:
http://git.dpdk.org/tools/dts/tree/test_plans/nic_single_core_perf_test_plan.rst?h=next
PS: I can't find out who is the maintainer of the test plan, so I'm randomly pointing my finger at the test plan doc copyright holder. :-)
Med venlig hilsen / kind regards
- Morten Brørup
> -----Original Message-----
> From: Olivier Matz [mailto:olivier.matz@6wind.com]
> Sent: Tuesday, January 19, 2021 9:32 AM
> To: Ali Alnubani
> Cc: David Marchand; Ferruh Yigit; zhaoyan.chen@intel.com; dev; Andrew
> Rybchenko; Ananyev, Konstantin; Morten Brørup; ajitkhaparde@gmail.com;
> dpdk stable; Ajit Khaparde; Slava Ovsiienko; Alexander Kozyrev
> Subject: Re: [dpdk-stable] [PATCH v4] mbuf: fix reset on mbuf free
>
> Hi Ali,
>
>
> On Mon, Jan 18, 2021 at 05:52:32PM +0000, Ali Alnubani wrote:
> > Hi,
> > (Sorry had to resend this to some recipients due to mail server
> problems).
> >
> > Just confirming that I can still reproduce the regression with single
> core and 64B frames on other servers.
>
> Many thanks for the feedback. Can you please detail what is the amount
> of performance loss in percent, and confirm the test case? (I suppose
> it
> is testpmd io forward).
>
> Unfortunatly, I won't be able to spend a lot of time on this soon
> (sorry
> for that). So I see at least these 2 options:
>
> - postpone the patch again, until I can find more time to analyze
> and optimize
> - apply the patch if the performance loss is acceptable compared to
> the added value of fixing a bug
>
> Regards,
> Olivier
>
>
> >
> > - Ali
> >
> > > -----Original Message-----
> > > From: Ali Alnubani <alialnu@nvidia.com>
> > > Sent: Friday, January 15, 2021 8:39 PM
> > > To: David Marchand <david.marchand@redhat.com>; Olivier Matz
> > > <olivier.matz@6wind.com>; Ferruh Yigit <ferruh.yigit@intel.com>;
> > > zhaoyan.chen@intel.com
> > > Cc: dev <dev@dpdk.org>; Andrew Rybchenko
> > > <andrew.rybchenko@oktetlabs.ru>; Ananyev, Konstantin
> > > <konstantin.ananyev@intel.com>; Morten Brørup
> > > <mb@smartsharesystems.com>; ajitkhaparde@gmail.com; dpdk stable
> > > <stable@dpdk.org>; Ajit Khaparde <ajit.khaparde@broadcom.com>
> > > Subject: Re: [dpdk-stable] [PATCH v4] mbuf: fix reset on mbuf free
> > >
> > > Hi,
> > > Adding Ferruh and Zhaoyan,
> > >
> > > > Ali,
> > > >
> > > > You reported some performance regression, did you confirm it?
> > > > If I get no reply by monday, I'll proceed with this patch.
> > >
> > > Sure I'll confirm by Monday.
> > >
> > > Doesn't the regression also reproduce on the Lab's Intel servers?
> > > Even though the check iol-intel-Performance isn't failing, I can
> see that the
> > > throughput differences from expected for this patch are less than
> those of
> > > another patch that was tested only 20 minutes earlier. Both patches
> were
> > > applied to the same tree:
> > >
> > > https://mails.dpdk.org/archives/test-report/2021-
> January/173927.html
> > > > | 64 | 512 | 1.571 |
> > >
> > > https://mails.dpdk.org/archives/test-report/2021-
> January/173919.html
> > > > | 64 | 512 | 2.698 |
> > >
> > > Assuming that pw86457 doesn't have an effect on this test, it looks
> to me
> > > that this patch caused a regression in Intel hardware as well.
> > >
> > > Can someone update the baseline's expected values for the Intel
> NICs and
> > > rerun the test on this patch?
> > >
> > > Thanks,
> > > Ali
On 1/19/2021 8:53 AM, Morten Brørup wrote:
> Could someone at Intel please update the test script to provide output according to the test plan? Or delegate to the right person.
>
> According to the test plan, the information requested by Olivier should be in the test output already:
> http://git.dpdk.org/tools/dts/tree/test_plans/nic_single_core_perf_test_plan.rst?h=next
>
> PS: I can't find out who is the maintainer of the test plan, so I'm randomly pointing my finger at the test plan doc copyright holder. :-)
>
Hi Morten,
Ali has a request to update the expected baseline, to be able to detect the
performance drops, let me internally figure out who can do this.
And do you have any other request, or asking same thing?
>
> Med venlig hilsen / kind regards
> - Morten Brørup
>
>> -----Original Message-----
>> From: Olivier Matz [mailto:olivier.matz@6wind.com]
>> Sent: Tuesday, January 19, 2021 9:32 AM
>> To: Ali Alnubani
>> Cc: David Marchand; Ferruh Yigit; zhaoyan.chen@intel.com; dev; Andrew
>> Rybchenko; Ananyev, Konstantin; Morten Brørup; ajitkhaparde@gmail.com;
>> dpdk stable; Ajit Khaparde; Slava Ovsiienko; Alexander Kozyrev
>> Subject: Re: [dpdk-stable] [PATCH v4] mbuf: fix reset on mbuf free
>>
>> Hi Ali,
>>
>>
>> On Mon, Jan 18, 2021 at 05:52:32PM +0000, Ali Alnubani wrote:
>>> Hi,
>>> (Sorry had to resend this to some recipients due to mail server
>> problems).
>>>
>>> Just confirming that I can still reproduce the regression with single
>> core and 64B frames on other servers.
>>
>> Many thanks for the feedback. Can you please detail what is the amount
>> of performance loss in percent, and confirm the test case? (I suppose
>> it
>> is testpmd io forward).
>>
>> Unfortunatly, I won't be able to spend a lot of time on this soon
>> (sorry
>> for that). So I see at least these 2 options:
>>
>> - postpone the patch again, until I can find more time to analyze
>> and optimize
>> - apply the patch if the performance loss is acceptable compared to
>> the added value of fixing a bug
>>
>> Regards,
>> Olivier
>>
>>
>>>
>>> - Ali
>>>
>>>> -----Original Message-----
>>>> From: Ali Alnubani <alialnu@nvidia.com>
>>>> Sent: Friday, January 15, 2021 8:39 PM
>>>> To: David Marchand <david.marchand@redhat.com>; Olivier Matz
>>>> <olivier.matz@6wind.com>; Ferruh Yigit <ferruh.yigit@intel.com>;
>>>> zhaoyan.chen@intel.com
>>>> Cc: dev <dev@dpdk.org>; Andrew Rybchenko
>>>> <andrew.rybchenko@oktetlabs.ru>; Ananyev, Konstantin
>>>> <konstantin.ananyev@intel.com>; Morten Brørup
>>>> <mb@smartsharesystems.com>; ajitkhaparde@gmail.com; dpdk stable
>>>> <stable@dpdk.org>; Ajit Khaparde <ajit.khaparde@broadcom.com>
>>>> Subject: Re: [dpdk-stable] [PATCH v4] mbuf: fix reset on mbuf free
>>>>
>>>> Hi,
>>>> Adding Ferruh and Zhaoyan,
>>>>
>>>>> Ali,
>>>>>
>>>>> You reported some performance regression, did you confirm it?
>>>>> If I get no reply by monday, I'll proceed with this patch.
>>>>
>>>> Sure I'll confirm by Monday.
>>>>
>>>> Doesn't the regression also reproduce on the Lab's Intel servers?
>>>> Even though the check iol-intel-Performance isn't failing, I can
>> see that the
>>>> throughput differences from expected for this patch are less than
>> those of
>>>> another patch that was tested only 20 minutes earlier. Both patches
>> were
>>>> applied to the same tree:
>>>>
>>>> https://mails.dpdk.org/archives/test-report/2021-
>> January/173927.html
>>>>> | 64 | 512 | 1.571 |
>>>>
>>>> https://mails.dpdk.org/archives/test-report/2021-
>> January/173919.html
>>>>> | 64 | 512 | 2.698 |
>>>>
>>>> Assuming that pw86457 doesn't have an effect on this test, it looks
>> to me
>>>> that this patch caused a regression in Intel hardware as well.
>>>>
>>>> Can someone update the baseline's expected values for the Intel
>> NICs and
>>>> rerun the test on this patch?
>>>>
>>>> Thanks,
>>>> Ali
>
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ferruh Yigit
> Sent: Tuesday, January 19, 2021 1:01 PM
>
> On 1/19/2021 8:53 AM, Morten Brørup wrote:
> > Could someone at Intel please update the test script to provide
> output according to the test plan? Or delegate to the right person.
> >
> > According to the test plan, the information requested by Olivier
> should be in the test output already:
> >
> http://git.dpdk.org/tools/dts/tree/test_plans/nic_single_core_perf_test
> _plan.rst?h=next
> >
> > PS: I can't find out who is the maintainer of the test plan, so I'm
> randomly pointing my finger at the test plan doc copyright holder. :-)
> >
>
> Hi Morten,
>
> Ali has a request to update the expected baseline, to be able to detect
> the
> performance drops, let me internally figure out who can do this.
>
> And do you have any other request, or asking same thing?
>
Hi Ferruh,
I am asking for something else:
The test script does not provide the output that its documentation says that it does.
Apparently, the test script for nic_single_core_perf produces an output table with these four columns (as seen at https://lab.dpdk.org/results/dashboard/patchsets/15142/#env-18):
+--------+--------------------+-----------------------+------------------------------+
| Result | frame_size (bytes) | txd/rxd (descriptors) | throughput Difference (Mpps) |
+--------+--------------------+-----------------------+------------------------------+
| PASS | 64 | 512 | 1.57100 |
+--------+--------------------+-----------------------+------------------------------+
| PASS | 64 | 2048 | 1.87500 |
+--------+--------------------+-----------------------+------------------------------+
But the test plan documentation (at http://git.dpdk.org/tools/dts/tree/test_plans/nic_single_core_perf_test_plan.rst) says that this output should be produced:
+------------+---------+-------------+---------+---------------------+
| Frame Size | TXD/RXD | Throughput | Rate | Expected Throughput |
+------------+---------+-------------+---------+---------------------+
| 64 | 512 | xxxxxx Mpps | xxx % | xxx Mpps |
+------------+---------+-------------+---------+---------------------+
| 64 | 2048 | xxxxxx Mpps | xxx % | xxx Mpps |
+------------+---------+-------------+---------+---------------------+
Olivier and I am saying that only showing the Throughput Difference (Mpps) does not provide any perspective to the result.
I am requesting that the Expected Throughput (Mpps) should be shown in the result too, as documented in the test plan.
> >
> > Med venlig hilsen / kind regards
> > - Morten Brørup
> >
> >> -----Original Message-----
> >> From: Olivier Matz [mailto:olivier.matz@6wind.com]
> >> Sent: Tuesday, January 19, 2021 9:32 AM
> >> To: Ali Alnubani
> >> Cc: David Marchand; Ferruh Yigit; zhaoyan.chen@intel.com; dev;
> Andrew
> >> Rybchenko; Ananyev, Konstantin; Morten Brørup;
> ajitkhaparde@gmail.com;
> >> dpdk stable; Ajit Khaparde; Slava Ovsiienko; Alexander Kozyrev
> >> Subject: Re: [dpdk-stable] [PATCH v4] mbuf: fix reset on mbuf free
> >>
> >> Hi Ali,
> >>
> >>
> >> On Mon, Jan 18, 2021 at 05:52:32PM +0000, Ali Alnubani wrote:
> >>> Hi,
> >>> (Sorry had to resend this to some recipients due to mail server
> >> problems).
> >>>
> >>> Just confirming that I can still reproduce the regression with
> single
> >> core and 64B frames on other servers.
> >>
> >> Many thanks for the feedback. Can you please detail what is the
> amount
> >> of performance loss in percent, and confirm the test case? (I
> suppose
> >> it
> >> is testpmd io forward).
> >>
> >> Unfortunatly, I won't be able to spend a lot of time on this soon
> >> (sorry
> >> for that). So I see at least these 2 options:
> >>
> >> - postpone the patch again, until I can find more time to analyze
> >> and optimize
> >> - apply the patch if the performance loss is acceptable compared to
> >> the added value of fixing a bug
> >>
> >> Regards,
> >> Olivier
> >>
> >>
> >>>
> >>> - Ali
> >>>
> >>>> -----Original Message-----
> >>>> From: Ali Alnubani <alialnu@nvidia.com>
> >>>> Sent: Friday, January 15, 2021 8:39 PM
> >>>> To: David Marchand <david.marchand@redhat.com>; Olivier Matz
> >>>> <olivier.matz@6wind.com>; Ferruh Yigit <ferruh.yigit@intel.com>;
> >>>> zhaoyan.chen@intel.com
> >>>> Cc: dev <dev@dpdk.org>; Andrew Rybchenko
> >>>> <andrew.rybchenko@oktetlabs.ru>; Ananyev, Konstantin
> >>>> <konstantin.ananyev@intel.com>; Morten Brørup
> >>>> <mb@smartsharesystems.com>; ajitkhaparde@gmail.com; dpdk stable
> >>>> <stable@dpdk.org>; Ajit Khaparde <ajit.khaparde@broadcom.com>
> >>>> Subject: Re: [dpdk-stable] [PATCH v4] mbuf: fix reset on mbuf free
> >>>>
> >>>> Hi,
> >>>> Adding Ferruh and Zhaoyan,
> >>>>
> >>>>> Ali,
> >>>>>
> >>>>> You reported some performance regression, did you confirm it?
> >>>>> If I get no reply by monday, I'll proceed with this patch.
> >>>>
> >>>> Sure I'll confirm by Monday.
> >>>>
> >>>> Doesn't the regression also reproduce on the Lab's Intel servers?
> >>>> Even though the check iol-intel-Performance isn't failing, I can
> >> see that the
> >>>> throughput differences from expected for this patch are less than
> >> those of
> >>>> another patch that was tested only 20 minutes earlier. Both
> patches
> >> were
> >>>> applied to the same tree:
> >>>>
> >>>> https://mails.dpdk.org/archives/test-report/2021-
> >> January/173927.html
> >>>>> | 64 | 512 | 1.571 |
> >>>>
> >>>> https://mails.dpdk.org/archives/test-report/2021-
> >> January/173919.html
> >>>>> | 64 | 512 | 2.698 |
> >>>>
> >>>> Assuming that pw86457 doesn't have an effect on this test, it
> looks
> >> to me
> >>>> that this patch caused a regression in Intel hardware as well.
> >>>>
> >>>> Can someone update the baseline's expected values for the Intel
> >> NICs and
> >>>> rerun the test on this patch?
> >>>>
> >>>> Thanks,
> >>>> Ali
> >
>
On 1/19/2021 12:27 PM, Morten Brørup wrote:
>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ferruh Yigit
>> Sent: Tuesday, January 19, 2021 1:01 PM
>>
>> On 1/19/2021 8:53 AM, Morten Brørup wrote:
>>> Could someone at Intel please update the test script to provide
>> output according to the test plan? Or delegate to the right person.
>>>
>>> According to the test plan, the information requested by Olivier
>> should be in the test output already:
>>>
>> http://git.dpdk.org/tools/dts/tree/test_plans/nic_single_core_perf_test
>> _plan.rst?h=next
>>>
>>> PS: I can't find out who is the maintainer of the test plan, so I'm
>> randomly pointing my finger at the test plan doc copyright holder. :-)
>>>
>>
>> Hi Morten,
>>
>> Ali has a request to update the expected baseline, to be able to detect
>> the
>> performance drops, let me internally figure out who can do this.
>>
>> And do you have any other request, or asking same thing?
>>
>
> Hi Ferruh,
>
> I am asking for something else:
>
> The test script does not provide the output that its documentation says that it does.
>
> Apparently, the test script for nic_single_core_perf produces an output table with these four columns (as seen at https://lab.dpdk.org/results/dashboard/patchsets/15142/#env-18):
>
> +--------+--------------------+-----------------------+------------------------------+
> | Result | frame_size (bytes) | txd/rxd (descriptors) | throughput Difference (Mpps) |
> +--------+--------------------+-----------------------+------------------------------+
> | PASS | 64 | 512 | 1.57100 |
> +--------+--------------------+-----------------------+------------------------------+
> | PASS | 64 | 2048 | 1.87500 |
> +--------+--------------------+-----------------------+------------------------------+
>
> But the test plan documentation (at http://git.dpdk.org/tools/dts/tree/test_plans/nic_single_core_perf_test_plan.rst) says that this output should be produced:
>
> +------------+---------+-------------+---------+---------------------+
> | Frame Size | TXD/RXD | Throughput | Rate | Expected Throughput |
> +------------+---------+-------------+---------+---------------------+
> | 64 | 512 | xxxxxx Mpps | xxx % | xxx Mpps |
> +------------+---------+-------------+---------+---------------------+
> | 64 | 2048 | xxxxxx Mpps | xxx % | xxx Mpps |
> +------------+---------+-------------+---------+---------------------+
>
> Olivier and I am saying that only showing the Throughput Difference (Mpps) does not provide any perspective to the result.
>
> I am requesting that the Expected Throughput (Mpps) should be shown in the result too, as documented in the test plan.
>
Ahh, this has a history, when the initial community lab infrastructure prepared
some vendor(s) didn't want to show the actual throughput numbers.
That is why this diff and baseline introduced, and this is the how current
infrastructure works. So this is not something related to Intel.
And as you can imagine this is not a technical issue, some companies seems not
willing to share their performance numbers via community lab, and I don't know
if something changed here in last a few years.
>>>
>>> Med venlig hilsen / kind regards
>>> - Morten Brørup
>>>
>>>> -----Original Message-----
>>>> From: Olivier Matz [mailto:olivier.matz@6wind.com]
>>>> Sent: Tuesday, January 19, 2021 9:32 AM
>>>> To: Ali Alnubani
>>>> Cc: David Marchand; Ferruh Yigit; zhaoyan.chen@intel.com; dev;
>> Andrew
>>>> Rybchenko; Ananyev, Konstantin; Morten Brørup;
>> ajitkhaparde@gmail.com;
>>>> dpdk stable; Ajit Khaparde; Slava Ovsiienko; Alexander Kozyrev
>>>> Subject: Re: [dpdk-stable] [PATCH v4] mbuf: fix reset on mbuf free
>>>>
>>>> Hi Ali,
>>>>
>>>>
>>>> On Mon, Jan 18, 2021 at 05:52:32PM +0000, Ali Alnubani wrote:
>>>>> Hi,
>>>>> (Sorry had to resend this to some recipients due to mail server
>>>> problems).
>>>>>
>>>>> Just confirming that I can still reproduce the regression with
>> single
>>>> core and 64B frames on other servers.
>>>>
>>>> Many thanks for the feedback. Can you please detail what is the
>> amount
>>>> of performance loss in percent, and confirm the test case? (I
>> suppose
>>>> it
>>>> is testpmd io forward).
>>>>
>>>> Unfortunatly, I won't be able to spend a lot of time on this soon
>>>> (sorry
>>>> for that). So I see at least these 2 options:
>>>>
>>>> - postpone the patch again, until I can find more time to analyze
>>>> and optimize
>>>> - apply the patch if the performance loss is acceptable compared to
>>>> the added value of fixing a bug
>>>>
>>>> Regards,
>>>> Olivier
>>>>
>>>>
>>>>>
>>>>> - Ali
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Ali Alnubani <alialnu@nvidia.com>
>>>>>> Sent: Friday, January 15, 2021 8:39 PM
>>>>>> To: David Marchand <david.marchand@redhat.com>; Olivier Matz
>>>>>> <olivier.matz@6wind.com>; Ferruh Yigit <ferruh.yigit@intel.com>;
>>>>>> zhaoyan.chen@intel.com
>>>>>> Cc: dev <dev@dpdk.org>; Andrew Rybchenko
>>>>>> <andrew.rybchenko@oktetlabs.ru>; Ananyev, Konstantin
>>>>>> <konstantin.ananyev@intel.com>; Morten Brørup
>>>>>> <mb@smartsharesystems.com>; ajitkhaparde@gmail.com; dpdk stable
>>>>>> <stable@dpdk.org>; Ajit Khaparde <ajit.khaparde@broadcom.com>
>>>>>> Subject: Re: [dpdk-stable] [PATCH v4] mbuf: fix reset on mbuf free
>>>>>>
>>>>>> Hi,
>>>>>> Adding Ferruh and Zhaoyan,
>>>>>>
>>>>>>> Ali,
>>>>>>>
>>>>>>> You reported some performance regression, did you confirm it?
>>>>>>> If I get no reply by monday, I'll proceed with this patch.
>>>>>>
>>>>>> Sure I'll confirm by Monday.
>>>>>>
>>>>>> Doesn't the regression also reproduce on the Lab's Intel servers?
>>>>>> Even though the check iol-intel-Performance isn't failing, I can
>>>> see that the
>>>>>> throughput differences from expected for this patch are less than
>>>> those of
>>>>>> another patch that was tested only 20 minutes earlier. Both
>> patches
>>>> were
>>>>>> applied to the same tree:
>>>>>>
>>>>>> https://mails.dpdk.org/archives/test-report/2021-
>>>> January/173927.html
>>>>>>> | 64 | 512 | 1.571 |
>>>>>>
>>>>>> https://mails.dpdk.org/archives/test-report/2021-
>>>> January/173919.html
>>>>>>> | 64 | 512 | 2.698 |
>>>>>>
>>>>>> Assuming that pw86457 doesn't have an effect on this test, it
>> looks
>>>> to me
>>>>>> that this patch caused a regression in Intel hardware as well.
>>>>>>
>>>>>> Can someone update the baseline's expected values for the Intel
>>>> NICs and
>>>>>> rerun the test on this patch?
>>>>>>
>>>>>> Thanks,
>>>>>> Ali
>>>
>>
>
Hi, All
Could we postpose this patch at least to rc2? We would like to conduct more investigations?
With best regards, Slava
> -----Original Message-----
> From: Olivier Matz <olivier.matz@6wind.com>
> Sent: Tuesday, January 19, 2021 10:32
> To: Ali Alnubani <alialnu@nvidia.com>
> Cc: David Marchand <david.marchand@redhat.com>; Ferruh Yigit
> <ferruh.yigit@intel.com>; zhaoyan.chen@intel.com; dev <dev@dpdk.org>;
> Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>; Ananyev, Konstantin
> <konstantin.ananyev@intel.com>; Morten Brørup
> <mb@smartsharesystems.com>; ajitkhaparde@gmail.com; dpdk stable
> <stable@dpdk.org>; Ajit Khaparde <ajit.khaparde@broadcom.com>; Slava
> Ovsiienko <viacheslavo@nvidia.com>; Alexander Kozyrev
> <akozyrev@nvidia.com>
> Subject: Re: [dpdk-stable] [PATCH v4] mbuf: fix reset on mbuf free
>
> Hi Ali,
>
>
> On Mon, Jan 18, 2021 at 05:52:32PM +0000, Ali Alnubani wrote:
> > Hi,
> > (Sorry had to resend this to some recipients due to mail server problems).
> >
> > Just confirming that I can still reproduce the regression with single core and
> 64B frames on other servers.
>
> Many thanks for the feedback. Can you please detail what is the amount of
> performance loss in percent, and confirm the test case? (I suppose it is
> testpmd io forward).
>
> Unfortunatly, I won't be able to spend a lot of time on this soon (sorry for
> that). So I see at least these 2 options:
>
> - postpone the patch again, until I can find more time to analyze
> and optimize
> - apply the patch if the performance loss is acceptable compared to
> the added value of fixing a bug
>
> Regards,
> Olivier
>
>
> >
> > - Ali
> >
> > > -----Original Message-----
> > > From: Ali Alnubani <alialnu@nvidia.com>
> > > Sent: Friday, January 15, 2021 8:39 PM
> > > To: David Marchand <david.marchand@redhat.com>; Olivier Matz
> > > <olivier.matz@6wind.com>; Ferruh Yigit <ferruh.yigit@intel.com>;
> > > zhaoyan.chen@intel.com
> > > Cc: dev <dev@dpdk.org>; Andrew Rybchenko
> > > <andrew.rybchenko@oktetlabs.ru>; Ananyev, Konstantin
> > > <konstantin.ananyev@intel.com>; Morten Brørup
> > > <mb@smartsharesystems.com>; ajitkhaparde@gmail.com; dpdk stable
> > > <stable@dpdk.org>; Ajit Khaparde <ajit.khaparde@broadcom.com>
> > > Subject: Re: [dpdk-stable] [PATCH v4] mbuf: fix reset on mbuf free
> > >
> > > Hi,
> > > Adding Ferruh and Zhaoyan,
> > >
> > > > Ali,
> > > >
> > > > You reported some performance regression, did you confirm it?
> > > > If I get no reply by monday, I'll proceed with this patch.
> > >
> > > Sure I'll confirm by Monday.
> > >
> > > Doesn't the regression also reproduce on the Lab's Intel servers?
> > > Even though the check iol-intel-Performance isn't failing, I can see
> > > that the throughput differences from expected for this patch are
> > > less than those of another patch that was tested only 20 minutes
> > > earlier. Both patches were applied to the same tree:
> > >
> > > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fma
> > > ils.dpdk.org%2Farchives%2Ftest-report%2F2021-
> January%2F173927.html&a
> > >
> mp;data=04%7C01%7Cviacheslavo%40nvidia.com%7Ce2d18a8563fb42dfaba40
> 8d
> > >
> 8bc54c83b%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C63746641
> 96385
> > >
> 80374%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2lu
> MzIiLC
> > >
> JBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=VGvj%2F5GcAOxof6C
> mlZkq
> > > KXKOL52GctXcuL5RJXr1y8g%3D&reserved=0
> > > > | 64 | 512 | 1.571 |
> > >
> > > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fma
> > > ils.dpdk.org%2Farchives%2Ftest-report%2F2021-
> January%2F173919.html&a
> > >
> mp;data=04%7C01%7Cviacheslavo%40nvidia.com%7Ce2d18a8563fb42dfaba40
> 8d
> > >
> 8bc54c83b%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C63746641
> 96385
> > >
> 80374%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2lu
> MzIiLC
> > >
> JBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=XSirRbm5G0WwfxySe
> b0ALp
> > > owVosqoY6Nlv4UZCd1CZM%3D&reserved=0
> > > > | 64 | 512 | 2.698 |
> > >
> > > Assuming that pw86457 doesn't have an effect on this test, it looks
> > > to me that this patch caused a regression in Intel hardware as well.
> > >
> > > Can someone update the baseline's expected values for the Intel NICs
> > > and rerun the test on this patch?
> > >
> > > Thanks,
> > > Ali
> From: Ferruh Yigit [mailto:ferruh.yigit@intel.com]
> Sent: Tuesday, January 19, 2021 3:03 PM
>
> On 1/19/2021 12:27 PM, Morten Brørup wrote:
> >> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ferruh Yigit
> >> Sent: Tuesday, January 19, 2021 1:01 PM
> >>
> >> On 1/19/2021 8:53 AM, Morten Brørup wrote:
> >>> Could someone at Intel please update the test script to provide
> >> output according to the test plan? Or delegate to the right person.
> >>>
> >>> According to the test plan, the information requested by Olivier
> >> should be in the test output already:
> >>>
> >>
> http://git.dpdk.org/tools/dts/tree/test_plans/nic_single_core_perf_test
> >> _plan.rst?h=next
> >>>
> >>> PS: I can't find out who is the maintainer of the test plan, so I'm
> >> randomly pointing my finger at the test plan doc copyright holder.
> :-)
> >>>
> >>
> >> Hi Morten,
> >>
> >> Ali has a request to update the expected baseline, to be able to
> detect
> >> the
> >> performance drops, let me internally figure out who can do this.
> >>
> >> And do you have any other request, or asking same thing?
> >>
> >
> > Hi Ferruh,
> >
> > I am asking for something else:
> >
> > The test script does not provide the output that its documentation
> says that it does.
> >
> > Apparently, the test script for nic_single_core_perf produces an
> output table with these four columns (as seen at
> https://lab.dpdk.org/results/dashboard/patchsets/15142/#env-18):
> >
> > +--------+--------------------+-----------------------+----------
> --------------------+
> > | Result | frame_size (bytes) | txd/rxd (descriptors) |
> throughput Difference (Mpps) |
> > +--------+--------------------+-----------------------+----------
> --------------------+
> > | PASS | 64 | 512 | 1.57100
> |
> > +--------+--------------------+-----------------------+----------
> --------------------+
> > | PASS | 64 | 2048 | 1.87500
> |
> > +--------+--------------------+-----------------------+----------
> --------------------+
> >
> > But the test plan documentation (at
> http://git.dpdk.org/tools/dts/tree/test_plans/nic_single_core_perf_test
> _plan.rst) says that this output should be produced:
> >
> > +------------+---------+-------------+---------+-----------------
> ----+
> > | Frame Size | TXD/RXD | Throughput | Rate | Expected
> Throughput |
> > +------------+---------+-------------+---------+-----------------
> ----+
> > | 64 | 512 | xxxxxx Mpps | xxx % | xxx Mpps
> |
> > +------------+---------+-------------+---------+-----------------
> ----+
> > | 64 | 2048 | xxxxxx Mpps | xxx % | xxx Mpps
> |
> > +------------+---------+-------------+---------+-----------------
> ----+
> >
> > Olivier and I am saying that only showing the Throughput Difference
> (Mpps) does not provide any perspective to the result.
> >
> > I am requesting that the Expected Throughput (Mpps) should be shown
> in the result too, as documented in the test plan.
> >
>
> Ahh, this has a history, when the initial community lab infrastructure
> prepared
> some vendor(s) didn't want to show the actual throughput numbers.
>
> That is why this diff and baseline introduced, and this is the how
> current
> infrastructure works. So this is not something related to Intel.
>
> And as you can imagine this is not a technical issue, some companies
> seems not
> willing to share their performance numbers via community lab, and I
> don't know
> if something changed here in last a few years.
>
That explains it!
If those companies still want to keep the community lab performance data hidden (which I don't object to), wouldn't it be better if the performance test scripts output the deviation from the expected throughput in percent (with one or two decimals after the comma) instead of in Mpps?
>
> >>>
> >>> Med venlig hilsen / kind regards
> >>> - Morten Brørup
> >>>
> >>>> -----Original Message-----
> >>>> From: Olivier Matz [mailto:olivier.matz@6wind.com]
> >>>> Sent: Tuesday, January 19, 2021 9:32 AM
> >>>> To: Ali Alnubani
> >>>> Cc: David Marchand; Ferruh Yigit; zhaoyan.chen@intel.com; dev;
> >> Andrew
> >>>> Rybchenko; Ananyev, Konstantin; Morten Brørup;
> >> ajitkhaparde@gmail.com;
> >>>> dpdk stable; Ajit Khaparde; Slava Ovsiienko; Alexander Kozyrev
> >>>> Subject: Re: [dpdk-stable] [PATCH v4] mbuf: fix reset on mbuf free
> >>>>
> >>>> Hi Ali,
> >>>>
> >>>>
> >>>> On Mon, Jan 18, 2021 at 05:52:32PM +0000, Ali Alnubani wrote:
> >>>>> Hi,
> >>>>> (Sorry had to resend this to some recipients due to mail server
> >>>> problems).
> >>>>>
> >>>>> Just confirming that I can still reproduce the regression with
> >> single
> >>>> core and 64B frames on other servers.
> >>>>
> >>>> Many thanks for the feedback. Can you please detail what is the
> >> amount
> >>>> of performance loss in percent, and confirm the test case? (I
> >> suppose
> >>>> it
> >>>> is testpmd io forward).
> >>>>
> >>>> Unfortunatly, I won't be able to spend a lot of time on this soon
> >>>> (sorry
> >>>> for that). So I see at least these 2 options:
> >>>>
> >>>> - postpone the patch again, until I can find more time to analyze
> >>>> and optimize
> >>>> - apply the patch if the performance loss is acceptable compared
> to
> >>>> the added value of fixing a bug
> >>>>
> >>>> Regards,
> >>>> Olivier
> >>>>
> >>>>
> >>>>>
> >>>>> - Ali
> >>>>>
> >>>>>> -----Original Message-----
> >>>>>> From: Ali Alnubani <alialnu@nvidia.com>
> >>>>>> Sent: Friday, January 15, 2021 8:39 PM
> >>>>>> To: David Marchand <david.marchand@redhat.com>; Olivier Matz
> >>>>>> <olivier.matz@6wind.com>; Ferruh Yigit <ferruh.yigit@intel.com>;
> >>>>>> zhaoyan.chen@intel.com
> >>>>>> Cc: dev <dev@dpdk.org>; Andrew Rybchenko
> >>>>>> <andrew.rybchenko@oktetlabs.ru>; Ananyev, Konstantin
> >>>>>> <konstantin.ananyev@intel.com>; Morten Brørup
> >>>>>> <mb@smartsharesystems.com>; ajitkhaparde@gmail.com; dpdk stable
> >>>>>> <stable@dpdk.org>; Ajit Khaparde <ajit.khaparde@broadcom.com>
> >>>>>> Subject: Re: [dpdk-stable] [PATCH v4] mbuf: fix reset on mbuf
> free
> >>>>>>
> >>>>>> Hi,
> >>>>>> Adding Ferruh and Zhaoyan,
> >>>>>>
> >>>>>>> Ali,
> >>>>>>>
> >>>>>>> You reported some performance regression, did you confirm it?
> >>>>>>> If I get no reply by monday, I'll proceed with this patch.
> >>>>>>
> >>>>>> Sure I'll confirm by Monday.
> >>>>>>
> >>>>>> Doesn't the regression also reproduce on the Lab's Intel
> servers?
> >>>>>> Even though the check iol-intel-Performance isn't failing, I can
> >>>> see that the
> >>>>>> throughput differences from expected for this patch are less
> than
> >>>> those of
> >>>>>> another patch that was tested only 20 minutes earlier. Both
> >> patches
> >>>> were
> >>>>>> applied to the same tree:
> >>>>>>
> >>>>>> https://mails.dpdk.org/archives/test-report/2021-
> >>>> January/173927.html
> >>>>>>> | 64 | 512 | 1.571 |
> >>>>>>
> >>>>>> https://mails.dpdk.org/archives/test-report/2021-
> >>>> January/173919.html
> >>>>>>> | 64 | 512 | 2.698 |
> >>>>>>
> >>>>>> Assuming that pw86457 doesn't have an effect on this test, it
> >> looks
> >>>> to me
> >>>>>> that this patch caused a regression in Intel hardware as well.
> >>>>>>
> >>>>>> Can someone update the baseline's expected values for the Intel
> >>>> NICs and
> >>>>>> rerun the test on this patch?
> >>>>>>
> >>>>>> Thanks,
> >>>>>> Ali
> >>>
> >>
> >
>
On 1/19/2021 2:21 PM, Morten Brørup wrote:
>> From: Ferruh Yigit [mailto:ferruh.yigit@intel.com]
>> Sent: Tuesday, January 19, 2021 3:03 PM
>>
>> On 1/19/2021 12:27 PM, Morten Brørup wrote:
>>>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ferruh Yigit
>>>> Sent: Tuesday, January 19, 2021 1:01 PM
>>>>
>>>> On 1/19/2021 8:53 AM, Morten Brørup wrote:
>>>>> Could someone at Intel please update the test script to provide
>>>> output according to the test plan? Or delegate to the right person.
>>>>>
>>>>> According to the test plan, the information requested by Olivier
>>>> should be in the test output already:
>>>>>
>>>>
>> http://git.dpdk.org/tools/dts/tree/test_plans/nic_single_core_perf_test
>>>> _plan.rst?h=next
>>>>>
>>>>> PS: I can't find out who is the maintainer of the test plan, so I'm
>>>> randomly pointing my finger at the test plan doc copyright holder.
>> :-)
>>>>>
>>>>
>>>> Hi Morten,
>>>>
>>>> Ali has a request to update the expected baseline, to be able to
>> detect
>>>> the
>>>> performance drops, let me internally figure out who can do this.
>>>>
>>>> And do you have any other request, or asking same thing?
>>>>
>>>
>>> Hi Ferruh,
>>>
>>> I am asking for something else:
>>>
>>> The test script does not provide the output that its documentation
>> says that it does.
>>>
>>> Apparently, the test script for nic_single_core_perf produces an
>> output table with these four columns (as seen at
>> https://lab.dpdk.org/results/dashboard/patchsets/15142/#env-18):
>>>
>>> +--------+--------------------+-----------------------+----------
>> --------------------+
>>> | Result | frame_size (bytes) | txd/rxd (descriptors) |
>> throughput Difference (Mpps) |
>>> +--------+--------------------+-----------------------+----------
>> --------------------+
>>> | PASS | 64 | 512 | 1.57100
>> |
>>> +--------+--------------------+-----------------------+----------
>> --------------------+
>>> | PASS | 64 | 2048 | 1.87500
>> |
>>> +--------+--------------------+-----------------------+----------
>> --------------------+
>>>
>>> But the test plan documentation (at
>> http://git.dpdk.org/tools/dts/tree/test_plans/nic_single_core_perf_test
>> _plan.rst) says that this output should be produced:
>>>
>>> +------------+---------+-------------+---------+-----------------
>> ----+
>>> | Frame Size | TXD/RXD | Throughput | Rate | Expected
>> Throughput |
>>> +------------+---------+-------------+---------+-----------------
>> ----+
>>> | 64 | 512 | xxxxxx Mpps | xxx % | xxx Mpps
>> |
>>> +------------+---------+-------------+---------+-----------------
>> ----+
>>> | 64 | 2048 | xxxxxx Mpps | xxx % | xxx Mpps
>> |
>>> +------------+---------+-------------+---------+-----------------
>> ----+
>>>
>>> Olivier and I am saying that only showing the Throughput Difference
>> (Mpps) does not provide any perspective to the result.
>>>
>>> I am requesting that the Expected Throughput (Mpps) should be shown
>> in the result too, as documented in the test plan.
>>>
>>
>> Ahh, this has a history, when the initial community lab infrastructure
>> prepared
>> some vendor(s) didn't want to show the actual throughput numbers.
>>
>> That is why this diff and baseline introduced, and this is the how
>> current
>> infrastructure works. So this is not something related to Intel.
>>
>> And as you can imagine this is not a technical issue, some companies
>> seems not
>> willing to share their performance numbers via community lab, and I
>> don't know
>> if something changed here in last a few years.
>>
>
> That explains it!
>
> If those companies still want to keep the community lab performance data hidden (which I don't object to), wouldn't it be better if the performance test scripts output the deviation from the expected throughput in percent (with one or two decimals after the comma) instead of in Mpps?
>
Sounds reasonable, I assume there is a reason behind it but I don't remember,
cc'ed lab.
>>
>>>>>
>>>>> Med venlig hilsen / kind regards
>>>>> - Morten Brørup
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Olivier Matz [mailto:olivier.matz@6wind.com]
>>>>>> Sent: Tuesday, January 19, 2021 9:32 AM
>>>>>> To: Ali Alnubani
>>>>>> Cc: David Marchand; Ferruh Yigit; zhaoyan.chen@intel.com; dev;
>>>> Andrew
>>>>>> Rybchenko; Ananyev, Konstantin; Morten Brørup;
>>>> ajitkhaparde@gmail.com;
>>>>>> dpdk stable; Ajit Khaparde; Slava Ovsiienko; Alexander Kozyrev
>>>>>> Subject: Re: [dpdk-stable] [PATCH v4] mbuf: fix reset on mbuf free
>>>>>>
>>>>>> Hi Ali,
>>>>>>
>>>>>>
>>>>>> On Mon, Jan 18, 2021 at 05:52:32PM +0000, Ali Alnubani wrote:
>>>>>>> Hi,
>>>>>>> (Sorry had to resend this to some recipients due to mail server
>>>>>> problems).
>>>>>>>
>>>>>>> Just confirming that I can still reproduce the regression with
>>>> single
>>>>>> core and 64B frames on other servers.
>>>>>>
>>>>>> Many thanks for the feedback. Can you please detail what is the
>>>> amount
>>>>>> of performance loss in percent, and confirm the test case? (I
>>>> suppose
>>>>>> it
>>>>>> is testpmd io forward).
>>>>>>
>>>>>> Unfortunatly, I won't be able to spend a lot of time on this soon
>>>>>> (sorry
>>>>>> for that). So I see at least these 2 options:
>>>>>>
>>>>>> - postpone the patch again, until I can find more time to analyze
>>>>>> and optimize
>>>>>> - apply the patch if the performance loss is acceptable compared
>> to
>>>>>> the added value of fixing a bug
>>>>>>
>>>>>> Regards,
>>>>>> Olivier
>>>>>>
>>>>>>
>>>>>>>
>>>>>>> - Ali
>>>>>>>
>>>>>>>> -----Original Message-----
>>>>>>>> From: Ali Alnubani <alialnu@nvidia.com>
>>>>>>>> Sent: Friday, January 15, 2021 8:39 PM
>>>>>>>> To: David Marchand <david.marchand@redhat.com>; Olivier Matz
>>>>>>>> <olivier.matz@6wind.com>; Ferruh Yigit <ferruh.yigit@intel.com>;
>>>>>>>> zhaoyan.chen@intel.com
>>>>>>>> Cc: dev <dev@dpdk.org>; Andrew Rybchenko
>>>>>>>> <andrew.rybchenko@oktetlabs.ru>; Ananyev, Konstantin
>>>>>>>> <konstantin.ananyev@intel.com>; Morten Brørup
>>>>>>>> <mb@smartsharesystems.com>; ajitkhaparde@gmail.com; dpdk stable
>>>>>>>> <stable@dpdk.org>; Ajit Khaparde <ajit.khaparde@broadcom.com>
>>>>>>>> Subject: Re: [dpdk-stable] [PATCH v4] mbuf: fix reset on mbuf
>> free
>>>>>>>>
>>>>>>>> Hi,
>>>>>>>> Adding Ferruh and Zhaoyan,
>>>>>>>>
>>>>>>>>> Ali,
>>>>>>>>>
>>>>>>>>> You reported some performance regression, did you confirm it?
>>>>>>>>> If I get no reply by monday, I'll proceed with this patch.
>>>>>>>>
>>>>>>>> Sure I'll confirm by Monday.
>>>>>>>>
>>>>>>>> Doesn't the regression also reproduce on the Lab's Intel
>> servers?
>>>>>>>> Even though the check iol-intel-Performance isn't failing, I can
>>>>>> see that the
>>>>>>>> throughput differences from expected for this patch are less
>> than
>>>>>> those of
>>>>>>>> another patch that was tested only 20 minutes earlier. Both
>>>> patches
>>>>>> were
>>>>>>>> applied to the same tree:
>>>>>>>>
>>>>>>>> https://mails.dpdk.org/archives/test-report/2021-
>>>>>> January/173927.html
>>>>>>>>> | 64 | 512 | 1.571 |
>>>>>>>>
>>>>>>>> https://mails.dpdk.org/archives/test-report/2021-
>>>>>> January/173919.html
>>>>>>>>> | 64 | 512 | 2.698 |
>>>>>>>>
>>>>>>>> Assuming that pw86457 doesn't have an effect on this test, it
>>>> looks
>>>>>> to me
>>>>>>>> that this patch caused a regression in Intel hardware as well.
>>>>>>>>
>>>>>>>> Can someone update the baseline's expected values for the Intel
>>>>>> NICs and
>>>>>>>> rerun the test on this patch?
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Ali
>>>>>
>>>>
>>>
>>
>
On 1/15/2021 6:39 PM, Ali Alnubani wrote:
> Hi,
> Adding Ferruh and Zhaoyan,
>
>> Ali,
>>
>> You reported some performance regression, did you confirm it?
>> If I get no reply by monday, I'll proceed with this patch.
>
> Sure I'll confirm by Monday.
>
> Doesn't the regression also reproduce on the Lab's Intel servers?
> Even though the check iol-intel-Performance isn't failing, I can see that the throughput differences from expected for this patch are less than those of another patch that was tested only 20 minutes earlier. Both patches were applied to the same tree:
>
> https://mails.dpdk.org/archives/test-report/2021-January/173927.html
>> | 64 | 512 | 1.571 |
>
> https://mails.dpdk.org/archives/test-report/2021-January/173919.html
>> | 64 | 512 | 2.698 |
>
> Assuming that pw86457 doesn't have an effect on this test, it looks to me that this patch caused a regression in Intel hardware as well.
>
> Can someone update the baseline's expected values for the Intel NICs and rerun the test on this patch?
>
Zhaoyan said that the baseline is calculated dynamically,
what I understand is baseline set based on previous days performance result, so
it shouldn't require updating.
But cc'ed the lab for more details.
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ferruh Yigit
> Sent: Thursday, January 21, 2021 10:19 AM
>
> On 1/15/2021 6:39 PM, Ali Alnubani wrote:
> > Hi,
> > Adding Ferruh and Zhaoyan,
> >
> >> Ali,
> >>
> >> You reported some performance regression, did you confirm it?
> >> If I get no reply by monday, I'll proceed with this patch.
> >
> > Sure I'll confirm by Monday.
> >
> > Doesn't the regression also reproduce on the Lab's Intel servers?
> > Even though the check iol-intel-Performance isn't failing, I can see
> that the throughput differences from expected for this patch are less
> than those of another patch that was tested only 20 minutes earlier.
> Both patches were applied to the same tree:
> >
> > https://mails.dpdk.org/archives/test-report/2021-January/173927.html
> >> | 64 | 512 | 1.571 |
> >
> > https://mails.dpdk.org/archives/test-report/2021-January/173919.html
> >> | 64 | 512 | 2.698 |
> >
> > Assuming that pw86457 doesn't have an effect on this test, it looks
> to me that this patch caused a regression in Intel hardware as well.
> >
> > Can someone update the baseline's expected values for the Intel NICs
> and rerun the test on this patch?
> >
>
> Zhaoyan said that the baseline is calculated dynamically,
> what I understand is baseline set based on previous days performance
> result, so
> it shouldn't require updating.
That sounds smart!
Perhaps another reference baseline could be added, for informational purposes only:
Deviation from the performance of the last official release.
>
> But cc'ed the lab for more details.
Hi All,
Trying to follow the specific conversation. It is correct, the lab does
not list the specific throughput values achieved by the hardware, as that
data can be sensitive to the hardware vendors, etc. The purpose of the lab
is to check for degradations caused by patches, so the difference is really
the important factor. The comparison is against a prior run on the same
hardware, via the DPDK main branch, so any delta should be caused by the
specific patch changes (excluding statistical "wiggle").
If the group would prefer, we could calculate additional references if
desired (i.e. difference from the last official release, or a monthly run
of the current, etc.). We just need the community to define their needs,
and we can add this to the development queue.
Cheers,
Lincoln
On Thu, Jan 21, 2021 at 4:29 AM Morten Brørup <mb@smartsharesystems.com>
wrote:
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ferruh Yigit
> > Sent: Thursday, January 21, 2021 10:19 AM
> >
> > On 1/15/2021 6:39 PM, Ali Alnubani wrote:
> > > Hi,
> > > Adding Ferruh and Zhaoyan,
> > >
> > >> Ali,
> > >>
> > >> You reported some performance regression, did you confirm it?
> > >> If I get no reply by monday, I'll proceed with this patch.
> > >
> > > Sure I'll confirm by Monday.
> > >
> > > Doesn't the regression also reproduce on the Lab's Intel servers?
> > > Even though the check iol-intel-Performance isn't failing, I can see
> > that the throughput differences from expected for this patch are less
> > than those of another patch that was tested only 20 minutes earlier.
> > Both patches were applied to the same tree:
> > >
> > > https://mails.dpdk.org/archives/test-report/2021-January/173927.html
> > >> | 64 | 512 | 1.571 |
> > >
> > > https://mails.dpdk.org/archives/test-report/2021-January/173919.html
> > >> | 64 | 512 | 2.698 |
> > >
> > > Assuming that pw86457 doesn't have an effect on this test, it looks
> > to me that this patch caused a regression in Intel hardware as well.
> > >
> > > Can someone update the baseline's expected values for the Intel NICs
> > and rerun the test on this patch?
> > >
> >
> > Zhaoyan said that the baseline is calculated dynamically,
> > what I understand is baseline set based on previous days performance
> > result, so
> > it shouldn't require updating.
>
> That sounds smart!
>
> Perhaps another reference baseline could be added, for informational
> purposes only:
> Deviation from the performance of the last official release.
>
> >
> > But cc'ed the lab for more details.
>
>
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Lincoln Lavoie
> Sent: Thursday, January 21, 2021 5:35 PM
> To: Morten Brørup
>
> Hi All,
>
> Trying to follow the specific conversation. It is correct, the lab
> does
> not list the specific throughput values achieved by the hardware, as
> that
> data can be sensitive to the hardware vendors, etc. The purpose of the
> lab
> is to check for degradations caused by patches, so the difference is
> really
> the important factor. The comparison is against a prior run on the
> same
> hardware, via the DPDK main branch, so any delta should be caused by
> the
> specific patch changes (excluding statistical "wiggle").
>
Thank you for clarifying, Lincoln.
This sounds like a perfect solution to the meet the purpose.
I request that you change the output to show the relative difference (i.e. percentage above/below baseline), instead of the absolute difference (i.e. number of packets per second).
By showing a percentage, anyone reading the test report can understand if the difference is insignificant, or big enough to require further discussion before accepting the patch. Showing the difference in packets per second requires the reader of the test report to have prior knowledge about the expected performance.
> If the group would prefer, we could calculate additional references if
> desired (i.e. difference from the last official release, or a monthly
> run
> of the current, etc.). We just need the community to define their
> needs,
> and we can add this to the development queue.
>
I retract my suggestion about adding additional references. You clearly explained how your baselining works, and I think it fully serves the needs of a regression test.
So please just put this small change in your development queue: Output the difference in percent with a few decimals after the comma, instead of the difference in packets per second.
For readability, performance drops should be shown as negative values, and increases as positive, e.g.:
Difference = (NewPPS - BaselinePPS) / BaselinePPS * 100.00 %.
If we want to compare performance against official releases, current or older, it should go elsewhere, not in the continuous testing environment. E.g. the release notes could include a report showing differences from the last few official releases. But that is a task for another day.
> Cheers,
> Lincoln
>
>
> On Thu, Jan 21, 2021 at 4:29 AM Morten Brørup
> <mb@smartsharesystems.com>
> wrote:
>
> > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ferruh Yigit
> > > Sent: Thursday, January 21, 2021 10:19 AM
> > >
> > > On 1/15/2021 6:39 PM, Ali Alnubani wrote:
> > > > Hi,
> > > > Adding Ferruh and Zhaoyan,
> > > >
> > > >> Ali,
> > > >>
> > > >> You reported some performance regression, did you confirm it?
> > > >> If I get no reply by monday, I'll proceed with this patch.
> > > >
> > > > Sure I'll confirm by Monday.
> > > >
> > > > Doesn't the regression also reproduce on the Lab's Intel servers?
> > > > Even though the check iol-intel-Performance isn't failing, I can
> see
> > > that the throughput differences from expected for this patch are
> less
> > > than those of another patch that was tested only 20 minutes
> earlier.
> > > Both patches were applied to the same tree:
> > > >
> > > > https://mails.dpdk.org/archives/test-report/2021-
> January/173927.html
> > > >> | 64 | 512 | 1.571 |
> > > >
> > > > https://mails.dpdk.org/archives/test-report/2021-
> January/173919.html
> > > >> | 64 | 512 | 2.698 |
> > > >
> > > > Assuming that pw86457 doesn't have an effect on this test, it
> looks
> > > to me that this patch caused a regression in Intel hardware as
> well.
> > > >
> > > > Can someone update the baseline's expected values for the Intel
> NICs
> > > and rerun the test on this patch?
> > > >
> > >
> > > Zhaoyan said that the baseline is calculated dynamically,
> > > what I understand is baseline set based on previous days
> performance
> > > result, so
> > > it shouldn't require updating.
> >
> > That sounds smart!
> >
> > Perhaps another reference baseline could be added, for informational
> > purposes only:
> > Deviation from the performance of the last official release.
> >
> > >
> > > But cc'ed the lab for more details.
> >
> >
>
> --
> *Lincoln Lavoie*
> Senior Engineer, Broadband Technologies
> 21 Madbury Rd., Ste. 100, Durham, NH 03824
> lylavoie@iol.unh.edu
> https://www.iol.unh.edu
> +1-603-674-2755 (m)
> <https://www.iol.unh.edu>
Hi,
This seems like a good task for us to do. I will see what it would
take in order to convert the difference into a decimal-formatted
percentage.
I have put this into bugzilla to keep track of this issue:
https://bugs.dpdk.org/show_bug.cgi?id=626
Thanks,
Brandon
On Sat, Jan 23, 2021 at 3:57 AM Morten Brørup <mb@smartsharesystems.com> wrote:
>
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Lincoln Lavoie
> > Sent: Thursday, January 21, 2021 5:35 PM
> > To: Morten Brørup
> >
> > Hi All,
> >
> > Trying to follow the specific conversation. It is correct, the lab
> > does
> > not list the specific throughput values achieved by the hardware, as
> > that
> > data can be sensitive to the hardware vendors, etc. The purpose of the
> > lab
> > is to check for degradations caused by patches, so the difference is
> > really
> > the important factor. The comparison is against a prior run on the
> > same
> > hardware, via the DPDK main branch, so any delta should be caused by
> > the
> > specific patch changes (excluding statistical "wiggle").
> >
>
> Thank you for clarifying, Lincoln.
>
> This sounds like a perfect solution to the meet the purpose.
>
> I request that you change the output to show the relative difference (i.e. percentage above/below baseline), instead of the absolute difference (i.e. number of packets per second).
>
> By showing a percentage, anyone reading the test report can understand if the difference is insignificant, or big enough to require further discussion before accepting the patch. Showing the difference in packets per second requires the reader of the test report to have prior knowledge about the expected performance.
>
> > If the group would prefer, we could calculate additional references if
> > desired (i.e. difference from the last official release, or a monthly
> > run
> > of the current, etc.). We just need the community to define their
> > needs,
> > and we can add this to the development queue.
> >
>
> I retract my suggestion about adding additional references. You clearly explained how your baselining works, and I think it fully serves the needs of a regression test.
>
> So please just put this small change in your development queue: Output the difference in percent with a few decimals after the comma, instead of the difference in packets per second.
>
> For readability, performance drops should be shown as negative values, and increases as positive, e.g.:
>
> Difference = (NewPPS - BaselinePPS) / BaselinePPS * 100.00 %.
>
>
> If we want to compare performance against official releases, current or older, it should go elsewhere, not in the continuous testing environment. E.g. the release notes could include a report showing differences from the last few official releases. But that is a task for another day.
>
>
> > Cheers,
> > Lincoln
> >
> >
> > On Thu, Jan 21, 2021 at 4:29 AM Morten Brørup
> > <mb@smartsharesystems.com>
> > wrote:
> >
> > > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ferruh Yigit
> > > > Sent: Thursday, January 21, 2021 10:19 AM
> > > >
> > > > On 1/15/2021 6:39 PM, Ali Alnubani wrote:
> > > > > Hi,
> > > > > Adding Ferruh and Zhaoyan,
> > > > >
> > > > >> Ali,
> > > > >>
> > > > >> You reported some performance regression, did you confirm it?
> > > > >> If I get no reply by monday, I'll proceed with this patch.
> > > > >
> > > > > Sure I'll confirm by Monday.
> > > > >
> > > > > Doesn't the regression also reproduce on the Lab's Intel servers?
> > > > > Even though the check iol-intel-Performance isn't failing, I can
> > see
> > > > that the throughput differences from expected for this patch are
> > less
> > > > than those of another patch that was tested only 20 minutes
> > earlier.
> > > > Both patches were applied to the same tree:
> > > > >
> > > > > https://mails.dpdk.org/archives/test-report/2021-
> > January/173927.html
> > > > >> | 64 | 512 | 1.571 |
> > > > >
> > > > > https://mails.dpdk.org/archives/test-report/2021-
> > January/173919.html
> > > > >> | 64 | 512 | 2.698 |
> > > > >
> > > > > Assuming that pw86457 doesn't have an effect on this test, it
> > looks
> > > > to me that this patch caused a regression in Intel hardware as
> > well.
> > > > >
> > > > > Can someone update the baseline's expected values for the Intel
> > NICs
> > > > and rerun the test on this patch?
> > > > >
> > > >
> > > > Zhaoyan said that the baseline is calculated dynamically,
> > > > what I understand is baseline set based on previous days
> > performance
> > > > result, so
> > > > it shouldn't require updating.
> > >
> > > That sounds smart!
> > >
> > > Perhaps another reference baseline could be added, for informational
> > > purposes only:
> > > Deviation from the performance of the last official release.
> > >
> > > >
> > > > But cc'ed the lab for more details.
> > >
> > >
> >
> > --
> > *Lincoln Lavoie*
> > Senior Engineer, Broadband Technologies
> > 21 Madbury Rd., Ste. 100, Durham, NH 03824
> > lylavoie@iol.unh.edu
> > https://www.iol.unh.edu
> > +1-603-674-2755 (m)
> > <https://www.iol.unh.edu>
>
On 1/21/2021 4:35 PM, Lincoln Lavoie wrote:
> Hi All,
>
> Trying to follow the specific conversation. It is correct, the lab does not
> list the specific throughput values achieved by the hardware, as that data can
> be sensitive to the hardware vendors, etc. The purpose of the lab is to check
> for degradations caused by patches, so the difference is really the important
> factor. The comparison is against a prior run on the same hardware, via the
> DPDK main branch, so any delta should be caused by the specific patch changes
> (excluding statistical "wiggle").
>
> If the group would prefer, we could calculate additional references if desired
> (i.e. difference from the last official release, or a monthly run of the
> current, etc.). We just need the community to define their needs, and we can
> add this to the development queue.
>
Hi Brandon,
Can you also put above to the backlog, to display the performance difference to
the a fixed point, like a previous release or a previous LTS?
Thanks,
ferruh
> Cheers,
> Lincoln
>
>
> On Thu, Jan 21, 2021 at 4:29 AM Morten Brørup <mb@smartsharesystems.com
> <mailto:mb@smartsharesystems.com>> wrote:
>
> > From: dev [mailto:dev-bounces@dpdk.org <mailto:dev-bounces@dpdk.org>] On
> Behalf Of Ferruh Yigit
> > Sent: Thursday, January 21, 2021 10:19 AM
> >
> > On 1/15/2021 6:39 PM, Ali Alnubani wrote:
> > > Hi,
> > > Adding Ferruh and Zhaoyan,
> > >
> > >> Ali,
> > >>
> > >> You reported some performance regression, did you confirm it?
> > >> If I get no reply by monday, I'll proceed with this patch.
> > >
> > > Sure I'll confirm by Monday.
> > >
> > > Doesn't the regression also reproduce on the Lab's Intel servers?
> > > Even though the check iol-intel-Performance isn't failing, I can see
> > that the throughput differences from expected for this patch are less
> > than those of another patch that was tested only 20 minutes earlier.
> > Both patches were applied to the same tree:
> > >
> > > https://mails.dpdk.org/archives/test-report/2021-January/173927.html
> <https://mails.dpdk.org/archives/test-report/2021-January/173927.html>
> > >> | 64 | 512 | 1.571 |
> > >
> > > https://mails.dpdk.org/archives/test-report/2021-January/173919.html
> <https://mails.dpdk.org/archives/test-report/2021-January/173919.html>
> > >> | 64 | 512 | 2.698 |
> > >
> > > Assuming that pw86457 doesn't have an effect on this test, it looks
> > to me that this patch caused a regression in Intel hardware as well.
> > >
> > > Can someone update the baseline's expected values for the Intel NICs
> > and rerun the test on this patch?
> > >
> >
> > Zhaoyan said that the baseline is calculated dynamically,
> > what I understand is baseline set based on previous days performance
> > result, so
> > it shouldn't require updating.
>
> That sounds smart!
>
> Perhaps another reference baseline could be added, for informational
> purposes only:
> Deviation from the performance of the last official release.
>
> >
> > But cc'ed the lab for more details.
>
>
>
> --
> *Lincoln Lavoie*
> Senior Engineer, Broadband Technologies
> 21 Madbury Rd., Ste. 100, Durham, NH 03824
> lylavoie@iol.unh.edu <mailto:lylavoie@iol.unh.edu>
> https://www.iol.unh.edu <https://www.iol.unh.edu>
> +1-603-674-2755 (m)
> <https://www.iol.unh.edu>
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Olivier Matz
> Sent: Wednesday, 13 January 2021 14.28
[snip]
> Fixes: 8f094a9ac5d7 ("mbuf: set mbuf fields while in pool")
> Cc: stable@dpdk.org
>
> Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
> Acked-by: Morten Brørup <mb@smartsharesystems.com>
> Acked-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
> Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> ---
>
> v4
> * add a unit test (suggested by David)
>
> v3
> * fix commit log again (thanks Morten for spotting it)
>
> v2
> * avoid write access if uneeded (suggested by Konstantin)
> * enhance comments in mbuf header file (suggested by Morten)
> * fix commit log
>
> app/test/test_mbuf.c | 69 +++++++++++++++++++++++++++++++++
> lib/librte_mbuf/rte_mbuf.c | 4 +-
> lib/librte_mbuf/rte_mbuf.h | 8 ++--
> lib/librte_mbuf/rte_mbuf_core.h | 13 ++++++-
> 4 files changed, 86 insertions(+), 8 deletions(-)
>
[snip]
> diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c
> index 7d09ee2939..5f77840557 100644
> --- a/lib/librte_mbuf/rte_mbuf.c
> +++ b/lib/librte_mbuf/rte_mbuf.c
> @@ -129,10 +129,10 @@ rte_pktmbuf_free_pinned_extmem(void *addr, void
> *opaque)
>
> rte_mbuf_ext_refcnt_set(m->shinfo, 1);
> m->ol_flags = EXT_ATTACHED_MBUF;
> - if (m->next != NULL) {
> + if (m->next != NULL)
> m->next = NULL;
> + if (m->nb_segs != 1)
> m->nb_segs = 1;
> - }
> rte_mbuf_raw_free(m);
> }
>
> diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
> index c4c9ebfaa0..8c1097ed76 100644
> --- a/lib/librte_mbuf/rte_mbuf.h
> +++ b/lib/librte_mbuf/rte_mbuf.h
> @@ -1340,10 +1340,10 @@ rte_pktmbuf_prefree_seg(struct rte_mbuf *m)
> return NULL;
> }
>
> - if (m->next != NULL) {
> + if (m->next != NULL)
> m->next = NULL;
> + if (m->nb_segs != 1)
> m->nb_segs = 1;
> - }
>
> return m;
>
> @@ -1357,10 +1357,10 @@ rte_pktmbuf_prefree_seg(struct rte_mbuf *m)
> return NULL;
> }
>
> - if (m->next != NULL) {
> + if (m->next != NULL)
> m->next = NULL;
> + if (m->nb_segs != 1)
> m->nb_segs = 1;
> - }
> rte_mbuf_refcnt_set(m, 1);
>
> return m;
Olivier, have you considered if Konstantin's suggestion (to avoid write access if unneeded) could be applied to any of the other functions in rte_mbuf.h, e.g. __rte_pktmbuf_free_direct()?
What's the follow-up for this patch?
19/01/2021 15:04, Slava Ovsiienko:
> Hi, All
>
> Could we postpose this patch at least to rc2? We would like to conduct more investigations?
>
> With best regards, Slava
>
> From: Olivier Matz <olivier.matz@6wind.com>
> > On Mon, Jan 18, 2021 at 05:52:32PM +0000, Ali Alnubani wrote:
> > > Hi,
> > > (Sorry had to resend this to some recipients due to mail server problems).
> > >
> > > Just confirming that I can still reproduce the regression with single core and
> > 64B frames on other servers.
> >
> > Many thanks for the feedback. Can you please detail what is the amount of
> > performance loss in percent, and confirm the test case? (I suppose it is
> > testpmd io forward).
> >
> > Unfortunatly, I won't be able to spend a lot of time on this soon (sorry for
> > that). So I see at least these 2 options:
> >
> > - postpone the patch again, until I can find more time to analyze
> > and optimize
> > - apply the patch if the performance loss is acceptable compared to
> > the added value of fixing a bug
> >
> > Regards,
> > Olivier
> >
[...]
> > > > Assuming that pw86457 doesn't have an effect on this test, it looks
> > > > to me that this patch caused a regression in Intel hardware as well.
> > > >
> > > > Can someone update the baseline's expected values for the Intel NICs
> > > > and rerun the test on this patch?
> > > >
> > > > Thanks,
> > > > Ali
Hi Thomas,
On Sat, Jul 24, 2021 at 10:47:34AM +0200, Thomas Monjalon wrote:
> What's the follow-up for this patch?
Unfortunatly, I still don't have the time to work on this topic yet.
In my initial tests, in our lab, I didn't notice any performance
regression, but Ali has seen an impact (0.5M PPS, but I don't know how
much in percent).
> 19/01/2021 15:04, Slava Ovsiienko:
> > Hi, All
> >
> > Could we postpose this patch at least to rc2? We would like to conduct more investigations?
> >
> > With best regards, Slava
> >
> > From: Olivier Matz <olivier.matz@6wind.com>
> > > On Mon, Jan 18, 2021 at 05:52:32PM +0000, Ali Alnubani wrote:
> > > > Hi,
> > > > (Sorry had to resend this to some recipients due to mail server problems).
> > > >
> > > > Just confirming that I can still reproduce the regression with single core and
> > > 64B frames on other servers.
> > >
> > > Many thanks for the feedback. Can you please detail what is the amount of
> > > performance loss in percent, and confirm the test case? (I suppose it is
> > > testpmd io forward).
> > >
> > > Unfortunatly, I won't be able to spend a lot of time on this soon (sorry for
> > > that). So I see at least these 2 options:
> > >
> > > - postpone the patch again, until I can find more time to analyze
> > > and optimize
> > > - apply the patch if the performance loss is acceptable compared to
> > > the added value of fixing a bug
> > >
> [...]
Statu quo...
Olivier
> > > > > Assuming that pw86457 doesn't have an effect on this test, it looks
> > > > > to me that this patch caused a regression in Intel hardware as well.
> > > > >
> > > > > Can someone update the baseline's expected values for the Intel NICs
> > > > > and rerun the test on this patch?
> > > > >
> > > > > Thanks,
> > > > > Ali
>
>
>
>
> From: Olivier Matz [mailto:olivier.matz@6wind.com]
> Sent: Friday, 30 July 2021 14.37
>
> Hi Thomas,
>
> On Sat, Jul 24, 2021 at 10:47:34AM +0200, Thomas Monjalon wrote:
> > What's the follow-up for this patch?
>
> Unfortunatly, I still don't have the time to work on this topic yet.
>
> In my initial tests, in our lab, I didn't notice any performance
> regression, but Ali has seen an impact (0.5M PPS, but I don't know how
> much in percent).
>
>
> > 19/01/2021 15:04, Slava Ovsiienko:
> > > Hi, All
> > >
> > > Could we postpose this patch at least to rc2? We would like to
> conduct more investigations?
> > >
> > > With best regards, Slava
> > >
> > > From: Olivier Matz <olivier.matz@6wind.com>
> > > > On Mon, Jan 18, 2021 at 05:52:32PM +0000, Ali Alnubani wrote:
> > > > > Hi,
> > > > > (Sorry had to resend this to some recipients due to mail server
> problems).
> > > > >
> > > > > Just confirming that I can still reproduce the regression with
> single core and
> > > > 64B frames on other servers.
> > > >
> > > > Many thanks for the feedback. Can you please detail what is the
> amount of
> > > > performance loss in percent, and confirm the test case? (I
> suppose it is
> > > > testpmd io forward).
> > > >
> > > > Unfortunatly, I won't be able to spend a lot of time on this soon
> (sorry for
> > > > that). So I see at least these 2 options:
> > > >
> > > > - postpone the patch again, until I can find more time to analyze
> > > > and optimize
> > > > - apply the patch if the performance loss is acceptable compared
> to
> > > > the added value of fixing a bug
> > > >
> > [...]
>
> Statu quo...
>
> Olivier
>
The decision should be simple:
Does the DPDK project support segmented packets?
If yes, then apply the patch to fix the bug!
If anyone seriously cares about the regression it introduces, optimization patches are welcome later. We shouldn't wait for it.
If the patch is not applied, the documentation must be updated to mention that we are releasing DPDK with a known bug: that segmented packets are handled incorrectly in the scenario described in this patch.
Generally, there could be some performance to gain by not supporting segmented packets at all, as a compile time option. But that is a different discussion.
-Morten
30/07/2021 16:35, Morten Brørup:
> > From: Olivier Matz [mailto:olivier.matz@6wind.com]
> > Sent: Friday, 30 July 2021 14.37
> >
> > Hi Thomas,
> >
> > On Sat, Jul 24, 2021 at 10:47:34AM +0200, Thomas Monjalon wrote:
> > > What's the follow-up for this patch?
> >
> > Unfortunatly, I still don't have the time to work on this topic yet.
> >
> > In my initial tests, in our lab, I didn't notice any performance
> > regression, but Ali has seen an impact (0.5M PPS, but I don't know how
> > much in percent).
> >
> >
> > > 19/01/2021 15:04, Slava Ovsiienko:
> > > > Hi, All
> > > >
> > > > Could we postpose this patch at least to rc2? We would like to
> > conduct more investigations?
> > > >
> > > > With best regards, Slava
> > > >
> > > > From: Olivier Matz <olivier.matz@6wind.com>
> > > > > On Mon, Jan 18, 2021 at 05:52:32PM +0000, Ali Alnubani wrote:
> > > > > > Hi,
> > > > > > (Sorry had to resend this to some recipients due to mail server
> > problems).
> > > > > >
> > > > > > Just confirming that I can still reproduce the regression with
> > single core and
> > > > > 64B frames on other servers.
> > > > >
> > > > > Many thanks for the feedback. Can you please detail what is the
> > amount of
> > > > > performance loss in percent, and confirm the test case? (I
> > suppose it is
> > > > > testpmd io forward).
> > > > >
> > > > > Unfortunatly, I won't be able to spend a lot of time on this soon
> > (sorry for
> > > > > that). So I see at least these 2 options:
> > > > >
> > > > > - postpone the patch again, until I can find more time to analyze
> > > > > and optimize
> > > > > - apply the patch if the performance loss is acceptable compared
> > to
> > > > > the added value of fixing a bug
> > > > >
> > > [...]
> >
> > Statu quo...
> >
> > Olivier
> >
>
> The decision should be simple:
>
> Does the DPDK project support segmented packets?
> If yes, then apply the patch to fix the bug!
>
> If anyone seriously cares about the regression it introduces, optimization patches are welcome later. We shouldn't wait for it.
You're right, but the regression is flagged to a 4-years old patch,
that's why I don't consider it as urgent.
> If the patch is not applied, the documentation must be updated to mention that we are releasing DPDK with a known bug: that segmented packets are handled incorrectly in the scenario described in this patch.
Yes, would be good to document the known issue,
no matter how old it is.
> Generally, there could be some performance to gain by not supporting segmented packets at all, as a compile time option. But that is a different discussion.
>
>
> -Morten
Hi,
On Fri, Jul 30, 2021 at 04:54:05PM +0200, Thomas Monjalon wrote:
> 30/07/2021 16:35, Morten Brørup:
> > > From: Olivier Matz [mailto:olivier.matz@6wind.com]
> > > Sent: Friday, 30 July 2021 14.37
> > >
> > > Hi Thomas,
> > >
> > > On Sat, Jul 24, 2021 at 10:47:34AM +0200, Thomas Monjalon wrote:
> > > > What's the follow-up for this patch?
> > >
> > > Unfortunatly, I still don't have the time to work on this topic yet.
> > >
> > > In my initial tests, in our lab, I didn't notice any performance
> > > regression, but Ali has seen an impact (0.5M PPS, but I don't know how
> > > much in percent).
> > >
> > >
> > > > 19/01/2021 15:04, Slava Ovsiienko:
> > > > > Hi, All
> > > > >
> > > > > Could we postpose this patch at least to rc2? We would like to
> > > conduct more investigations?
> > > > >
> > > > > With best regards, Slava
> > > > >
> > > > > From: Olivier Matz <olivier.matz@6wind.com>
> > > > > > On Mon, Jan 18, 2021 at 05:52:32PM +0000, Ali Alnubani wrote:
> > > > > > > Hi,
> > > > > > > (Sorry had to resend this to some recipients due to mail server
> > > problems).
> > > > > > >
> > > > > > > Just confirming that I can still reproduce the regression with
> > > single core and
> > > > > > 64B frames on other servers.
> > > > > >
> > > > > > Many thanks for the feedback. Can you please detail what is the
> > > amount of
> > > > > > performance loss in percent, and confirm the test case? (I
> > > suppose it is
> > > > > > testpmd io forward).
> > > > > >
> > > > > > Unfortunatly, I won't be able to spend a lot of time on this soon
> > > (sorry for
> > > > > > that). So I see at least these 2 options:
> > > > > >
> > > > > > - postpone the patch again, until I can find more time to analyze
> > > > > > and optimize
> > > > > > - apply the patch if the performance loss is acceptable compared
> > > to
> > > > > > the added value of fixing a bug
> > > > > >
> > > > [...]
> > >
> > > Statu quo...
> > >
> > > Olivier
> > >
> >
> > The decision should be simple:
> >
> > Does the DPDK project support segmented packets?
> > If yes, then apply the patch to fix the bug!
> >
> > If anyone seriously cares about the regression it introduces, optimization patches are welcome later. We shouldn't wait for it.
>
> You're right, but the regression is flagged to a 4-years old patch,
> that's why I don't consider it as urgent.
>
> > If the patch is not applied, the documentation must be updated to mention that we are releasing DPDK with a known bug: that segmented packets are handled incorrectly in the scenario described in this patch.
>
> Yes, would be good to document the known issue,
> no matter how old it is.
The problem description could be something like this:
It is expected that free mbufs have their field m->nb_seg set to 1, so
that when it is allocated, the user does not need to set its
value. The mbuf free functions are responsible of resetting this field
to 1 before returning the mbuf to the pool.
When a multi-segment mbuf is freed, the m->nb_seg field is not reset
to 1 for the last segment of the chain. On next allocation of this
segment, if the field is not explicitly reset by the user, an invalid
mbuf can be created, and can cause an undefined behavior.
> > Generally, there could be some performance to gain by not supporting segmented packets at all, as a compile time option. But that is a different discussion.
> >
> >
> > -Morten
>
>
>
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Olivier Matz
> Sent: Friday, 30 July 2021 17.15
>
> Hi,
>
> On Fri, Jul 30, 2021 at 04:54:05PM +0200, Thomas Monjalon wrote:
> > 30/07/2021 16:35, Morten Brørup:
> > > > From: Olivier Matz [mailto:olivier.matz@6wind.com]
> > > > Sent: Friday, 30 July 2021 14.37
> > > >
> > > > Hi Thomas,
> > > >
> > > > On Sat, Jul 24, 2021 at 10:47:34AM +0200, Thomas Monjalon wrote:
> > > > > What's the follow-up for this patch?
> > > >
> > > > Unfortunatly, I still don't have the time to work on this topic
> yet.
> > > >
> > > > In my initial tests, in our lab, I didn't notice any performance
> > > > regression, but Ali has seen an impact (0.5M PPS, but I don't
> know how
> > > > much in percent).
> > > >
> > > >
> > > > > 19/01/2021 15:04, Slava Ovsiienko:
> > > > > > Hi, All
> > > > > >
> > > > > > Could we postpose this patch at least to rc2? We would like
> to
> > > > conduct more investigations?
> > > > > >
> > > > > > With best regards, Slava
> > > > > >
> > > > > > From: Olivier Matz <olivier.matz@6wind.com>
> > > > > > > On Mon, Jan 18, 2021 at 05:52:32PM +0000, Ali Alnubani
> wrote:
> > > > > > > > Hi,
> > > > > > > > (Sorry had to resend this to some recipients due to mail
> server
> > > > problems).
> > > > > > > >
> > > > > > > > Just confirming that I can still reproduce the regression
> with
> > > > single core and
> > > > > > > 64B frames on other servers.
> > > > > > >
> > > > > > > Many thanks for the feedback. Can you please detail what is
> the
> > > > amount of
> > > > > > > performance loss in percent, and confirm the test case? (I
> > > > suppose it is
> > > > > > > testpmd io forward).
> > > > > > >
> > > > > > > Unfortunatly, I won't be able to spend a lot of time on
> this soon
> > > > (sorry for
> > > > > > > that). So I see at least these 2 options:
> > > > > > >
> > > > > > > - postpone the patch again, until I can find more time to
> analyze
> > > > > > > and optimize
> > > > > > > - apply the patch if the performance loss is acceptable
> compared
> > > > to
> > > > > > > the added value of fixing a bug
> > > > > > >
> > > > > [...]
> > > >
> > > > Statu quo...
> > > >
> > > > Olivier
> > > >
> > >
> > > The decision should be simple:
> > >
> > > Does the DPDK project support segmented packets?
> > > If yes, then apply the patch to fix the bug!
> > >
> > > If anyone seriously cares about the regression it introduces,
> optimization patches are welcome later. We shouldn't wait for it.
> >
> > You're right, but the regression is flagged to a 4-years old patch,
> > that's why I don't consider it as urgent.
> >
> > > If the patch is not applied, the documentation must be updated to
> mention that we are releasing DPDK with a known bug: that segmented
> packets are handled incorrectly in the scenario described in this
> patch.
> >
> > Yes, would be good to document the known issue,
> > no matter how old it is.
>
> The problem description could be something like this:
>
> It is expected that free mbufs have their field m->nb_seg set to 1,
> so
> that when it is allocated, the user does not need to set its
> value. The mbuf free functions are responsible of resetting this
> field
> to 1 before returning the mbuf to the pool.
>
> When a multi-segment mbuf is freed, the m->nb_seg field is not reset
> to 1 for the last segment of the chain. On next allocation of this
> segment, if the field is not explicitly reset by the user, an invalid
> mbuf can be created, and can cause an undefined behavior.
>
And it needs to be put somewhere very prominent if we expect the users to read it.
Would adding an RTE_VERIFY() - instead of fixing the bug - cause a regression? If not, then any affected user will know what went wrong and where. This would still be an improvement, if the bugfix patch cannot be applied.
>
> > > Generally, there could be some performance to gain by not
> supporting segmented packets at all, as a compile time option. But that
> is a different discussion.
> > >
> > >
> > > -Morten
> >
> >
> >
Follow-up again:
We have added a note in 21.08, we should fix it in 21.11.
If there are no counter proposal, I suggest applying this patch,
no matter the performance regression.
30/07/2021 16:54, Thomas Monjalon:
> 30/07/2021 16:35, Morten Brørup:
> > > From: Olivier Matz [mailto:olivier.matz@6wind.com]
> > > Sent: Friday, 30 July 2021 14.37
> > >
> > > Hi Thomas,
> > >
> > > On Sat, Jul 24, 2021 at 10:47:34AM +0200, Thomas Monjalon wrote:
> > > > What's the follow-up for this patch?
> > >
> > > Unfortunatly, I still don't have the time to work on this topic yet.
> > >
> > > In my initial tests, in our lab, I didn't notice any performance
> > > regression, but Ali has seen an impact (0.5M PPS, but I don't know how
> > > much in percent).
> > >
> > >
> > > > 19/01/2021 15:04, Slava Ovsiienko:
> > > > > Hi, All
> > > > >
> > > > > Could we postpose this patch at least to rc2? We would like to
> > > conduct more investigations?
> > > > >
> > > > > With best regards, Slava
> > > > >
> > > > > From: Olivier Matz <olivier.matz@6wind.com>
> > > > > > On Mon, Jan 18, 2021 at 05:52:32PM +0000, Ali Alnubani wrote:
> > > > > > > Hi,
> > > > > > > (Sorry had to resend this to some recipients due to mail server
> > > problems).
> > > > > > >
> > > > > > > Just confirming that I can still reproduce the regression with
> > > single core and
> > > > > > 64B frames on other servers.
> > > > > >
> > > > > > Many thanks for the feedback. Can you please detail what is the
> > > amount of
> > > > > > performance loss in percent, and confirm the test case? (I
> > > suppose it is
> > > > > > testpmd io forward).
> > > > > >
> > > > > > Unfortunatly, I won't be able to spend a lot of time on this soon
> > > (sorry for
> > > > > > that). So I see at least these 2 options:
> > > > > >
> > > > > > - postpone the patch again, until I can find more time to analyze
> > > > > > and optimize
> > > > > > - apply the patch if the performance loss is acceptable compared
> > > to
> > > > > > the added value of fixing a bug
> > > > > >
> > > > [...]
> > >
> > > Statu quo...
> > >
> > > Olivier
> > >
> >
> > The decision should be simple:
> >
> > Does the DPDK project support segmented packets?
> > If yes, then apply the patch to fix the bug!
> >
> > If anyone seriously cares about the regression it introduces, optimization patches are welcome later. We shouldn't wait for it.
>
> You're right, but the regression is flagged to a 4-years old patch,
> that's why I don't consider it as urgent.
>
> > If the patch is not applied, the documentation must be updated to mention that we are releasing DPDK with a known bug: that segmented packets are handled incorrectly in the scenario described in this patch.
>
> Yes, would be good to document the known issue,
> no matter how old it is.
>
> > Generally, there could be some performance to gain by not supporting segmented packets at all, as a compile time option. But that is a different discussion.
Hi,
I've re-read the entire thread.
If I understand correctly, the root problem was (in initial patch):
> m1 = rte_pktmbuf_alloc(mp);
> rte_pktmbuf_append(m1, 500);
> m2 = rte_pktmbuf_alloc(mp);
> rte_pktmbuf_append(m2, 500);
> rte_pktmbuf_chain(m1, m2);
> m0 = rte_pktmbuf_alloc(mp);
> rte_pktmbuf_append(m0, 500);
> rte_pktmbuf_chain(m0, m1);
>
> As rte_pktmbuf_chain() does not reset nb_seg in the initial m1 segment
> (this is not required), after this code the mbuf chain have 3
> segments:
> - m0: next=m1, nb_seg=3
> - m1: next=m2, nb_seg=2
> - m2: next=NULL, nb_seg=1
>
The proposed fix was to ALWAYS set next and nb_seg fields on mbuf_free(),
regardless next field content. That would perform unconditional write
to mbuf, and might affect the configurations, where are no multi-segment
packets at al. mbuf_free() is "backbone" API, it is used by all cases, all
scenaries are affected.
As far as I know, the current approach for nb_seg field - it contains other
value than 1 only in the first mbuf , for the following segments, it should
not be considered at all (only the first segment fields are valid), and it is
supposed to contain 1, as it was initially allocated from the pool.
In the example above the problem was introduced by
rte_pktmbuf_chain(). Could we consider fixing the rte_pktmbuf_chain()
(used in potentially fewer common sceneries) instead of touching
the extremely common rte_mbuf_free() ?
With best regards,
Slava
> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Tuesday, September 28, 2021 11:29
> To: Olivier Matz <olivier.matz@6wind.com>; Ali Alnubani
> <alialnu@nvidia.com>; Slava Ovsiienko <viacheslavo@nvidia.com>
> Cc: Morten Brørup <mb@smartsharesystems.com>; dev@dpdk.org; David
> Marchand <david.marchand@redhat.com>; Alexander Kozyrev
> <akozyrev@nvidia.com>; Ferruh Yigit <ferruh.yigit@intel.com>;
> zhaoyan.chen@intel.com; Andrew Rybchenko
> <andrew.rybchenko@oktetlabs.ru>; Ananyev, Konstantin
> <konstantin.ananyev@intel.com>; Ajit Khaparde
> <ajit.khaparde@broadcom.com>; jerinj@marvell.com
> Subject: Re: [dpdk-dev] [dpdk-stable] [PATCH v4] mbuf: fix reset on mbuf
> free
>
> Follow-up again:
> We have added a note in 21.08, we should fix it in 21.11.
> If there are no counter proposal, I suggest applying this patch, no matter the
> performance regression.
>
>
> 30/07/2021 16:54, Thomas Monjalon:
> > 30/07/2021 16:35, Morten Brørup:
> > > > From: Olivier Matz [mailto:olivier.matz@6wind.com]
> > > > Sent: Friday, 30 July 2021 14.37
> > > >
> > > > Hi Thomas,
> > > >
> > > > On Sat, Jul 24, 2021 at 10:47:34AM +0200, Thomas Monjalon wrote:
> > > > > What's the follow-up for this patch?
> > > >
> > > > Unfortunatly, I still don't have the time to work on this topic yet.
> > > >
> > > > In my initial tests, in our lab, I didn't notice any performance
> > > > regression, but Ali has seen an impact (0.5M PPS, but I don't know
> > > > how much in percent).
> > > >
> > > >
> > > > > 19/01/2021 15:04, Slava Ovsiienko:
> > > > > > Hi, All
> > > > > >
> > > > > > Could we postpose this patch at least to rc2? We would like to
> > > > conduct more investigations?
> > > > > >
> > > > > > With best regards, Slava
> > > > > >
> > > > > > From: Olivier Matz <olivier.matz@6wind.com>
> > > > > > > On Mon, Jan 18, 2021 at 05:52:32PM +0000, Ali Alnubani wrote:
> > > > > > > > Hi,
> > > > > > > > (Sorry had to resend this to some recipients due to mail
> > > > > > > > server
> > > > problems).
> > > > > > > >
> > > > > > > > Just confirming that I can still reproduce the regression
> > > > > > > > with
> > > > single core and
> > > > > > > 64B frames on other servers.
> > > > > > >
> > > > > > > Many thanks for the feedback. Can you please detail what is
> > > > > > > the
> > > > amount of
> > > > > > > performance loss in percent, and confirm the test case? (I
> > > > suppose it is
> > > > > > > testpmd io forward).
> > > > > > >
> > > > > > > Unfortunatly, I won't be able to spend a lot of time on this
> > > > > > > soon
> > > > (sorry for
> > > > > > > that). So I see at least these 2 options:
> > > > > > >
> > > > > > > - postpone the patch again, until I can find more time to analyze
> > > > > > > and optimize
> > > > > > > - apply the patch if the performance loss is acceptable
> > > > > > > compared
> > > > to
> > > > > > > the added value of fixing a bug
> > > > > > >
> > > > > [...]
> > > >
> > > > Statu quo...
> > > >
> > > > Olivier
> > > >
> > >
> > > The decision should be simple:
> > >
> > > Does the DPDK project support segmented packets?
> > > If yes, then apply the patch to fix the bug!
> > >
> > > If anyone seriously cares about the regression it introduces, optimization
> patches are welcome later. We shouldn't wait for it.
> >
> > You're right, but the regression is flagged to a 4-years old patch,
> > that's why I don't consider it as urgent.
> >
> > > If the patch is not applied, the documentation must be updated to
> mention that we are releasing DPDK with a known bug: that segmented
> packets are handled incorrectly in the scenario described in this patch.
> >
> > Yes, would be good to document the known issue, no matter how old it
> > is.
> >
> > > Generally, there could be some performance to gain by not supporting
> segmented packets at all, as a compile time option. But that is a different
> discussion.
>
>
>
>
> Hi,
>
> I've re-read the entire thread.
> If I understand correctly, the root problem was (in initial patch):
>
> > m1 = rte_pktmbuf_alloc(mp);
> > rte_pktmbuf_append(m1, 500);
> > m2 = rte_pktmbuf_alloc(mp);
> > rte_pktmbuf_append(m2, 500);
> > rte_pktmbuf_chain(m1, m2);
> > m0 = rte_pktmbuf_alloc(mp);
> > rte_pktmbuf_append(m0, 500);
> > rte_pktmbuf_chain(m0, m1);
> >
> > As rte_pktmbuf_chain() does not reset nb_seg in the initial m1 segment
> > (this is not required), after this code the mbuf chain have 3
> > segments:
> > - m0: next=m1, nb_seg=3
> > - m1: next=m2, nb_seg=2
> > - m2: next=NULL, nb_seg=1
> >
> The proposed fix was to ALWAYS set next and nb_seg fields on mbuf_free(),
> regardless next field content. That would perform unconditional write
> to mbuf,
I don't think it is a correct understanding see below.
Current code:
if (m->next != NULL) {
m->next = NULL;
m->nb_segs = 1;
}
Proposed code:
if (m->next != NULL)
m->next = NULL;
if (m->nb_segs != 1)
m->nb_segs = 1;
So what this patch adds: one more load and compare.
Note that load is from the first mbuf cache line, which
already has to be in the L1 cache by that time.
As I remember the reported slowdown is really tiny.
My vote would be to go ahead with this patch.
> and might affect the configurations, where are no multi-segment
> packets at al. mbuf_free() is "backbone" API, it is used by all cases, all
> scenaries are affected.
>
> As far as I know, the current approach for nb_seg field - it contains other
> value than 1 only in the first mbuf , for the following segments, it should
> not be considered at all (only the first segment fields are valid), and it is
> supposed to contain 1, as it was initially allocated from the pool.
>
> In the example above the problem was introduced by
> rte_pktmbuf_chain(). Could we consider fixing the rte_pktmbuf_chain()
> (used in potentially fewer common sceneries) instead of touching
> the extremely common rte_mbuf_free() ?
>
> With best regards,
> Slava
>
> > -----Original Message-----
> > From: Thomas Monjalon <thomas@monjalon.net>
> > Sent: Tuesday, September 28, 2021 11:29
> > To: Olivier Matz <olivier.matz@6wind.com>; Ali Alnubani
> > <alialnu@nvidia.com>; Slava Ovsiienko <viacheslavo@nvidia.com>
> > Cc: Morten Brørup <mb@smartsharesystems.com>; dev@dpdk.org; David
> > Marchand <david.marchand@redhat.com>; Alexander Kozyrev
> > <akozyrev@nvidia.com>; Ferruh Yigit <ferruh.yigit@intel.com>;
> > zhaoyan.chen@intel.com; Andrew Rybchenko
> > <andrew.rybchenko@oktetlabs.ru>; Ananyev, Konstantin
> > <konstantin.ananyev@intel.com>; Ajit Khaparde
> > <ajit.khaparde@broadcom.com>; jerinj@marvell.com
> > Subject: Re: [dpdk-dev] [dpdk-stable] [PATCH v4] mbuf: fix reset on mbuf
> > free
> >
> > Follow-up again:
> > We have added a note in 21.08, we should fix it in 21.11.
> > If there are no counter proposal, I suggest applying this patch, no matter the
> > performance regression.
> >
> >
> > 30/07/2021 16:54, Thomas Monjalon:
> > > 30/07/2021 16:35, Morten Brørup:
> > > > > From: Olivier Matz [mailto:olivier.matz@6wind.com]
> > > > > Sent: Friday, 30 July 2021 14.37
> > > > >
> > > > > Hi Thomas,
> > > > >
> > > > > On Sat, Jul 24, 2021 at 10:47:34AM +0200, Thomas Monjalon wrote:
> > > > > > What's the follow-up for this patch?
> > > > >
> > > > > Unfortunatly, I still don't have the time to work on this topic yet.
> > > > >
> > > > > In my initial tests, in our lab, I didn't notice any performance
> > > > > regression, but Ali has seen an impact (0.5M PPS, but I don't know
> > > > > how much in percent).
> > > > >
> > > > >
> > > > > > 19/01/2021 15:04, Slava Ovsiienko:
> > > > > > > Hi, All
> > > > > > >
> > > > > > > Could we postpose this patch at least to rc2? We would like to
> > > > > conduct more investigations?
> > > > > > >
> > > > > > > With best regards, Slava
> > > > > > >
> > > > > > > From: Olivier Matz <olivier.matz@6wind.com>
> > > > > > > > On Mon, Jan 18, 2021 at 05:52:32PM +0000, Ali Alnubani wrote:
> > > > > > > > > Hi,
> > > > > > > > > (Sorry had to resend this to some recipients due to mail
> > > > > > > > > server
> > > > > problems).
> > > > > > > > >
> > > > > > > > > Just confirming that I can still reproduce the regression
> > > > > > > > > with
> > > > > single core and
> > > > > > > > 64B frames on other servers.
> > > > > > > >
> > > > > > > > Many thanks for the feedback. Can you please detail what is
> > > > > > > > the
> > > > > amount of
> > > > > > > > performance loss in percent, and confirm the test case? (I
> > > > > suppose it is
> > > > > > > > testpmd io forward).
> > > > > > > >
> > > > > > > > Unfortunatly, I won't be able to spend a lot of time on this
> > > > > > > > soon
> > > > > (sorry for
> > > > > > > > that). So I see at least these 2 options:
> > > > > > > >
> > > > > > > > - postpone the patch again, until I can find more time to analyze
> > > > > > > > and optimize
> > > > > > > > - apply the patch if the performance loss is acceptable
> > > > > > > > compared
> > > > > to
> > > > > > > > the added value of fixing a bug
> > > > > > > >
> > > > > > [...]
> > > > >
> > > > > Statu quo...
> > > > >
> > > > > Olivier
> > > > >
> > > >
> > > > The decision should be simple:
> > > >
> > > > Does the DPDK project support segmented packets?
> > > > If yes, then apply the patch to fix the bug!
> > > >
> > > > If anyone seriously cares about the regression it introduces, optimization
> > patches are welcome later. We shouldn't wait for it.
> > >
> > > You're right, but the regression is flagged to a 4-years old patch,
> > > that's why I don't consider it as urgent.
> > >
> > > > If the patch is not applied, the documentation must be updated to
> > mention that we are releasing DPDK with a known bug: that segmented
> > packets are handled incorrectly in the scenario described in this patch.
> > >
> > > Yes, would be good to document the known issue, no matter how old it
> > > is.
> > >
> > > > Generally, there could be some performance to gain by not supporting
> > segmented packets at all, as a compile time option. But that is a different
> > discussion.
> >
> >
> >
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Slava Ovsiienko
> Sent: Tuesday, 28 September 2021 11.01
>
> Hi,
>
> I've re-read the entire thread.
> If I understand correctly, the root problem was (in initial patch):
>
> > m1 = rte_pktmbuf_alloc(mp);
> > rte_pktmbuf_append(m1, 500);
> > m2 = rte_pktmbuf_alloc(mp);
> > rte_pktmbuf_append(m2, 500);
> > rte_pktmbuf_chain(m1, m2);
> > m0 = rte_pktmbuf_alloc(mp);
> > rte_pktmbuf_append(m0, 500);
> > rte_pktmbuf_chain(m0, m1);
> >
> > As rte_pktmbuf_chain() does not reset nb_seg in the initial m1
> segment
> > (this is not required), after this code the mbuf chain have 3
> > segments:
> > - m0: next=m1, nb_seg=3
> > - m1: next=m2, nb_seg=2
> > - m2: next=NULL, nb_seg=1
> >
> The proposed fix was to ALWAYS set next and nb_seg fields on
> mbuf_free(),
> regardless next field content. That would perform unconditional write
> to mbuf, and might affect the configurations, where are no multi-
> segment
> packets at al. mbuf_free() is "backbone" API, it is used by all cases,
> all
> scenaries are affected.
>
> As far as I know, the current approach for nb_seg field - it contains
> other
> value than 1 only in the first mbuf , for the following segments, it
> should
> not be considered at all (only the first segment fields are valid), and
> it is
> supposed to contain 1, as it was initially allocated from the pool.
>
> In the example above the problem was introduced by
> rte_pktmbuf_chain(). Could we consider fixing the rte_pktmbuf_chain()
> (used in potentially fewer common sceneries) instead of touching
> the extremely common rte_mbuf_free() ?
>
> With best regards,
> Slava
Great idea, Slava!
Changing the invariant for 'nb_segs', so it must be 1, except in the first segment of a segmented packet.
Thinking further about it, perhaps we can achieve even higher performance by a minor additional modification: Use 0 instead of 1? Or offset 'nb_segs' by -1, so it reflects the number of additional segments?
And perhaps combining the invariants for 'nb_segs' and 'next' could provide even more performance improvements. I don't know, just sharing a thought.
Anyway, I vote for fixing the bug. One way or the other!
-Morten
>
> > -----Original Message-----
> > From: Thomas Monjalon <thomas@monjalon.net>
> > Sent: Tuesday, September 28, 2021 11:29
> >
> > Follow-up again:
> > We have added a note in 21.08, we should fix it in 21.11.
> > If there are no counter proposal, I suggest applying this patch, no
> matter the
> > performance regression.
> >
> >
> > 30/07/2021 16:54, Thomas Monjalon:
> > > 30/07/2021 16:35, Morten Brørup:
> > > > > From: Olivier Matz [mailto:olivier.matz@6wind.com]
> > > > > Sent: Friday, 30 July 2021 14.37
> > > > >
> > > > > Hi Thomas,
> > > > >
> > > > > On Sat, Jul 24, 2021 at 10:47:34AM +0200, Thomas Monjalon
> wrote:
> > > > > > What's the follow-up for this patch?
> > > > >
> > > > > Unfortunatly, I still don't have the time to work on this topic
> yet.
> > > > >
> > > > > In my initial tests, in our lab, I didn't notice any
> performance
> > > > > regression, but Ali has seen an impact (0.5M PPS, but I don't
> know
> > > > > how much in percent).
> > > > >
> > > > >
> > > > > > 19/01/2021 15:04, Slava Ovsiienko:
> > > > > > > Hi, All
> > > > > > >
> > > > > > > Could we postpose this patch at least to rc2? We would like
> to
> > > > > conduct more investigations?
> > > > > > >
> > > > > > > With best regards, Slava
> > > > > > >
> > > > > > > From: Olivier Matz <olivier.matz@6wind.com>
> > > > > > > > On Mon, Jan 18, 2021 at 05:52:32PM +0000, Ali Alnubani
> wrote:
> > > > > > > > > Hi,
> > > > > > > > > (Sorry had to resend this to some recipients due to
> mail
> > > > > > > > > server
> > > > > problems).
> > > > > > > > >
> > > > > > > > > Just confirming that I can still reproduce the
> regression
> > > > > > > > > with
> > > > > single core and
> > > > > > > > 64B frames on other servers.
> > > > > > > >
> > > > > > > > Many thanks for the feedback. Can you please detail what
> is
> > > > > > > > the
> > > > > amount of
> > > > > > > > performance loss in percent, and confirm the test case?
> (I
> > > > > suppose it is
> > > > > > > > testpmd io forward).
> > > > > > > >
> > > > > > > > Unfortunatly, I won't be able to spend a lot of time on
> this
> > > > > > > > soon
> > > > > (sorry for
> > > > > > > > that). So I see at least these 2 options:
> > > > > > > >
> > > > > > > > - postpone the patch again, until I can find more time to
> analyze
> > > > > > > > and optimize
> > > > > > > > - apply the patch if the performance loss is acceptable
> > > > > > > > compared
> > > > > to
> > > > > > > > the added value of fixing a bug
> > > > > > > >
> > > > > > [...]
> > > > >
> > > > > Statu quo...
> > > > >
> > > > > Olivier
> > > > >
> > > >
> > > > The decision should be simple:
> > > >
> > > > Does the DPDK project support segmented packets?
> > > > If yes, then apply the patch to fix the bug!
> > > >
> > > > If anyone seriously cares about the regression it introduces,
> optimization
> > patches are welcome later. We shouldn't wait for it.
> > >
> > > You're right, but the regression is flagged to a 4-years old patch,
> > > that's why I don't consider it as urgent.
> > >
> > > > If the patch is not applied, the documentation must be updated to
> > mention that we are releasing DPDK with a known bug: that segmented
> > packets are handled incorrectly in the scenario described in this
> patch.
> > >
> > > Yes, would be good to document the known issue, no matter how old
> it
> > > is.
> > >
> > > > Generally, there could be some performance to gain by not
> supporting
> > segmented packets at all, as a compile time option. But that is a
> different
> > discussion.
> >
> >
> >
>
Hi Olivier,
I wanted to retest the patch on latest main, but it no longer applies, could you please rebase it?
Thanks,
Ali
> -----Original Message-----
> From: Morten Brørup <mb@smartsharesystems.com>
> Sent: Tuesday, September 28, 2021 12:40 PM
> To: Slava Ovsiienko <viacheslavo@nvidia.com>; NBU-Contact-Thomas
> Monjalon <thomas@monjalon.net>; Olivier Matz <olivier.matz@6wind.com>;
> Ali Alnubani <alialnu@nvidia.com>
> Cc: dev@dpdk.org; David Marchand <david.marchand@redhat.com>; Alexander
> Kozyrev <akozyrev@nvidia.com>; Ferruh Yigit <ferruh.yigit@intel.com>;
> zhaoyan.chen@intel.com; Andrew Rybchenko
> <andrew.rybchenko@oktetlabs.ru>; Ananyev, Konstantin
> <konstantin.ananyev@intel.com>; Ajit Khaparde
> <ajit.khaparde@broadcom.com>; jerinj@marvell.com
> Subject: RE: [dpdk-dev] [dpdk-stable] [PATCH v4] mbuf: fix reset on mbuf free
>
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Slava Ovsiienko
> > Sent: Tuesday, 28 September 2021 11.01
> >
> > Hi,
> >
> > I've re-read the entire thread.
> > If I understand correctly, the root problem was (in initial patch):
> >
> > > m1 = rte_pktmbuf_alloc(mp);
> > > rte_pktmbuf_append(m1, 500);
> > > m2 = rte_pktmbuf_alloc(mp);
> > > rte_pktmbuf_append(m2, 500);
> > > rte_pktmbuf_chain(m1, m2);
> > > m0 = rte_pktmbuf_alloc(mp);
> > > rte_pktmbuf_append(m0, 500);
> > > rte_pktmbuf_chain(m0, m1);
> > >
> > > As rte_pktmbuf_chain() does not reset nb_seg in the initial m1
> > segment
> > > (this is not required), after this code the mbuf chain have 3
> > > segments:
> > > - m0: next=m1, nb_seg=3
> > > - m1: next=m2, nb_seg=2
> > > - m2: next=NULL, nb_seg=1
> > >
> > The proposed fix was to ALWAYS set next and nb_seg fields on
> > mbuf_free(), regardless next field content. That would perform
> > unconditional write to mbuf, and might affect the configurations,
> > where are no multi- segment packets at al. mbuf_free() is "backbone"
> > API, it is used by all cases, all scenaries are affected.
> >
> > As far as I know, the current approach for nb_seg field - it contains
> > other value than 1 only in the first mbuf , for the following
> > segments, it should not be considered at all (only the first segment
> > fields are valid), and it is supposed to contain 1, as it was
> > initially allocated from the pool.
> >
> > In the example above the problem was introduced by
> > rte_pktmbuf_chain(). Could we consider fixing the rte_pktmbuf_chain()
> > (used in potentially fewer common sceneries) instead of touching the
> > extremely common rte_mbuf_free() ?
> >
> > With best regards,
> > Slava
>
> Great idea, Slava!
>
> Changing the invariant for 'nb_segs', so it must be 1, except in the first segment
> of a segmented packet.
>
> Thinking further about it, perhaps we can achieve even higher performance by a
> minor additional modification: Use 0 instead of 1? Or offset 'nb_segs' by -1, so it
> reflects the number of additional segments?
>
> And perhaps combining the invariants for 'nb_segs' and 'next' could provide even
> more performance improvements. I don't know, just sharing a thought.
>
> Anyway, I vote for fixing the bug. One way or the other!
>
> -Morten
>
> >
> > > -----Original Message-----
> > > From: Thomas Monjalon <thomas@monjalon.net>
> > > Sent: Tuesday, September 28, 2021 11:29
> > >
> > > Follow-up again:
> > > We have added a note in 21.08, we should fix it in 21.11.
> > > If there are no counter proposal, I suggest applying this patch, no
> > matter the
> > > performance regression.
> > >
> > >
> > > 30/07/2021 16:54, Thomas Monjalon:
> > > > 30/07/2021 16:35, Morten Brørup:
> > > > > > From: Olivier Matz [mailto:olivier.matz@6wind.com]
> > > > > > Sent: Friday, 30 July 2021 14.37
> > > > > >
> > > > > > Hi Thomas,
> > > > > >
> > > > > > On Sat, Jul 24, 2021 at 10:47:34AM +0200, Thomas Monjalon
> > wrote:
> > > > > > > What's the follow-up for this patch?
> > > > > >
> > > > > > Unfortunatly, I still don't have the time to work on this
> > > > > > topic
> > yet.
> > > > > >
> > > > > > In my initial tests, in our lab, I didn't notice any
> > performance
> > > > > > regression, but Ali has seen an impact (0.5M PPS, but I don't
> > know
> > > > > > how much in percent).
> > > > > >
> > > > > >
> > > > > > > 19/01/2021 15:04, Slava Ovsiienko:
> > > > > > > > Hi, All
> > > > > > > >
> > > > > > > > Could we postpose this patch at least to rc2? We would
> > > > > > > > like
> > to
> > > > > > conduct more investigations?
> > > > > > > >
> > > > > > > > With best regards, Slava
> > > > > > > >
> > > > > > > > From: Olivier Matz <olivier.matz@6wind.com>
> > > > > > > > > On Mon, Jan 18, 2021 at 05:52:32PM +0000, Ali Alnubani
> > wrote:
> > > > > > > > > > Hi,
> > > > > > > > > > (Sorry had to resend this to some recipients due to
> > mail
> > > > > > > > > > server
> > > > > > problems).
> > > > > > > > > >
> > > > > > > > > > Just confirming that I can still reproduce the
> > regression
> > > > > > > > > > with
> > > > > > single core and
> > > > > > > > > 64B frames on other servers.
> > > > > > > > >
> > > > > > > > > Many thanks for the feedback. Can you please detail what
> > is
> > > > > > > > > the
> > > > > > amount of
> > > > > > > > > performance loss in percent, and confirm the test case?
> > (I
> > > > > > suppose it is
> > > > > > > > > testpmd io forward).
> > > > > > > > >
> > > > > > > > > Unfortunatly, I won't be able to spend a lot of time on
> > this
> > > > > > > > > soon
> > > > > > (sorry for
> > > > > > > > > that). So I see at least these 2 options:
> > > > > > > > >
> > > > > > > > > - postpone the patch again, until I can find more time
> > > > > > > > > to
> > analyze
> > > > > > > > > and optimize
> > > > > > > > > - apply the patch if the performance loss is acceptable
> > > > > > > > > compared
> > > > > > to
> > > > > > > > > the added value of fixing a bug
> > > > > > > > >
> > > > > > > [...]
> > > > > >
> > > > > > Statu quo...
> > > > > >
> > > > > > Olivier
> > > > > >
> > > > >
> > > > > The decision should be simple:
> > > > >
> > > > > Does the DPDK project support segmented packets?
> > > > > If yes, then apply the patch to fix the bug!
> > > > >
> > > > > If anyone seriously cares about the regression it introduces,
> > optimization
> > > patches are welcome later. We shouldn't wait for it.
> > > >
> > > > You're right, but the regression is flagged to a 4-years old
> > > > patch, that's why I don't consider it as urgent.
> > > >
> > > > > If the patch is not applied, the documentation must be updated
> > > > > to
> > > mention that we are releasing DPDK with a known bug: that segmented
> > > packets are handled incorrectly in the scenario described in this
> > patch.
> > > >
> > > > Yes, would be good to document the known issue, no matter how old
> > it
> > > > is.
> > > >
> > > > > Generally, there could be some performance to gain by not
> > supporting
> > > segmented packets at all, as a compile time option. But that is a
> > different
> > > discussion.
> > >
> > >
> > >
> >
Hi Ali,
On Wed, Sep 29, 2021 at 08:03:17AM +0000, Ali Alnubani wrote:
> Hi Olivier,
>
> I wanted to retest the patch on latest main, but it no longer applies, could you please rebase it?
I rebased the patch:
https://patchwork.dpdk.org/project/dpdk/patch/20210929213707.17727-1-olivier.matz@6wind.com/
Thanks,
Olivier
>
> Thanks,
> Ali
>
> > -----Original Message-----
> > From: Morten Brørup <mb@smartsharesystems.com>
> > Sent: Tuesday, September 28, 2021 12:40 PM
> > To: Slava Ovsiienko <viacheslavo@nvidia.com>; NBU-Contact-Thomas
> > Monjalon <thomas@monjalon.net>; Olivier Matz <olivier.matz@6wind.com>;
> > Ali Alnubani <alialnu@nvidia.com>
> > Cc: dev@dpdk.org; David Marchand <david.marchand@redhat.com>; Alexander
> > Kozyrev <akozyrev@nvidia.com>; Ferruh Yigit <ferruh.yigit@intel.com>;
> > zhaoyan.chen@intel.com; Andrew Rybchenko
> > <andrew.rybchenko@oktetlabs.ru>; Ananyev, Konstantin
> > <konstantin.ananyev@intel.com>; Ajit Khaparde
> > <ajit.khaparde@broadcom.com>; jerinj@marvell.com
> > Subject: RE: [dpdk-dev] [dpdk-stable] [PATCH v4] mbuf: fix reset on mbuf free
> >
> > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Slava Ovsiienko
> > > Sent: Tuesday, 28 September 2021 11.01
> > >
> > > Hi,
> > >
> > > I've re-read the entire thread.
> > > If I understand correctly, the root problem was (in initial patch):
> > >
> > > > m1 = rte_pktmbuf_alloc(mp);
> > > > rte_pktmbuf_append(m1, 500);
> > > > m2 = rte_pktmbuf_alloc(mp);
> > > > rte_pktmbuf_append(m2, 500);
> > > > rte_pktmbuf_chain(m1, m2);
> > > > m0 = rte_pktmbuf_alloc(mp);
> > > > rte_pktmbuf_append(m0, 500);
> > > > rte_pktmbuf_chain(m0, m1);
> > > >
> > > > As rte_pktmbuf_chain() does not reset nb_seg in the initial m1
> > > segment
> > > > (this is not required), after this code the mbuf chain have 3
> > > > segments:
> > > > - m0: next=m1, nb_seg=3
> > > > - m1: next=m2, nb_seg=2
> > > > - m2: next=NULL, nb_seg=1
> > > >
> > > The proposed fix was to ALWAYS set next and nb_seg fields on
> > > mbuf_free(), regardless next field content. That would perform
> > > unconditional write to mbuf, and might affect the configurations,
> > > where are no multi- segment packets at al. mbuf_free() is "backbone"
> > > API, it is used by all cases, all scenaries are affected.
> > >
> > > As far as I know, the current approach for nb_seg field - it contains
> > > other value than 1 only in the first mbuf , for the following
> > > segments, it should not be considered at all (only the first segment
> > > fields are valid), and it is supposed to contain 1, as it was
> > > initially allocated from the pool.
> > >
> > > In the example above the problem was introduced by
> > > rte_pktmbuf_chain(). Could we consider fixing the rte_pktmbuf_chain()
> > > (used in potentially fewer common sceneries) instead of touching the
> > > extremely common rte_mbuf_free() ?
> > >
> > > With best regards,
> > > Slava
> >
> > Great idea, Slava!
> >
> > Changing the invariant for 'nb_segs', so it must be 1, except in the first segment
> > of a segmented packet.
> >
> > Thinking further about it, perhaps we can achieve even higher performance by a
> > minor additional modification: Use 0 instead of 1? Or offset 'nb_segs' by -1, so it
> > reflects the number of additional segments?
> >
> > And perhaps combining the invariants for 'nb_segs' and 'next' could provide even
> > more performance improvements. I don't know, just sharing a thought.
> >
> > Anyway, I vote for fixing the bug. One way or the other!
> >
> > -Morten
> >
> > >
> > > > -----Original Message-----
> > > > From: Thomas Monjalon <thomas@monjalon.net>
> > > > Sent: Tuesday, September 28, 2021 11:29
> > > >
> > > > Follow-up again:
> > > > We have added a note in 21.08, we should fix it in 21.11.
> > > > If there are no counter proposal, I suggest applying this patch, no
> > > matter the
> > > > performance regression.
> > > >
> > > >
> > > > 30/07/2021 16:54, Thomas Monjalon:
> > > > > 30/07/2021 16:35, Morten Brørup:
> > > > > > > From: Olivier Matz [mailto:olivier.matz@6wind.com]
> > > > > > > Sent: Friday, 30 July 2021 14.37
> > > > > > >
> > > > > > > Hi Thomas,
> > > > > > >
> > > > > > > On Sat, Jul 24, 2021 at 10:47:34AM +0200, Thomas Monjalon
> > > wrote:
> > > > > > > > What's the follow-up for this patch?
> > > > > > >
> > > > > > > Unfortunatly, I still don't have the time to work on this
> > > > > > > topic
> > > yet.
> > > > > > >
> > > > > > > In my initial tests, in our lab, I didn't notice any
> > > performance
> > > > > > > regression, but Ali has seen an impact (0.5M PPS, but I don't
> > > know
> > > > > > > how much in percent).
> > > > > > >
> > > > > > >
> > > > > > > > 19/01/2021 15:04, Slava Ovsiienko:
> > > > > > > > > Hi, All
> > > > > > > > >
> > > > > > > > > Could we postpose this patch at least to rc2? We would
> > > > > > > > > like
> > > to
> > > > > > > conduct more investigations?
> > > > > > > > >
> > > > > > > > > With best regards, Slava
> > > > > > > > >
> > > > > > > > > From: Olivier Matz <olivier.matz@6wind.com>
> > > > > > > > > > On Mon, Jan 18, 2021 at 05:52:32PM +0000, Ali Alnubani
> > > wrote:
> > > > > > > > > > > Hi,
> > > > > > > > > > > (Sorry had to resend this to some recipients due to
> > > mail
> > > > > > > > > > > server
> > > > > > > problems).
> > > > > > > > > > >
> > > > > > > > > > > Just confirming that I can still reproduce the
> > > regression
> > > > > > > > > > > with
> > > > > > > single core and
> > > > > > > > > > 64B frames on other servers.
> > > > > > > > > >
> > > > > > > > > > Many thanks for the feedback. Can you please detail what
> > > is
> > > > > > > > > > the
> > > > > > > amount of
> > > > > > > > > > performance loss in percent, and confirm the test case?
> > > (I
> > > > > > > suppose it is
> > > > > > > > > > testpmd io forward).
> > > > > > > > > >
> > > > > > > > > > Unfortunatly, I won't be able to spend a lot of time on
> > > this
> > > > > > > > > > soon
> > > > > > > (sorry for
> > > > > > > > > > that). So I see at least these 2 options:
> > > > > > > > > >
> > > > > > > > > > - postpone the patch again, until I can find more time
> > > > > > > > > > to
> > > analyze
> > > > > > > > > > and optimize
> > > > > > > > > > - apply the patch if the performance loss is acceptable
> > > > > > > > > > compared
> > > > > > > to
> > > > > > > > > > the added value of fixing a bug
> > > > > > > > > >
> > > > > > > > [...]
> > > > > > >
> > > > > > > Statu quo...
> > > > > > >
> > > > > > > Olivier
> > > > > > >
> > > > > >
> > > > > > The decision should be simple:
> > > > > >
> > > > > > Does the DPDK project support segmented packets?
> > > > > > If yes, then apply the patch to fix the bug!
> > > > > >
> > > > > > If anyone seriously cares about the regression it introduces,
> > > optimization
> > > > patches are welcome later. We shouldn't wait for it.
> > > > >
> > > > > You're right, but the regression is flagged to a 4-years old
> > > > > patch, that's why I don't consider it as urgent.
> > > > >
> > > > > > If the patch is not applied, the documentation must be updated
> > > > > > to
> > > > mention that we are releasing DPDK with a known bug: that segmented
> > > > packets are handled incorrectly in the scenario described in this
> > > patch.
> > > > >
> > > > > Yes, would be good to document the known issue, no matter how old
> > > it
> > > > > is.
> > > > >
> > > > > > Generally, there could be some performance to gain by not
> > > supporting
> > > > segmented packets at all, as a compile time option. But that is a
> > > different
> > > > discussion.
> > > >
> > > >
> > > >
> > >
>
> -----Original Message-----
> From: Olivier Matz <olivier.matz@6wind.com>
> Sent: Thursday, September 30, 2021 12:39 AM
> To: Ali Alnubani <alialnu@nvidia.com>
> Cc: dev@dpdk.org; David Marchand <david.marchand@redhat.com>;
> Alexander Kozyrev <akozyrev@nvidia.com>; NBU-Contact-Thomas Monjalon
> <thomas@monjalon.net>; Ferruh Yigit <ferruh.yigit@intel.com>; Slava
> Ovsiienko <viacheslavo@nvidia.com>; zhaoyan.chen@intel.com; Morten
> Brørup <mb@smartsharesystems.com>; Andrew Rybchenko
> <andrew.rybchenko@oktetlabs.ru>; Ananyev, Konstantin
> <konstantin.ananyev@intel.com>; Ajit Khaparde
> <ajit.khaparde@broadcom.com>; jerinj@marvell.com
> Subject: Re: [dpdk-dev] [dpdk-stable] [PATCH v4] mbuf: fix reset on mbuf
> free
>
> Hi Ali,
>
>
> On Wed, Sep 29, 2021 at 08:03:17AM +0000, Ali Alnubani wrote:
> > Hi Olivier,
> >
> > I wanted to retest the patch on latest main, but it no longer applies, could
> you please rebase it?
>
> I rebased the patch:
> https://patchwork.dpdk.org/project/dpdk/patch/20210929213707.17727-1-
> olivier.matz@6wind.com/
>
Thanks for the update,
I tested v5 on main (84b3e4555a1f) and the regression we previously saw no longer reproduces.
Will the patch be backported to 20.11? I can still reproduce a 2% drop in single core packet forwarding performance with v4 applied on v20.11.3.
Testpmd: dpdk-testpmd -n 4 -a 0000:82:00.0 -a 0000:82:00.1 -c 0x9000 -- -i --nb-cores=1 --txd=256 --rxd=256 --auto-start
Traffic generator is TREX with pattern (Ether / IP / UDP).
Drop is from ~50.5 mpps to ~49.5 mpps.
OS: 18.04.5 LTS
Device: ConnectX-5
MLNX_OFED: MLNX_OFED_LINUX-5.4-1.0.3.0
Firmware: 16.31.1014
Regards,
Ali
30/09/2021 15:29, Ali Alnubani:
> From: Olivier Matz <olivier.matz@6wind.com>
> > On Wed, Sep 29, 2021 at 08:03:17AM +0000, Ali Alnubani wrote:
> > > Hi Olivier,
> > >
> > > I wanted to retest the patch on latest main, but it no longer applies, could
> > you please rebase it?
> >
> > I rebased the patch:
> > https://patchwork.dpdk.org/project/dpdk/patch/20210929213707.17727-1-
> > olivier.matz@6wind.com/
>
> Thanks for the update,
> I tested v5 on main (84b3e4555a1f) and the regression we previously saw no longer reproduces.
>
> Will the patch be backported to 20.11? I can still reproduce a 2% drop in single core packet forwarding performance with v4 applied on v20.11.3.
As any significant fix, we must backport, or at least, try.
If there is a real issue in a stable branch, we'll discuss it
in the test&release process of this branch.
@@ -2677,6 +2677,70 @@ test_mbuf_dyn(struct rte_mempool *pktmbuf_pool)
return -1;
}
+/* check that m->nb_segs and m->next are reset on mbuf free */
+static int
+test_nb_segs_and_next_reset(void)
+{
+ struct rte_mbuf *m0 = NULL, *m1 = NULL, *m2 = NULL;
+ struct rte_mempool *pool = NULL;
+
+ pool = rte_pktmbuf_pool_create("test_mbuf_reset",
+ 3, 0, 0, MBUF_DATA_SIZE, SOCKET_ID_ANY);
+ if (pool == NULL)
+ GOTO_FAIL("Failed to create mbuf pool");
+
+ /* alloc mbufs */
+ m0 = rte_pktmbuf_alloc(pool);
+ m1 = rte_pktmbuf_alloc(pool);
+ m2 = rte_pktmbuf_alloc(pool);
+ if (m0 == NULL || m1 == NULL || m2 == NULL)
+ GOTO_FAIL("Failed to allocate mbuf");
+
+ /* append data in all of them */
+ if (rte_pktmbuf_append(m0, 500) == NULL ||
+ rte_pktmbuf_append(m1, 500) == NULL ||
+ rte_pktmbuf_append(m2, 500) == NULL)
+ GOTO_FAIL("Failed to append data in mbuf");
+
+ /* chain them in one mbuf m0 */
+ rte_pktmbuf_chain(m1, m2);
+ rte_pktmbuf_chain(m0, m1);
+ if (m0->nb_segs != 3 || m0->next != m1 || m1->next != m2 ||
+ m2->next != NULL) {
+ m1 = m2 = NULL;
+ GOTO_FAIL("Failed to chain mbufs");
+ }
+
+ /* split m0 chain in two, between m1 and m2 */
+ m0->nb_segs = 2;
+ m1->next = NULL;
+ m2->nb_segs = 1;
+
+ /* free the 2 mbuf chains m0 and m2 */
+ rte_pktmbuf_free(m0);
+ rte_pktmbuf_free(m2);
+
+ /* realloc the 3 mbufs */
+ m0 = rte_mbuf_raw_alloc(pool);
+ m1 = rte_mbuf_raw_alloc(pool);
+ m2 = rte_mbuf_raw_alloc(pool);
+ if (m0 == NULL || m1 == NULL || m2 == NULL)
+ GOTO_FAIL("Failed to reallocate mbuf");
+
+ /* ensure that m->next and m->nb_segs are reset allocated mbufs */
+ if (m0->nb_segs != 1 || m0->next != NULL ||
+ m1->nb_segs != 1 || m1->next != NULL ||
+ m2->nb_segs != 1 || m2->next != NULL)
+ GOTO_FAIL("nb_segs or next was not reset properly");
+
+ return 0;
+
+fail:
+ if (pool != NULL)
+ rte_mempool_free(pool);
+ return -1;
+}
+
static int
test_mbuf(void)
{
@@ -2867,6 +2931,11 @@ test_mbuf(void)
goto err;
}
+ /* test reset of m->nb_segs and m->next on mbuf free */
+ if (test_nb_segs_and_next_reset() < 0) {
+ printf("test_nb_segs_and_next_reset() failed\n");
+ goto err;
+ }
ret = 0;
err:
@@ -129,10 +129,10 @@ rte_pktmbuf_free_pinned_extmem(void *addr, void *opaque)
rte_mbuf_ext_refcnt_set(m->shinfo, 1);
m->ol_flags = EXT_ATTACHED_MBUF;
- if (m->next != NULL) {
+ if (m->next != NULL)
m->next = NULL;
+ if (m->nb_segs != 1)
m->nb_segs = 1;
- }
rte_mbuf_raw_free(m);
}
@@ -1340,10 +1340,10 @@ rte_pktmbuf_prefree_seg(struct rte_mbuf *m)
return NULL;
}
- if (m->next != NULL) {
+ if (m->next != NULL)
m->next = NULL;
+ if (m->nb_segs != 1)
m->nb_segs = 1;
- }
return m;
@@ -1357,10 +1357,10 @@ rte_pktmbuf_prefree_seg(struct rte_mbuf *m)
return NULL;
}
- if (m->next != NULL) {
+ if (m->next != NULL)
m->next = NULL;
+ if (m->nb_segs != 1)
m->nb_segs = 1;
- }
rte_mbuf_refcnt_set(m, 1);
return m;
@@ -495,7 +495,12 @@ struct rte_mbuf {
* or non-atomic) is controlled by the RTE_MBUF_REFCNT_ATOMIC flag.
*/
uint16_t refcnt;
- uint16_t nb_segs; /**< Number of segments. */
+
+ /**
+ * Number of segments. Only valid for the first segment of an mbuf
+ * chain.
+ */
+ uint16_t nb_segs;
/** Input port (16 bits to support more than 256 virtual ports).
* The event eth Tx adapter uses this field to specify the output port.
@@ -591,7 +596,11 @@ struct rte_mbuf {
/* second cache line - fields only used in slow path or on TX */
RTE_MARKER cacheline1 __rte_cache_min_aligned;
- struct rte_mbuf *next; /**< Next segment of scattered packet. */
+ /**
+ * Next segment of scattered packet. Must be NULL in the last segment or
+ * in case of non-segmented packet.
+ */
+ struct rte_mbuf *next;
/* fields to support TX offloads */
RTE_STD_C11