[dpdk-stable] [PATCH] mbuf: fix reset on mbuf free

[dpdk-stable] [PATCH] mbuf: fix reset on mbuf free

[Olivier Matz]

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

Freeing this mbuf chain will not restore nb_seg=1 in the second
segment. This 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>
---
 lib/librte_mbuf/rte_mbuf.c |  6 ++----
 lib/librte_mbuf/rte_mbuf.h | 12 ++++--------
 2 files changed, 6 insertions(+), 12 deletions(-)

diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c
index 8a456e5e64..e632071c23 100644
--- a/lib/librte_mbuf/rte_mbuf.c
+++ b/lib/librte_mbuf/rte_mbuf.c
@@ -129,10 +129,8 @@ 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) {
-		m->next = NULL;
-		m->nb_segs = 1;
-	}
+	m->next = NULL;
+	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 a1414ed7cd..ef5800c8ef 100644
--- a/lib/librte_mbuf/rte_mbuf.h
+++ b/lib/librte_mbuf/rte_mbuf.h
@@ -1329,10 +1329,8 @@ rte_pktmbuf_prefree_seg(struct rte_mbuf *m)
 				return NULL;
 		}
 
-		if (m->next != NULL) {
-			m->next = NULL;
-			m->nb_segs = 1;
-		}
+		m->next = NULL;
+		m->nb_segs = 1;
 
 		return m;
 
@@ -1346,10 +1344,8 @@ rte_pktmbuf_prefree_seg(struct rte_mbuf *m)
 				return NULL;
 		}
 
-		if (m->next != NULL) {
-			m->next = NULL;
-			m->nb_segs = 1;
-		}
+		m->next = NULL;
+		m->nb_segs = 1;
 		rte_mbuf_refcnt_set(m, 1);
 
 		return m;
-- 
2.25.1

Re: [dpdk-stable] [PATCH] mbuf: fix reset on mbuf free

[Ananyev, Konstantin]

Hi Olivier,
 
> 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
> 
> Freeing this mbuf chain will not restore nb_seg=1 in the second
> segment. 

Hmm, not sure why is that?
You are talking about freeing m1, right?
rte_pktmbuf_prefree_seg(struct rte_mbuf *m)
{
	...
	if (m->next != NULL) {
                        m->next = NULL;
                        m->nb_segs = 1;
                }

m1->next != NULL, so it will enter the if() block,
and will reset both next and nb_segs.
What I am missing here? 
Thinking in more generic way, that change:
 -		if (m->next != NULL) {
 -			m->next = NULL;
 -			m->nb_segs = 1;
 -		}
 +		m->next = NULL;
 +		m->nb_segs = 1;

Assumes that it is ok to have an mbuf with
nb_seg > 1 and next == NULL.
Which seems wrong to me.
Konstantin


>This 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>
> ---
>  lib/librte_mbuf/rte_mbuf.c |  6 ++----
>  lib/librte_mbuf/rte_mbuf.h | 12 ++++--------
>  2 files changed, 6 insertions(+), 12 deletions(-)
> 
> diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c
> index 8a456e5e64..e632071c23 100644
> --- a/lib/librte_mbuf/rte_mbuf.c
> +++ b/lib/librte_mbuf/rte_mbuf.c
> @@ -129,10 +129,8 @@ 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) {
> -		m->next = NULL;
> -		m->nb_segs = 1;
> -	}
> +	m->next = NULL;
> +	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 a1414ed7cd..ef5800c8ef 100644
> --- a/lib/librte_mbuf/rte_mbuf.h
> +++ b/lib/librte_mbuf/rte_mbuf.h
> @@ -1329,10 +1329,8 @@ rte_pktmbuf_prefree_seg(struct rte_mbuf *m)
>  				return NULL;
>  		}
> 
> -		if (m->next != NULL) {
> -			m->next = NULL;
> -			m->nb_segs = 1;
> -		}
> +		m->next = NULL;
> +		m->nb_segs = 1;
> 
>  		return m;
> 
> @@ -1346,10 +1344,8 @@ rte_pktmbuf_prefree_seg(struct rte_mbuf *m)
>  				return NULL;
>  		}
> 
> -		if (m->next != NULL) {
> -			m->next = NULL;
> -			m->nb_segs = 1;
> -		}
> +		m->next = NULL;
> +		m->nb_segs = 1;
>  		rte_mbuf_refcnt_set(m, 1);
> 
>  		return m;
> --
> 2.25.1

Re: [dpdk-stable] [PATCH] mbuf: fix reset on mbuf free

[Olivier Matz]

On Thu, Nov 05, 2020 at 12:15:49AM +0000, Ananyev, Konstantin wrote:
> 
> Hi Olivier,
>  
> > 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
> > 
> > Freeing this mbuf chain will not restore nb_seg=1 in the second
> > segment. 
> 
> Hmm, not sure why is that?
> You are talking about freeing m1, right?
> rte_pktmbuf_prefree_seg(struct rte_mbuf *m)
> {
> 	...
> 	if (m->next != NULL) {
>                         m->next = NULL;
>                         m->nb_segs = 1;
>                 }
> 
> m1->next != NULL, so it will enter the if() block,
> and will reset both next and nb_segs.
> What I am missing here? 
> Thinking in more generic way, that change:
>  -		if (m->next != NULL) {
>  -			m->next = NULL;
>  -			m->nb_segs = 1;
>  -		}
>  +		m->next = NULL;
>  +		m->nb_segs = 1;

Ah, sorry. I oversimplified the example and now it does not
show the issue...

The full example also adds a split() to break the mbuf chain
between m1 and m2. The kind of thing that would be done for
software TCP segmentation.

After this operation, we have 2 mbuf chain:
 - m0 with 2 segments, the last one has next=NULL but nb_seg=2
 - new_m with 1 segment

Freeing m0 will not restore nb_seg=1 in the second segment.

> Assumes that it is ok to have an mbuf with
> nb_seg > 1 and next == NULL.
> Which seems wrong to me.

I don't think it is wrong: nb_seg is just ignored when not in the first
segment, and there is nothing saying it should be set to 1. Typically,
rte_pktmbuf_chain() does not change it, and I guess it's the same for
many similar functions in applications.

Olivier

> 
> 
> >This 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>
> > ---
> >  lib/librte_mbuf/rte_mbuf.c |  6 ++----
> >  lib/librte_mbuf/rte_mbuf.h | 12 ++++--------
> >  2 files changed, 6 insertions(+), 12 deletions(-)
> > 
> > diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c
> > index 8a456e5e64..e632071c23 100644
> > --- a/lib/librte_mbuf/rte_mbuf.c
> > +++ b/lib/librte_mbuf/rte_mbuf.c
> > @@ -129,10 +129,8 @@ 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) {
> > -		m->next = NULL;
> > -		m->nb_segs = 1;
> > -	}
> > +	m->next = NULL;
> > +	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 a1414ed7cd..ef5800c8ef 100644
> > --- a/lib/librte_mbuf/rte_mbuf.h
> > +++ b/lib/librte_mbuf/rte_mbuf.h
> > @@ -1329,10 +1329,8 @@ rte_pktmbuf_prefree_seg(struct rte_mbuf *m)
> >  				return NULL;
> >  		}
> > 
> > -		if (m->next != NULL) {
> > -			m->next = NULL;
> > -			m->nb_segs = 1;
> > -		}
> > +		m->next = NULL;
> > +		m->nb_segs = 1;
> > 
> >  		return m;
> > 
> > @@ -1346,10 +1344,8 @@ rte_pktmbuf_prefree_seg(struct rte_mbuf *m)
> >  				return NULL;
> >  		}
> > 
> > -		if (m->next != NULL) {
> > -			m->next = NULL;
> > -			m->nb_segs = 1;
> > -		}
> > +		m->next = NULL;
> > +		m->nb_segs = 1;
> >  		rte_mbuf_refcnt_set(m, 1);
> > 
> >  		return m;
> > --
> > 2.25.1
> 

Re: [dpdk-stable] [dpdk-dev] [PATCH] mbuf: fix reset on mbuf free

[Morten Brørup]

> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Olivier Matz
> Sent: Thursday, November 5, 2020 8:46 AM
> 
> On Thu, Nov 05, 2020 at 12:15:49AM +0000, Ananyev, Konstantin wrote:
> >
> > Hi Olivier,
> >
> > > 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
> > >
> > > Freeing this mbuf chain will not restore nb_seg=1 in the second
> > > segment.
> >
> > Hmm, not sure why is that?
> > You are talking about freeing m1, right?
> > rte_pktmbuf_prefree_seg(struct rte_mbuf *m)
> > {
> > 	...
> > 	if (m->next != NULL) {
> >                         m->next = NULL;
> >                         m->nb_segs = 1;
> >                 }
> >
> > m1->next != NULL, so it will enter the if() block,
> > and will reset both next and nb_segs.
> > What I am missing here?
> > Thinking in more generic way, that change:
> >  -		if (m->next != NULL) {
> >  -			m->next = NULL;
> >  -			m->nb_segs = 1;
> >  -		}
> >  +		m->next = NULL;
> >  +		m->nb_segs = 1;
> 
> Ah, sorry. I oversimplified the example and now it does not
> show the issue...
> 
> The full example also adds a split() to break the mbuf chain
> between m1 and m2. The kind of thing that would be done for
> software TCP segmentation.
> 
> After this operation, we have 2 mbuf chain:
>  - m0 with 2 segments, the last one has next=NULL but nb_seg=2
>  - new_m with 1 segment
> 
> Freeing m0 will not restore nb_seg=1 in the second segment.
> 
> > Assumes that it is ok to have an mbuf with
> > nb_seg > 1 and next == NULL.
> > Which seems wrong to me.
> 
> I don't think it is wrong: nb_seg is just ignored when not in the first
> segment, and there is nothing saying it should be set to 1. Typically,
> rte_pktmbuf_chain() does not change it, and I guess it's the same for
> many similar functions in applications.
> 
> Olivier

Acked-by: Morten Brørup <mb@smartsharesystems.com>

And while you are at it, please consider extending the description of the two mbuf fields with their invariants:
1. nb_segs is only valid for the first segment of a multi-segment packet.
2. next is NULL for non-segmented packets.

> 
> >
> >
> > >This 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>
> > > ---
> > >  lib/librte_mbuf/rte_mbuf.c |  6 ++----
> > >  lib/librte_mbuf/rte_mbuf.h | 12 ++++--------
> > >  2 files changed, 6 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/lib/librte_mbuf/rte_mbuf.c
> b/lib/librte_mbuf/rte_mbuf.c
> > > index 8a456e5e64..e632071c23 100644
> > > --- a/lib/librte_mbuf/rte_mbuf.c
> > > +++ b/lib/librte_mbuf/rte_mbuf.c
> > > @@ -129,10 +129,8 @@ 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) {
> > > -		m->next = NULL;
> > > -		m->nb_segs = 1;
> > > -	}
> > > +	m->next = NULL;
> > > +	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 a1414ed7cd..ef5800c8ef 100644
> > > --- a/lib/librte_mbuf/rte_mbuf.h
> > > +++ b/lib/librte_mbuf/rte_mbuf.h
> > > @@ -1329,10 +1329,8 @@ rte_pktmbuf_prefree_seg(struct rte_mbuf *m)
> > >  				return NULL;
> > >  		}
> > >
> > > -		if (m->next != NULL) {
> > > -			m->next = NULL;
> > > -			m->nb_segs = 1;
> > > -		}
> > > +		m->next = NULL;
> > > +		m->nb_segs = 1;
> > >
> > >  		return m;
> > >
> > > @@ -1346,10 +1344,8 @@ rte_pktmbuf_prefree_seg(struct rte_mbuf *m)
> > >  				return NULL;
> > >  		}
> > >
> > > -		if (m->next != NULL) {
> > > -			m->next = NULL;
> > > -			m->nb_segs = 1;
> > > -		}
> > > +		m->next = NULL;
> > > +		m->nb_segs = 1;
> > >  		rte_mbuf_refcnt_set(m, 1);
> > >
> > >  		return m;
> > > --
> > > 2.25.1
> >

Re: [dpdk-stable] [dpdk-dev] [PATCH] mbuf: fix reset on mbuf free

[Olivier Matz]

On Thu, Nov 05, 2020 at 09:33:58AM +0100, Morten Brørup wrote:
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Olivier Matz
> > Sent: Thursday, November 5, 2020 8:46 AM
> > 
> > On Thu, Nov 05, 2020 at 12:15:49AM +0000, Ananyev, Konstantin wrote:
> > >
> > > Hi Olivier,
> > >
> > > > 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
> > > >
> > > > Freeing this mbuf chain will not restore nb_seg=1 in the second
> > > > segment.
> > >
> > > Hmm, not sure why is that?
> > > You are talking about freeing m1, right?
> > > rte_pktmbuf_prefree_seg(struct rte_mbuf *m)
> > > {
> > > 	...
> > > 	if (m->next != NULL) {
> > >                         m->next = NULL;
> > >                         m->nb_segs = 1;
> > >                 }
> > >
> > > m1->next != NULL, so it will enter the if() block,
> > > and will reset both next and nb_segs.
> > > What I am missing here?
> > > Thinking in more generic way, that change:
> > >  -		if (m->next != NULL) {
> > >  -			m->next = NULL;
> > >  -			m->nb_segs = 1;
> > >  -		}
> > >  +		m->next = NULL;
> > >  +		m->nb_segs = 1;
> > 
> > Ah, sorry. I oversimplified the example and now it does not
> > show the issue...
> > 
> > The full example also adds a split() to break the mbuf chain
> > between m1 and m2. The kind of thing that would be done for
> > software TCP segmentation.
> > 
> > After this operation, we have 2 mbuf chain:
> >  - m0 with 2 segments, the last one has next=NULL but nb_seg=2
> >  - new_m with 1 segment
> > 
> > Freeing m0 will not restore nb_seg=1 in the second segment.
> > 
> > > Assumes that it is ok to have an mbuf with
> > > nb_seg > 1 and next == NULL.
> > > Which seems wrong to me.
> > 
> > I don't think it is wrong: nb_seg is just ignored when not in the first
> > segment, and there is nothing saying it should be set to 1. Typically,
> > rte_pktmbuf_chain() does not change it, and I guess it's the same for
> > many similar functions in applications.
> > 
> > Olivier
> 
> Acked-by: Morten Brørup <mb@smartsharesystems.com>
> 
> And while you are at it, please consider extending the description of the two mbuf fields with their invariants:
> 1. nb_segs is only valid for the first segment of a multi-segment packet.
> 2. next is NULL for non-segmented packets.

Good point, will add it in v2.

> 
> > 
> > >
> > >
> > > >This 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>
> > > > ---
> > > >  lib/librte_mbuf/rte_mbuf.c |  6 ++----
> > > >  lib/librte_mbuf/rte_mbuf.h | 12 ++++--------
> > > >  2 files changed, 6 insertions(+), 12 deletions(-)
> > > >
> > > > diff --git a/lib/librte_mbuf/rte_mbuf.c
> > b/lib/librte_mbuf/rte_mbuf.c
> > > > index 8a456e5e64..e632071c23 100644
> > > > --- a/lib/librte_mbuf/rte_mbuf.c
> > > > +++ b/lib/librte_mbuf/rte_mbuf.c
> > > > @@ -129,10 +129,8 @@ 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) {
> > > > -		m->next = NULL;
> > > > -		m->nb_segs = 1;
> > > > -	}
> > > > +	m->next = NULL;
> > > > +	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 a1414ed7cd..ef5800c8ef 100644
> > > > --- a/lib/librte_mbuf/rte_mbuf.h
> > > > +++ b/lib/librte_mbuf/rte_mbuf.h
> > > > @@ -1329,10 +1329,8 @@ rte_pktmbuf_prefree_seg(struct rte_mbuf *m)
> > > >  				return NULL;
> > > >  		}
> > > >
> > > > -		if (m->next != NULL) {
> > > > -			m->next = NULL;
> > > > -			m->nb_segs = 1;
> > > > -		}
> > > > +		m->next = NULL;
> > > > +		m->nb_segs = 1;
> > > >
> > > >  		return m;
> > > >
> > > > @@ -1346,10 +1344,8 @@ rte_pktmbuf_prefree_seg(struct rte_mbuf *m)
> > > >  				return NULL;
> > > >  		}
> > > >
> > > > -		if (m->next != NULL) {
> > > > -			m->next = NULL;
> > > > -			m->nb_segs = 1;
> > > > -		}
> > > > +		m->next = NULL;
> > > > +		m->nb_segs = 1;
> > > >  		rte_mbuf_refcnt_set(m, 1);
> > > >
> > > >  		return m;
> > > > --
> > > > 2.25.1
> > >
> 

Re: [dpdk-stable] [dpdk-dev] [PATCH] mbuf: fix reset on mbuf free

[Andrew Rybchenko]

Just resend with lost Cc restored.

On 11/5/20 10:46 AM, Olivier Matz wrote:
> On Thu, Nov 05, 2020 at 12:15:49AM +0000, Ananyev, Konstantin wrote:
>>
>> Hi Olivier,
>>  
>>> 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
>>>
>>> Freeing this mbuf chain will not restore nb_seg=1 in the second
>>> segment. 
>>
>> Hmm, not sure why is that?
>> You are talking about freeing m1, right?
>> rte_pktmbuf_prefree_seg(struct rte_mbuf *m)
>> {
>> 	...
>> 	if (m->next != NULL) {
>>                         m->next = NULL;
>>                         m->nb_segs = 1;
>>                 }
>>
>> m1->next != NULL, so it will enter the if() block,
>> and will reset both next and nb_segs.
>> What I am missing here? 
>> Thinking in more generic way, that change:
>>  -		if (m->next != NULL) {
>>  -			m->next = NULL;
>>  -			m->nb_segs = 1;
>>  -		}
>>  +		m->next = NULL;
>>  +		m->nb_segs = 1;
> 
> Ah, sorry. I oversimplified the example and now it does not
> show the issue...
> 
> The full example also adds a split() to break the mbuf chain
> between m1 and m2. The kind of thing that would be done for
> software TCP segmentation.
> 

If so, may be the right solution is to care about nb_segs
when next is set to NULL on split? Any place when next is set
to NULL. Just to keep the optimization in a more generic place.

> After this operation, we have 2 mbuf chain:
>  - m0 with 2 segments, the last one has next=NULL but nb_seg=2
>  - new_m with 1 segment
> 
> Freeing m0 will not restore nb_seg=1 in the second segment.
> 
>> Assumes that it is ok to have an mbuf with
>> nb_seg > 1 and next == NULL.
>> Which seems wrong to me.
> 
> I don't think it is wrong: nb_seg is just ignored when not in the first
> segment, and there is nothing saying it should be set to 1. Typically,
> rte_pktmbuf_chain() does not change it, and I guess it's the same for
> many similar functions in applications.
> 
> Olivier
> 
>>
>>
>>> This 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>
>>> ---
>>>  lib/librte_mbuf/rte_mbuf.c |  6 ++----
>>>  lib/librte_mbuf/rte_mbuf.h | 12 ++++--------
>>>  2 files changed, 6 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c
>>> index 8a456e5e64..e632071c23 100644
>>> --- a/lib/librte_mbuf/rte_mbuf.c
>>> +++ b/lib/librte_mbuf/rte_mbuf.c
>>> @@ -129,10 +129,8 @@ 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) {
>>> -		m->next = NULL;
>>> -		m->nb_segs = 1;
>>> -	}
>>> +	m->next = NULL;
>>> +	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 a1414ed7cd..ef5800c8ef 100644
>>> --- a/lib/librte_mbuf/rte_mbuf.h
>>> +++ b/lib/librte_mbuf/rte_mbuf.h
>>> @@ -1329,10 +1329,8 @@ rte_pktmbuf_prefree_seg(struct rte_mbuf *m)
>>>  				return NULL;
>>>  		}
>>>
>>> -		if (m->next != NULL) {
>>> -			m->next = NULL;
>>> -			m->nb_segs = 1;
>>> -		}
>>> +		m->next = NULL;
>>> +		m->nb_segs = 1;
>>>
>>>  		return m;
>>>
>>> @@ -1346,10 +1344,8 @@ rte_pktmbuf_prefree_seg(struct rte_mbuf *m)
>>>  				return NULL;
>>>  		}
>>>
>>> -		if (m->next != NULL) {
>>> -			m->next = NULL;
>>> -			m->nb_segs = 1;
>>> -		}
>>> +		m->next = NULL;
>>> +		m->nb_segs = 1;
>>>  		rte_mbuf_refcnt_set(m, 1);
>>>
>>>  		return m;
>>> --
>>> 2.25.1
>>

Re: [dpdk-stable] [dpdk-dev] [PATCH] mbuf: fix reset on mbuf free

[Ali Alnubani]

Hi Olivier,

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Olivier Matz
> Sent: Wednesday, November 4, 2020 7:00 PM
> To: dev@dpdk.org
> Cc: konstantin.ananyev@intel.com; stable@dpdk.org
> Subject: [dpdk-dev] [PATCH] mbuf: fix reset on mbuf free
> 

I can reproduce the failure in "ci/iol-mellanox-Performance" locally also with this patch.
I see a 2 mpps degradation with single core on ConnectX-5. Packet size is 64B. Testpmd command:
"""
dpdk-testpmd -n 4  -w 0000:82:00.1  -w 0000:82:00.0 -c 0x9000  -- --burst=64 --mbcache=512 -i  --nb-cores=1  --txd=256 --rxd=256
"""

Regards,
Ali

[dpdk-stable] [PATCH v2] mbuf: fix reset on mbuf free

[Olivier Matz]

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=3
    - m1: next=m2, 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>
---
 lib/librte_mbuf/rte_mbuf.c      |  4 ++--
 lib/librte_mbuf/rte_mbuf.h      |  8 ++++----
 lib/librte_mbuf/rte_mbuf_core.h | 13 +++++++++++--
 3 files changed, 17 insertions(+), 8 deletions(-)

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;
diff --git a/lib/librte_mbuf/rte_mbuf_core.h b/lib/librte_mbuf/rte_mbuf_core.h
index 567551deab..78a1fcc8ff 100644
--- a/lib/librte_mbuf/rte_mbuf_core.h
+++ b/lib/librte_mbuf/rte_mbuf_core.h
@@ -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
-- 
2.25.1

Re: [dpdk-stable] [dpdk-dev] [PATCH v2] mbuf: fix reset on mbuf free

[Morten Brørup]

> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Olivier Matz
> 
> 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=3
>     - m1: next=m2, nb_seg=2

I think you mean:

- m0: next=m1, nb_seg=2  //< nb_seg corrected
- m1: next=NULL, nb_seg=2   //< next corrected

>   - 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>

The code looks good, so:
Acked-by: Morten Brørup <mb@smartsharesystems.com>

Re: [dpdk-stable] [dpdk-dev] [PATCH v2] mbuf: fix reset on mbuf free

[Ajit Khaparde]

On Fri, Dec 18, 2020 at 5:18 AM Morten Brørup <mb@smartsharesystems.com> wrote:
>
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Olivier Matz
> >
> > 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=3
> >     - m1: next=m2, nb_seg=2
>
> I think you mean:
>
> - m0: next=m1, nb_seg=2  //< nb_seg corrected
> - m1: next=NULL, nb_seg=2   //< next corrected
>
> >   - 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>
>
> The code looks good, so:
> Acked-by: Morten Brørup <mb@smartsharesystems.com>

Acked-by: Ajit Khaparde <ajit.khaparde@broadcom.com>

[dpdk-stable] [PATCH v3] mbuf: fix reset on mbuf free

[Olivier Matz]

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>
---

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


 lib/librte_mbuf/rte_mbuf.c      |  4 ++--
 lib/librte_mbuf/rte_mbuf.h      |  8 ++++----
 lib/librte_mbuf/rte_mbuf_core.h | 13 +++++++++++--
 3 files changed, 17 insertions(+), 8 deletions(-)

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;
diff --git a/lib/librte_mbuf/rte_mbuf_core.h b/lib/librte_mbuf/rte_mbuf_core.h
index 567551deab..78a1fcc8ff 100644
--- a/lib/librte_mbuf/rte_mbuf_core.h
+++ b/lib/librte_mbuf/rte_mbuf_core.h
@@ -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
-- 
2.29.2

Re: [dpdk-stable] [PATCH v3] mbuf: fix reset on mbuf free

[Ali Alnubani]

Hi Olivier,

> -----Original Message-----
> From: Olivier Matz <olivier.matz@6wind.com>
> Sent: Wednesday, January 6, 2021 3:34 PM
> To: dev@dpdk.org
> Cc: andrew.rybchenko@oktetlabs.ru; konstantin.ananyev@intel.com;
> mb@smartsharesystems.com; Ali Alnubani <alialnu@nvidia.com>;
> ajitkhaparde@gmail.com; stable@dpdk.org; Ajit Khaparde
> <ajit.khaparde@broadcom.com>
> Subject: [PATCH v3] mbuf: fix reset on mbuf free
> 

Even though the performance tests on Mellanox NICs are passing, I see a performance drop of up to 0.5 mpps with 64B frames (tests only fail for 1 mpps drop or more):
https://mails.dpdk.org/archives/test-report/2021-January/172759.html

I'll verify this on local hardware and reply back.

Regards,
Ali 

Re: [dpdk-stable] [PATCH v3] mbuf: fix reset on mbuf free

[Ananyev, Konstantin]

> 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>
> ---
> 
> 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
> 
> 
>  lib/librte_mbuf/rte_mbuf.c      |  4 ++--
>  lib/librte_mbuf/rte_mbuf.h      |  8 ++++----
>  lib/librte_mbuf/rte_mbuf_core.h | 13 +++++++++++--
>  3 files changed, 17 insertions(+), 8 deletions(-)
> 
> 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;
> diff --git a/lib/librte_mbuf/rte_mbuf_core.h b/lib/librte_mbuf/rte_mbuf_core.h
> index 567551deab..78a1fcc8ff 100644
> --- a/lib/librte_mbuf/rte_mbuf_core.h
> +++ b/lib/librte_mbuf/rte_mbuf_core.h
> @@ -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
> --

Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>

> 2.29.2

[dpdk-stable] [PATCH v4] mbuf: fix reset on mbuf free

[Olivier Matz]

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(-)

diff --git a/app/test/test_mbuf.c b/app/test/test_mbuf.c
index a40f7d4883..ad2cbab600 100644
--- a/app/test/test_mbuf.c
+++ b/app/test/test_mbuf.c
@@ -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:
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;
diff --git a/lib/librte_mbuf/rte_mbuf_core.h b/lib/librte_mbuf/rte_mbuf_core.h
index 567551deab..78a1fcc8ff 100644
--- a/lib/librte_mbuf/rte_mbuf_core.h
+++ b/lib/librte_mbuf/rte_mbuf_core.h
@@ -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
-- 
2.29.2

Re: [dpdk-stable] [PATCH v4] mbuf: fix reset on mbuf free

[David Marchand]

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.
-- 
David Marchand

Re: [dpdk-stable] [PATCH v4] mbuf: fix reset on mbuf free

[Ali Alnubani]

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

Re: [dpdk-stable] [PATCH v4] mbuf: fix reset on mbuf free

[Ali Alnubani]

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

Re: [dpdk-stable] [PATCH v4] mbuf: fix reset on mbuf free

[Olivier Matz]

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

Re: [dpdk-stable] [PATCH v4] mbuf: fix reset on mbuf free

[Morten Brørup]

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

Re: [dpdk-stable] [PATCH v4] mbuf: fix reset on mbuf free

[Ferruh Yigit]

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
> 

Re: [dpdk-stable] [dpdk-dev] [PATCH v4] mbuf: fix reset on mbuf free

[Morten Brørup]

> 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
> >
> 

Re: [dpdk-stable] [dpdk-dev] [PATCH v4] mbuf: fix reset on mbuf free

[Ferruh Yigit]

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
>>>
>>
> 

Re: [dpdk-stable] [PATCH v4] mbuf: fix reset on mbuf free

[Slava Ovsiienko]

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&amp;sdata=VGvj%2F5GcAOxof6C
> mlZkq
> > > KXKOL52GctXcuL5RJXr1y8g%3D&amp;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&amp;sdata=XSirRbm5G0WwfxySe
> b0ALp
> > > owVosqoY6Nlv4UZCd1CZM%3D&amp;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

Re: [dpdk-stable] [dpdk-dev] [PATCH v4] mbuf: fix reset on mbuf free

[Morten Brørup]

> 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
> >>>
> >>
> >
> 

Re: [dpdk-stable] [dpdk-dev] [PATCH v4] mbuf: fix reset on mbuf free

[Ferruh Yigit]

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
>>>>>
>>>>
>>>
>>
> 

Re: [dpdk-stable] [PATCH v4] mbuf: fix reset on mbuf free

[Ferruh Yigit]

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.

Re: [dpdk-stable] [dpdk-dev] [PATCH v4] mbuf: fix reset on mbuf free

[Morten Brørup]

> 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.

Re: [dpdk-stable] [dpdklab] RE: [dpdk-dev] [PATCH v4] mbuf: fix reset on mbuf free

[Lincoln Lavoie]

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.
>
>

-- 
*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>

Re: [dpdk-stable] [dpdk-dev] [dpdklab] RE: [PATCH v4] mbuf: fix reset on mbuf free

[Morten Brørup]

> 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>

Re: [dpdk-stable] [dpdk-dev] [dpdklab] RE: [PATCH v4] mbuf: fix reset on mbuf free

[Brandon Lo]

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>
>


-- 

Brandon Lo

UNH InterOperability Laboratory

21 Madbury Rd, Suite 100, Durham, NH 03824

blo@iol.unh.edu

www.iol.unh.edu

Re: [dpdk-stable] [dpdklab] RE: [dpdk-dev] [PATCH v4] mbuf: fix reset on mbuf free

[Ferruh Yigit]

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>

Re: [dpdk-stable] [dpdk-dev] [PATCH v4] mbuf: fix reset on mbuf free

[Morten Brørup]

> 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()?

Re: [dpdk-stable] [dpdk-dev] [PATCH v4] mbuf: fix reset on mbuf free

[Thomas Monjalon]

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

Re: [dpdk-stable] [dpdk-dev] [PATCH v4] mbuf: fix reset on mbuf free

[Olivier Matz]

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
> 
> 
> 
> 

Re: [dpdk-stable] [dpdk-dev] [PATCH v4] mbuf: fix reset on mbuf free

[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.

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

Re: [dpdk-stable] [dpdk-dev] [PATCH v4] mbuf: fix reset on mbuf free

[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.
> 
> 
> -Morten

Re: [dpdk-stable] [dpdk-dev] [PATCH v4] mbuf: fix reset on mbuf free

[Olivier Matz]

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
> 
> 
> 

Re: [dpdk-stable] [dpdk-dev] [PATCH v4] mbuf: fix reset on mbuf free

[Morten Brørup]

> 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
> >
> >
> >

[dpdk-stable] [PATCH v5] mbuf: fix reset on mbuf free

[Olivier Matz]

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>
---
 app/test/test_mbuf.c     | 69 ++++++++++++++++++++++++++++++++++++++++
 lib/mbuf/rte_mbuf.c      |  4 +--
 lib/mbuf/rte_mbuf.h      |  8 ++---
 lib/mbuf/rte_mbuf_core.h | 13 ++++++--
 4 files changed, 86 insertions(+), 8 deletions(-)

diff --git a/app/test/test_mbuf.c b/app/test/test_mbuf.c
index 9a248dfaea..a4e32e933a 100644
--- a/app/test/test_mbuf.c
+++ b/app/test/test_mbuf.c
@@ -2684,6 +2684,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)
 {
@@ -2874,6 +2938,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:
diff --git a/lib/mbuf/rte_mbuf.c b/lib/mbuf/rte_mbuf.c
index f7e3c1a187..f145cd800a 100644
--- a/lib/mbuf/rte_mbuf.c
+++ b/lib/mbuf/rte_mbuf.c
@@ -134,10 +134,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/mbuf/rte_mbuf.h b/lib/mbuf/rte_mbuf.h
index a555f216ae..dded3dd70c 100644
--- a/lib/mbuf/rte_mbuf.h
+++ b/lib/mbuf/rte_mbuf.h
@@ -1346,10 +1346,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;
 
@@ -1363,10 +1363,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;
diff --git a/lib/mbuf/rte_mbuf_core.h b/lib/mbuf/rte_mbuf_core.h
index 9d8e3ddc86..9ef807daaa 100644
--- a/lib/mbuf/rte_mbuf_core.h
+++ b/lib/mbuf/rte_mbuf_core.h
@@ -508,7 +508,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.
@@ -604,7 +609,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
-- 
2.30.2

Re: [dpdk-stable] [PATCH v5] mbuf: fix reset on mbuf free

[Ali Alnubani]

> -----Original Message-----
> From: Olivier Matz <olivier.matz@6wind.com>
> Sent: Thursday, September 30, 2021 12:37 AM
> To: dev@dpdk.org
> Cc: ajit.khaparde@broadcom.com; ajitkhaparde@gmail.com; Ali Alnubani
> <alialnu@nvidia.com>; andrew.rybchenko@oktetlabs.ru;
> konstantin.ananyev@intel.com; mb@smartsharesystems.com;
> stable@dpdk.org; Slava Ovsiienko <viacheslavo@nvidia.com>
> Subject: [PATCH v5] mbuf: fix reset on mbuf free
> 
> 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>
> ---

Tested-by: Ali Alnubani <alialnu@nvidia.com>

Re: [dpdk-stable] [dpdk-dev] [PATCH v5] mbuf: fix reset on mbuf free

[David Marchand]

On Wed, Sep 29, 2021 at 11:37 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>
Tested-by: Ali Alnubani <alialnu@nvidia.com>

Applied, thanks.


-- 
David Marchand