patches for DPDK stable branches
 help / color / mirror / Atom feed
* [dpdk-stable] [PATCH] mbuf: fix reset on mbuf free
@ 2020-11-04 17:00 Olivier Matz
  2020-11-05  0:15 ` Ananyev, Konstantin
                   ` (4 more replies)
  0 siblings, 5 replies; 16+ messages in thread
From: Olivier Matz @ 2020-11-04 17:00 UTC (permalink / raw)
  To: dev; +Cc: konstantin.ananyev, stable

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


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [dpdk-stable] [PATCH] mbuf: fix reset on mbuf free
  2020-11-04 17:00 [dpdk-stable] [PATCH] mbuf: fix reset on mbuf free Olivier Matz
@ 2020-11-05  0:15 ` Ananyev, Konstantin
  2020-11-05  7:46   ` Olivier Matz
  2020-11-08  7:25 ` Ali Alnubani
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 16+ messages in thread
From: Ananyev, Konstantin @ 2020-11-05  0:15 UTC (permalink / raw)
  To: Olivier Matz, dev; +Cc: stable


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


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [dpdk-stable] [PATCH] mbuf: fix reset on mbuf free
  2020-11-05  0:15 ` Ananyev, Konstantin
@ 2020-11-05  7:46   ` Olivier Matz
  2020-11-05  8:33     ` [dpdk-stable] [dpdk-dev] " Morten Brørup
  2020-11-05  9:09     ` Andrew Rybchenko
  0 siblings, 2 replies; 16+ messages in thread
From: Olivier Matz @ 2020-11-05  7:46 UTC (permalink / raw)
  To: Ananyev, Konstantin; +Cc: dev, stable

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
> 

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [dpdk-stable] [dpdk-dev] [PATCH] mbuf: fix reset on mbuf free
  2020-11-05  7:46   ` Olivier Matz
@ 2020-11-05  8:33     ` Morten Brørup
  2020-11-05  9:03       ` Olivier Matz
  2020-11-05  9:09     ` Andrew Rybchenko
  1 sibling, 1 reply; 16+ messages in thread
From: Morten Brørup @ 2020-11-05  8:33 UTC (permalink / raw)
  To: Olivier Matz, Ananyev, Konstantin; +Cc: dev, stable

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


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [dpdk-stable] [dpdk-dev] [PATCH] mbuf: fix reset on mbuf free
  2020-11-05  8:33     ` [dpdk-stable] [dpdk-dev] " Morten Brørup
@ 2020-11-05  9:03       ` Olivier Matz
  0 siblings, 0 replies; 16+ messages in thread
From: Olivier Matz @ 2020-11-05  9:03 UTC (permalink / raw)
  To: Morten Brørup; +Cc: Ananyev, Konstantin, dev, stable

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [dpdk-stable] [dpdk-dev] [PATCH] mbuf: fix reset on mbuf free
  2020-11-05  7:46   ` Olivier Matz
  2020-11-05  8:33     ` [dpdk-stable] [dpdk-dev] " Morten Brørup
@ 2020-11-05  9:09     ` Andrew Rybchenko
  1 sibling, 0 replies; 16+ messages in thread
From: Andrew Rybchenko @ 2020-11-05  9:09 UTC (permalink / raw)
  To: dev; +Cc: Morten Brørup, Ananyev, Konstantin, dpdk stable, Olivier Matz

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


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [dpdk-stable] [dpdk-dev] [PATCH] mbuf: fix reset on mbuf free
  2020-11-04 17:00 [dpdk-stable] [PATCH] mbuf: fix reset on mbuf free Olivier Matz
  2020-11-05  0:15 ` Ananyev, Konstantin
@ 2020-11-08  7:25 ` Ali Alnubani
  2020-12-18 12:52 ` [dpdk-stable] [PATCH v2] " Olivier Matz
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 16+ messages in thread
From: Ali Alnubani @ 2020-11-08  7:25 UTC (permalink / raw)
  To: Olivier Matz, dev; +Cc: konstantin.ananyev, stable, David Marchand

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [dpdk-stable] [PATCH v2] mbuf: fix reset on mbuf free
  2020-11-04 17:00 [dpdk-stable] [PATCH] mbuf: fix reset on mbuf free Olivier Matz
  2020-11-05  0:15 ` Ananyev, Konstantin
  2020-11-08  7:25 ` Ali Alnubani
@ 2020-12-18 12:52 ` Olivier Matz
  2020-12-18 13:18   ` [dpdk-stable] [dpdk-dev] " Morten Brørup
  2021-01-06 13:33 ` [dpdk-stable] [PATCH v3] " Olivier Matz
  2021-01-13 13:27 ` [dpdk-stable] [PATCH v4] " Olivier Matz
  4 siblings, 1 reply; 16+ messages in thread
From: Olivier Matz @ 2020-12-18 12:52 UTC (permalink / raw)
  To: dev; +Cc: andrew.rybchenko, konstantin.ananyev, mb, stable

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


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [dpdk-stable] [dpdk-dev] [PATCH v2] mbuf: fix reset on mbuf free
  2020-12-18 12:52 ` [dpdk-stable] [PATCH v2] " Olivier Matz
@ 2020-12-18 13:18   ` Morten Brørup
  2020-12-18 23:33     ` Ajit Khaparde
  0 siblings, 1 reply; 16+ messages in thread
From: Morten Brørup @ 2020-12-18 13:18 UTC (permalink / raw)
  To: Olivier Matz, dev; +Cc: andrew.rybchenko, konstantin.ananyev, stable

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


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [dpdk-stable] [dpdk-dev] [PATCH v2] mbuf: fix reset on mbuf free
  2020-12-18 13:18   ` [dpdk-stable] [dpdk-dev] " Morten Brørup
@ 2020-12-18 23:33     ` Ajit Khaparde
  0 siblings, 0 replies; 16+ messages in thread
From: Ajit Khaparde @ 2020-12-18 23:33 UTC (permalink / raw)
  To: Morten Brørup
  Cc: Olivier Matz, dev, andrew.rybchenko, konstantin.ananyev, stable

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>

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [dpdk-stable] [PATCH v3] mbuf: fix reset on mbuf free
  2020-11-04 17:00 [dpdk-stable] [PATCH] mbuf: fix reset on mbuf free Olivier Matz
                   ` (2 preceding siblings ...)
  2020-12-18 12:52 ` [dpdk-stable] [PATCH v2] " Olivier Matz
@ 2021-01-06 13:33 ` Olivier Matz
  2021-01-10  9:28   ` Ali Alnubani
  2021-01-11 13:14   ` Ananyev, Konstantin
  2021-01-13 13:27 ` [dpdk-stable] [PATCH v4] " Olivier Matz
  4 siblings, 2 replies; 16+ messages in thread
From: Olivier Matz @ 2021-01-06 13:33 UTC (permalink / raw)
  To: dev
  Cc: andrew.rybchenko, konstantin.ananyev, mb, alialnu, ajitkhaparde,
	stable, Ajit Khaparde

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


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [dpdk-stable] [PATCH v3] mbuf: fix reset on mbuf free
  2021-01-06 13:33 ` [dpdk-stable] [PATCH v3] " Olivier Matz
@ 2021-01-10  9:28   ` Ali Alnubani
  2021-01-11 13:14   ` Ananyev, Konstantin
  1 sibling, 0 replies; 16+ messages in thread
From: Ali Alnubani @ 2021-01-10  9:28 UTC (permalink / raw)
  To: Olivier Matz, dev
  Cc: andrew.rybchenko, konstantin.ananyev, mb, ajitkhaparde, stable,
	Ajit Khaparde

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 

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [dpdk-stable] [PATCH v3] mbuf: fix reset on mbuf free
  2021-01-06 13:33 ` [dpdk-stable] [PATCH v3] " Olivier Matz
  2021-01-10  9:28   ` Ali Alnubani
@ 2021-01-11 13:14   ` Ananyev, Konstantin
  1 sibling, 0 replies; 16+ messages in thread
From: Ananyev, Konstantin @ 2021-01-11 13:14 UTC (permalink / raw)
  To: Olivier Matz, dev
  Cc: andrew.rybchenko, mb, alialnu, ajitkhaparde, stable, Ajit Khaparde

> 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


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [dpdk-stable] [PATCH v4] mbuf: fix reset on mbuf free
  2020-11-04 17:00 [dpdk-stable] [PATCH] mbuf: fix reset on mbuf free Olivier Matz
                   ` (3 preceding siblings ...)
  2021-01-06 13:33 ` [dpdk-stable] [PATCH v3] " Olivier Matz
@ 2021-01-13 13:27 ` Olivier Matz
  2021-01-15 13:59   ` David Marchand
  4 siblings, 1 reply; 16+ messages in thread
From: Olivier Matz @ 2021-01-13 13:27 UTC (permalink / raw)
  To: dev
  Cc: andrew.rybchenko, konstantin.ananyev, mb, alialnu, ajitkhaparde,
	stable, Ajit Khaparde

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


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [dpdk-stable] [PATCH v4] mbuf: fix reset on mbuf free
  2021-01-13 13:27 ` [dpdk-stable] [PATCH v4] " Olivier Matz
@ 2021-01-15 13:59   ` David Marchand
  2021-01-15 18:39     ` Ali Alnubani
  0 siblings, 1 reply; 16+ messages in thread
From: David Marchand @ 2021-01-15 13:59 UTC (permalink / raw)
  To: Olivier Matz, Ali Alnubani
  Cc: dev, Andrew Rybchenko, Ananyev, Konstantin, Morten Brørup,
	ajitkhaparde, dpdk stable, Ajit Khaparde

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


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [dpdk-stable] [PATCH v4] mbuf: fix reset on mbuf free
  2021-01-15 13:59   ` David Marchand
@ 2021-01-15 18:39     ` Ali Alnubani
  0 siblings, 0 replies; 16+ messages in thread
From: Ali Alnubani @ 2021-01-15 18:39 UTC (permalink / raw)
  To: David Marchand, Olivier Matz, Ferruh Yigit, zhaoyan.chen
  Cc: dev, Andrew Rybchenko, Ananyev, Konstantin, Morten Brørup,
	ajitkhaparde, dpdk stable, Ajit Khaparde

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2021-01-15 18:39 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-04 17:00 [dpdk-stable] [PATCH] mbuf: fix reset on mbuf free Olivier Matz
2020-11-05  0:15 ` Ananyev, Konstantin
2020-11-05  7:46   ` Olivier Matz
2020-11-05  8:33     ` [dpdk-stable] [dpdk-dev] " Morten Brørup
2020-11-05  9:03       ` Olivier Matz
2020-11-05  9:09     ` Andrew Rybchenko
2020-11-08  7:25 ` Ali Alnubani
2020-12-18 12:52 ` [dpdk-stable] [PATCH v2] " Olivier Matz
2020-12-18 13:18   ` [dpdk-stable] [dpdk-dev] " Morten Brørup
2020-12-18 23:33     ` Ajit Khaparde
2021-01-06 13:33 ` [dpdk-stable] [PATCH v3] " Olivier Matz
2021-01-10  9:28   ` Ali Alnubani
2021-01-11 13:14   ` Ananyev, Konstantin
2021-01-13 13:27 ` [dpdk-stable] [PATCH v4] " Olivier Matz
2021-01-15 13:59   ` David Marchand
2021-01-15 18:39     ` Ali Alnubani

patches for DPDK stable branches

This inbox may be cloned and mirrored by anyone:

	git clone --mirror http://inbox.dpdk.org/stable/0 stable/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 stable stable/ http://inbox.dpdk.org/stable \
		stable@dpdk.org
	public-inbox-index stable

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.stable


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git