[PATCH v1] mbuf: remove the redundant code for mbuf prefree

[PATCH v1] mbuf: remove the redundant code for mbuf prefree

[Feifei Wang]

For 'rte_pktmbuf_prefree_seg' function, 'rte_mbuf_refcnt_read(m) == 1'
and '__rte_mbuf_refcnt_update(m, -1) == 0' are the same cases where
mbuf's refcnt value should be 1. Thus we can simplify the code and
remove the redundant part.

Furthermore, according to [1], when the mbuf is stored inside the
mempool, the m->refcnt value should be 1. And then it is detached
from its parent for an indirect mbuf. Thus change the description of
'rte_pktmbuf_prefree_seg' function.

[1] https://patches.dpdk.org/project/dpdk/patch/20170404162807.20157-4-olivier.matz@6wind.com/

Suggested-by: Ruifeng Wang <ruifeng.wang@arm.com>
Signed-off-by: Feifei Wang <feifei.wang2@arm.com>
---
 lib/mbuf/rte_mbuf.h | 22 +++-------------------
 1 file changed, 3 insertions(+), 19 deletions(-)

diff --git a/lib/mbuf/rte_mbuf.h b/lib/mbuf/rte_mbuf.h
index 286b32b788..42e9b50d51 100644
--- a/lib/mbuf/rte_mbuf.h
+++ b/lib/mbuf/rte_mbuf.h
@@ -1328,7 +1328,7 @@ static inline int __rte_pktmbuf_pinned_extbuf_decref(struct rte_mbuf *m)
  *
  * This function does the same than a free, except that it does not
  * return the segment to its pool.
- * It decreases the reference counter, and if it reaches 0, it is
+ * It decreases the reference counter, and if it reaches 1, it is
  * detached from its parent for an indirect mbuf.
  *
  * @param m
@@ -1358,25 +1358,9 @@ rte_pktmbuf_prefree_seg(struct rte_mbuf *m)
 			m->nb_segs = 1;
 
 		return m;
-
-	} else if (__rte_mbuf_refcnt_update(m, -1) == 0) {
-
-		if (!RTE_MBUF_DIRECT(m)) {
-			rte_pktmbuf_detach(m);
-			if (RTE_MBUF_HAS_EXTBUF(m) &&
-			    RTE_MBUF_HAS_PINNED_EXTBUF(m) &&
-			    __rte_pktmbuf_pinned_extbuf_decref(m))
-				return 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;
 	}
+
+	__rte_mbuf_refcnt_update(m, -1);
 	return NULL;
 }
 
-- 
2.25.1

RE: [PATCH v1] mbuf: remove the redundant code for mbuf prefree

[Morten Brørup]

> From: Feifei Wang [mailto:feifei.wang2@arm.com]
> Sent: Monday, 4 December 2023 03.39
> 
> For 'rte_pktmbuf_prefree_seg' function, 'rte_mbuf_refcnt_read(m) == 1'
> and '__rte_mbuf_refcnt_update(m, -1) == 0' are the same cases where
> mbuf's refcnt value should be 1. Thus we can simplify the code and
> remove the redundant part.
> 
> Furthermore, according to [1], when the mbuf is stored inside the
> mempool, the m->refcnt value should be 1. And then it is detached
> from its parent for an indirect mbuf. Thus change the description of
> 'rte_pktmbuf_prefree_seg' function.
> 
> [1] https://patches.dpdk.org/project/dpdk/patch/20170404162807.20157-4-
> olivier.matz@6wind.com/
> 
> Suggested-by: Ruifeng Wang <ruifeng.wang@arm.com>
> Signed-off-by: Feifei Wang <feifei.wang2@arm.com>
> ---
>  lib/mbuf/rte_mbuf.h | 22 +++-------------------
>  1 file changed, 3 insertions(+), 19 deletions(-)
> 
> diff --git a/lib/mbuf/rte_mbuf.h b/lib/mbuf/rte_mbuf.h
> index 286b32b788..42e9b50d51 100644
> --- a/lib/mbuf/rte_mbuf.h
> +++ b/lib/mbuf/rte_mbuf.h
> @@ -1328,7 +1328,7 @@ static inline int
> __rte_pktmbuf_pinned_extbuf_decref(struct rte_mbuf *m)
>   *
>   * This function does the same than a free, except that it does not
>   * return the segment to its pool.
> - * It decreases the reference counter, and if it reaches 0, it is
> + * It decreases the reference counter, and if it reaches 1, it is

No, the original description is correct.
However, the reference counter is set to 1 when put back in the pool, as a shortcut so it isn't needed to be set back to 1 when allocated from the pool.

>   * detached from its parent for an indirect mbuf.
>   *
>   * @param m
> @@ -1358,25 +1358,9 @@ rte_pktmbuf_prefree_seg(struct rte_mbuf *m)

The preceding "if (likely(rte_mbuf_refcnt_read(m) == 1)) {" is only a shortcut for the likely case.

>  			m->nb_segs = 1;
> 
>  		return m;
> -
> -	} else if (__rte_mbuf_refcnt_update(m, -1) == 0) {
> -
> -		if (!RTE_MBUF_DIRECT(m)) {
> -			rte_pktmbuf_detach(m);
> -			if (RTE_MBUF_HAS_EXTBUF(m) &&
> -			    RTE_MBUF_HAS_PINNED_EXTBUF(m) &&
> -			    __rte_pktmbuf_pinned_extbuf_decref(m))
> -				return 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;
>  	}
> +
> +	__rte_mbuf_refcnt_update(m, -1);
>  	return NULL;
>  }
> 
> --
> 2.25.1

NAK.

This patch is not race safe. With the patch:

This thread:
if (likely(rte_mbuf_refcnt_read(m) == 1)) { // Assume it's 2.

The other thread:
if (likely(rte_mbuf_refcnt_read(m) == 1)) { // It's 2.
__rte_mbuf_refcnt_update(m, -1); // Now it's 1.
return NULL;

This thread:
__rte_mbuf_refcnt_update(m, -1); // Now it's 0.
return NULL;

None of the threads have done the "prefree" work.

RE: [PATCH v1] mbuf: remove the redundant code for mbuf prefree

[Konstantin Ananyev]

> > For 'rte_pktmbuf_prefree_seg' function, 'rte_mbuf_refcnt_read(m) == 1'
> > and '__rte_mbuf_refcnt_update(m, -1) == 0' are the same cases where
> > mbuf's refcnt value should be 1. Thus we can simplify the code and
> > remove the redundant part.
> >
> > Furthermore, according to [1], when the mbuf is stored inside the
> > mempool, the m->refcnt value should be 1. And then it is detached
> > from its parent for an indirect mbuf. Thus change the description of
> > 'rte_pktmbuf_prefree_seg' function.
> >
> > [1] https://patches.dpdk.org/project/dpdk/patch/20170404162807.20157-4-
> > olivier.matz@6wind.com/
> >
> > Suggested-by: Ruifeng Wang <ruifeng.wang@arm.com>
> > Signed-off-by: Feifei Wang <feifei.wang2@arm.com>
> > ---
> >  lib/mbuf/rte_mbuf.h | 22 +++-------------------
> >  1 file changed, 3 insertions(+), 19 deletions(-)
> >
> > diff --git a/lib/mbuf/rte_mbuf.h b/lib/mbuf/rte_mbuf.h
> > index 286b32b788..42e9b50d51 100644
> > --- a/lib/mbuf/rte_mbuf.h
> > +++ b/lib/mbuf/rte_mbuf.h
> > @@ -1328,7 +1328,7 @@ static inline int
> > __rte_pktmbuf_pinned_extbuf_decref(struct rte_mbuf *m)
> >   *
> >   * This function does the same than a free, except that it does not
> >   * return the segment to its pool.
> > - * It decreases the reference counter, and if it reaches 0, it is
> > + * It decreases the reference counter, and if it reaches 1, it is
> 
> No, the original description is correct.
> However, the reference counter is set to 1 when put back in the pool, as a shortcut so it isn't needed to be set back to 1 when
> allocated from the pool.
> 
> >   * detached from its parent for an indirect mbuf.
> >   *
> >   * @param m
> > @@ -1358,25 +1358,9 @@ rte_pktmbuf_prefree_seg(struct rte_mbuf *m)
> 
> The preceding "if (likely(rte_mbuf_refcnt_read(m) == 1)) {" is only a shortcut for the likely case.
> 
> >  			m->nb_segs = 1;
> >
> >  		return m;
> > -
> > -	} else if (__rte_mbuf_refcnt_update(m, -1) == 0) {
> > -
> > -		if (!RTE_MBUF_DIRECT(m)) {
> > -			rte_pktmbuf_detach(m);
> > -			if (RTE_MBUF_HAS_EXTBUF(m) &&
> > -			    RTE_MBUF_HAS_PINNED_EXTBUF(m) &&
> > -			    __rte_pktmbuf_pinned_extbuf_decref(m))
> > -				return 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;
> >  	}
> > +
> > +	__rte_mbuf_refcnt_update(m, -1);
> >  	return NULL;
> >  }
> >
> > --
> > 2.25.1
> 
> NAK.
> 
> This patch is not race safe. 

+1, It is a bad idea.

> With the patch:
> 
> This thread:
> if (likely(rte_mbuf_refcnt_read(m) == 1)) { // Assume it's 2.
> 
> The other thread:
> if (likely(rte_mbuf_refcnt_read(m) == 1)) { // It's 2.
> __rte_mbuf_refcnt_update(m, -1); // Now it's 1.
> return NULL;
> 
> This thread:
> __rte_mbuf_refcnt_update(m, -1); // Now it's 0.
> return NULL;
> 
> None of the threads have done the "prefree" work.

Re: [PATCH v1] mbuf: remove the redundant code for mbuf prefree

[Stephen Hemminger]

On Mon, 4 Dec 2023 11:07:08 +0000
Konstantin Ananyev <konstantin.ananyev@huawei.com> wrote:

> > > 2.25.1  
> > 
> > NAK.
> > 
> > This patch is not race safe.   
> 
> +1, It is a bad idea.

The patch does raise a couple of issues that could be addressed by
rearranging. There is duplicate code, and there are no comments
to explain the rationale.

Maybe something like the following (untested):

diff --git a/lib/mbuf/rte_mbuf.h b/lib/mbuf/rte_mbuf.h
index 286b32b788a5..b43c055fbe3f 100644
--- a/lib/mbuf/rte_mbuf.h
+++ b/lib/mbuf/rte_mbuf.h
@@ -1342,42 +1342,32 @@ rte_pktmbuf_prefree_seg(struct rte_mbuf *m)
 {
 	__rte_mbuf_sanity_check(m, 0);
 
-	if (likely(rte_mbuf_refcnt_read(m) == 1)) {
-
-		if (!RTE_MBUF_DIRECT(m)) {
-			rte_pktmbuf_detach(m);
-			if (RTE_MBUF_HAS_EXTBUF(m) &&
-			    RTE_MBUF_HAS_PINNED_EXTBUF(m) &&
-			    __rte_pktmbuf_pinned_extbuf_decref(m))
-				return NULL;
-		}
-
-		if (m->next != NULL)
-			m->next = NULL;
-		if (m->nb_segs != 1)
-			m->nb_segs = 1;
-
-		return m;
-
-	} else if (__rte_mbuf_refcnt_update(m, -1) == 0) {
-
-		if (!RTE_MBUF_DIRECT(m)) {
-			rte_pktmbuf_detach(m);
-			if (RTE_MBUF_HAS_EXTBUF(m) &&
-			    RTE_MBUF_HAS_PINNED_EXTBUF(m) &&
-			    __rte_pktmbuf_pinned_extbuf_decref(m))
-				return NULL;
-		}
-
-		if (m->next != NULL)
-			m->next = NULL;
-		if (m->nb_segs != 1)
-			m->nb_segs = 1;
+	if (likely(rte_mbuf_refcnt_read(m) != 1) ) {
+		/* If this is the only reference to the mbuf it can just
+		 * be setup for reuse without modifying reference count.
+		 */
+	} else if (unlikely(__rte_mbuf_refcnt_update(m, -1) == 0)) {
+		/* This was last reference reset to 1 for recycling/free. */
 		rte_mbuf_refcnt_set(m, 1);
+	} else {
+		/* mbuf is still in use */
+		return NULL;
+	}
 
-		return m;
+	if (!RTE_MBUF_DIRECT(m)) {
+		rte_pktmbuf_detach(m);
+		if (RTE_MBUF_HAS_EXTBUF(m) &&
+		    RTE_MBUF_HAS_PINNED_EXTBUF(m) &&
+		    __rte_pktmbuf_pinned_extbuf_decref(m))
+
+		return NULL;
 	}
-	return NULL;
+
+	if (m->next != NULL)
+		m->next = NULL;
+	if (m->nb_segs != 1)
+		m->nb_segs = 1;
+	return m;
 }
 
 /**

Re: [PATCH v1] mbuf: remove the redundant code for mbuf prefree

[Bruce Richardson]

On Tue, Dec 05, 2023 at 10:50:32AM -0800, Stephen Hemminger wrote:
> On Mon, 4 Dec 2023 11:07:08 +0000
> Konstantin Ananyev <konstantin.ananyev@huawei.com> wrote:
> 
> > > > 2.25.1  
> > > 
> > > NAK.
> > > 
> > > This patch is not race safe.   
> > 
> > +1, It is a bad idea.
> 
> The patch does raise a couple of issues that could be addressed by
> rearranging. There is duplicate code, and there are no comments
> to explain the rationale.
> 
> Maybe something like the following (untested):
> 
> diff --git a/lib/mbuf/rte_mbuf.h b/lib/mbuf/rte_mbuf.h
> index 286b32b788a5..b43c055fbe3f 100644
> --- a/lib/mbuf/rte_mbuf.h
> +++ b/lib/mbuf/rte_mbuf.h
> @@ -1342,42 +1342,32 @@ rte_pktmbuf_prefree_seg(struct rte_mbuf *m)
>  {
>  	__rte_mbuf_sanity_check(m, 0);
>  
> -	if (likely(rte_mbuf_refcnt_read(m) == 1)) {
> -
> -		if (!RTE_MBUF_DIRECT(m)) {
> -			rte_pktmbuf_detach(m);
> -			if (RTE_MBUF_HAS_EXTBUF(m) &&
> -			    RTE_MBUF_HAS_PINNED_EXTBUF(m) &&
> -			    __rte_pktmbuf_pinned_extbuf_decref(m))
> -				return NULL;
> -		}
> -
> -		if (m->next != NULL)
> -			m->next = NULL;
> -		if (m->nb_segs != 1)
> -			m->nb_segs = 1;
> -
> -		return m;
> -
> -	} else if (__rte_mbuf_refcnt_update(m, -1) == 0) {
> -
> -		if (!RTE_MBUF_DIRECT(m)) {
> -			rte_pktmbuf_detach(m);
> -			if (RTE_MBUF_HAS_EXTBUF(m) &&
> -			    RTE_MBUF_HAS_PINNED_EXTBUF(m) &&
> -			    __rte_pktmbuf_pinned_extbuf_decref(m))
> -				return NULL;
> -		}
> -
> -		if (m->next != NULL)
> -			m->next = NULL;
> -		if (m->nb_segs != 1)
> -			m->nb_segs = 1;
> +	if (likely(rte_mbuf_refcnt_read(m) != 1) ) {

== 1

> +		/* If this is the only reference to the mbuf it can just
> +		 * be setup for reuse without modifying reference count.
> +		 */
> +	} else if (unlikely(__rte_mbuf_refcnt_update(m, -1) == 0)) {
> +		/* This was last reference reset to 1 for recycling/free. */
>  		rte_mbuf_refcnt_set(m, 1);
> +	} else {
> +		/* mbuf is still in use */
> +		return NULL;
> +	}
>  

This seems much clearer with good comments.

> -		return m;
> +	if (!RTE_MBUF_DIRECT(m)) {
> +		rte_pktmbuf_detach(m);
> +		if (RTE_MBUF_HAS_EXTBUF(m) &&
> +		    RTE_MBUF_HAS_PINNED_EXTBUF(m) &&
> +		    __rte_pktmbuf_pinned_extbuf_decref(m))
> +
> +		return NULL;
>  	}
> -	return NULL;
> +
> +	if (m->next != NULL)
> +		m->next = NULL;
> +	if (m->nb_segs != 1)
> +		m->nb_segs = 1;
> +	return m;
>  }
>  
>  /**

RE: [PATCH v1] mbuf: remove the redundant code for mbuf prefree

[Konstantin Ananyev]

> > > >
> > > > NAK.
> > > >
> > > > This patch is not race safe.
> > >
> > > +1, It is a bad idea.
> >
> > The patch does raise a couple of issues that could be addressed by
> > rearranging. There is duplicate code, and there are no comments
> > to explain the rationale.
> >
> > Maybe something like the following (untested):
> >
> > diff --git a/lib/mbuf/rte_mbuf.h b/lib/mbuf/rte_mbuf.h
> > index 286b32b788a5..b43c055fbe3f 100644
> > --- a/lib/mbuf/rte_mbuf.h
> > +++ b/lib/mbuf/rte_mbuf.h
> > @@ -1342,42 +1342,32 @@ rte_pktmbuf_prefree_seg(struct rte_mbuf *m)
> >  {
> >  	__rte_mbuf_sanity_check(m, 0);
> >
> > -	if (likely(rte_mbuf_refcnt_read(m) == 1)) {
> > -
> > -		if (!RTE_MBUF_DIRECT(m)) {
> > -			rte_pktmbuf_detach(m);
> > -			if (RTE_MBUF_HAS_EXTBUF(m) &&
> > -			    RTE_MBUF_HAS_PINNED_EXTBUF(m) &&
> > -			    __rte_pktmbuf_pinned_extbuf_decref(m))
> > -				return NULL;
> > -		}
> > -
> > -		if (m->next != NULL)
> > -			m->next = NULL;
> > -		if (m->nb_segs != 1)
> > -			m->nb_segs = 1;
> > -
> > -		return m;
> > -
> > -	} else if (__rte_mbuf_refcnt_update(m, -1) == 0) {
> > -
> > -		if (!RTE_MBUF_DIRECT(m)) {
> > -			rte_pktmbuf_detach(m);
> > -			if (RTE_MBUF_HAS_EXTBUF(m) &&
> > -			    RTE_MBUF_HAS_PINNED_EXTBUF(m) &&
> > -			    __rte_pktmbuf_pinned_extbuf_decref(m))
> > -				return NULL;
> > -		}
> > -
> > -		if (m->next != NULL)
> > -			m->next = NULL;
> > -		if (m->nb_segs != 1)
> > -			m->nb_segs = 1;
> > +	if (likely(rte_mbuf_refcnt_read(m) != 1) ) {
> 
> == 1
> 
> > +		/* If this is the only reference to the mbuf it can just
> > +		 * be setup for reuse without modifying reference count.
> > +		 */
> > +	} else if (unlikely(__rte_mbuf_refcnt_update(m, -1) == 0)) {
> > +		/* This was last reference reset to 1 for recycling/free. */
> >  		rte_mbuf_refcnt_set(m, 1);
> > +	} else {
> > +		/* mbuf is still in use */
> > +		return NULL;
> > +	}
> >
> 
> This seems much clearer with good comments.

Then, it could be just:

/* put whatever likely/unlikely we believe would be the most common case */
if (rte_mbuf_refcnt_read(m) != 1 && __rte_mbuf_refcnt_update(m, -1) != 0)
   return NULL;

/* do whatever cleanup is necessary */
 



> 
> > -		return m;
> > +	if (!RTE_MBUF_DIRECT(m)) {
> > +		rte_pktmbuf_detach(m);
> > +		if (RTE_MBUF_HAS_EXTBUF(m) &&
> > +		    RTE_MBUF_HAS_PINNED_EXTBUF(m) &&
> > +		    __rte_pktmbuf_pinned_extbuf_decref(m))
> > +
> > +		return NULL;
> >  	}
> > -	return NULL;
> > +
> > +	if (m->next != NULL)
> > +		m->next = NULL;
> > +	if (m->nb_segs != 1)
> > +		m->nb_segs = 1;
> > +	return m;
> >  }
> >
> >  /**

Re: [PATCH v1] mbuf: remove the redundant code for mbuf prefree

[Feifei Wang]

在 2023/12/4 15:41, Morten Brørup 写道:
>> From: Feifei Wang [mailto:feifei.wang2@arm.com]
>> Sent: Monday, 4 December 2023 03.39
>>
>> For 'rte_pktmbuf_prefree_seg' function, 'rte_mbuf_refcnt_read(m) == 1'
>> and '__rte_mbuf_refcnt_update(m, -1) == 0' are the same cases where
>> mbuf's refcnt value should be 1. Thus we can simplify the code and
>> remove the redundant part.
>>
>> Furthermore, according to [1], when the mbuf is stored inside the
>> mempool, the m->refcnt value should be 1. And then it is detached
>> from its parent for an indirect mbuf. Thus change the description of
>> 'rte_pktmbuf_prefree_seg' function.
>>
>> [1] https://patches.dpdk.org/project/dpdk/patch/20170404162807.20157-4-
>> olivier.matz@6wind.com/
>>
>> Suggested-by: Ruifeng Wang <ruifeng.wang@arm.com>
>> Signed-off-by: Feifei Wang <feifei.wang2@arm.com>
>> ---
>>   lib/mbuf/rte_mbuf.h | 22 +++-------------------
>>   1 file changed, 3 insertions(+), 19 deletions(-)
>>
>> diff --git a/lib/mbuf/rte_mbuf.h b/lib/mbuf/rte_mbuf.h
>> index 286b32b788..42e9b50d51 100644
>> --- a/lib/mbuf/rte_mbuf.h
>> +++ b/lib/mbuf/rte_mbuf.h
>> @@ -1328,7 +1328,7 @@ static inline int
>> __rte_pktmbuf_pinned_extbuf_decref(struct rte_mbuf *m)
>>    *
>>    * This function does the same than a free, except that it does not
>>    * return the segment to its pool.
>> - * It decreases the reference counter, and if it reaches 0, it is
>> + * It decreases the reference counter, and if it reaches 1, it is
> No, the original description is correct.
> However, the reference counter is set to 1 when put back in the pool, as a shortcut so it isn't needed to be set back to 1 when allocated from the pool.

Ok.

for 'else if (__rte_mbuf_refcnt_update(m, -1) == 0)' case, it is easy to 
understand.

but for '(likely(rte_mbuf_refcnt_read(m) == 1))' case, I think this will 
create misleading. dpdk users maybe difficult to understand why the code 
can not match the function description.

Maybe we need some explanation here?

>>    * detached from its parent for an indirect mbuf.
>>    *
>>    * @param m
>> @@ -1358,25 +1358,9 @@ rte_pktmbuf_prefree_seg(struct rte_mbuf *m)
> The preceding "if (likely(rte_mbuf_refcnt_read(m) == 1)) {" is only a shortcut for the likely case.
Ok.
>>   			m->nb_segs = 1;
>>
>>   		return m;
>> -
>> -	} else if (__rte_mbuf_refcnt_update(m, -1) == 0) {
>> -
>> -		if (!RTE_MBUF_DIRECT(m)) {
>> -			rte_pktmbuf_detach(m);
>> -			if (RTE_MBUF_HAS_EXTBUF(m) &&
>> -			    RTE_MBUF_HAS_PINNED_EXTBUF(m) &&
>> -			    __rte_pktmbuf_pinned_extbuf_decref(m))
>> -				return 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;
>>   	}
>> +
>> +	__rte_mbuf_refcnt_update(m, -1);
>>   	return NULL;
>>   }
>>
>> --
>> 2.25.1
> NAK.
>
> This patch is not race safe. With the patch:
>
> This thread:
> if (likely(rte_mbuf_refcnt_read(m) == 1)) { // Assume it's 2.
>
> The other thread:
> if (likely(rte_mbuf_refcnt_read(m) == 1)) { // It's 2.
> __rte_mbuf_refcnt_update(m, -1); // Now it's 1.
> return NULL;
>
> This thread:
> __rte_mbuf_refcnt_update(m, -1); // Now it's 0.
> return NULL;
>
> None of the threads have done the "prefree" work.
>
Agree. After we see the failture of unit_test, we realize that we 
ignored the mutiple thread case.

Also maybe we need to add extra descripion to avoid misleading？

RE: [PATCH v1] mbuf: remove the redundant code for mbuf prefree

[Morten Brørup]

> From: Feifei Wang [mailto:feifei.wang2@arm.com]
> Sent: Tuesday, 5 December 2023 04.13
> 
> 在 2023/12/4 15:41, Morten Brørup 写道:
> >> From: Feifei Wang [mailto:feifei.wang2@arm.com]
> >> Sent: Monday, 4 December 2023 03.39
> >>
> >> For 'rte_pktmbuf_prefree_seg' function, 'rte_mbuf_refcnt_read(m) ==
> 1'
> >> and '__rte_mbuf_refcnt_update(m, -1) == 0' are the same cases where
> >> mbuf's refcnt value should be 1. Thus we can simplify the code and
> >> remove the redundant part.
> >>
> >> Furthermore, according to [1], when the mbuf is stored inside the
> >> mempool, the m->refcnt value should be 1. And then it is detached
> >> from its parent for an indirect mbuf. Thus change the description of
> >> 'rte_pktmbuf_prefree_seg' function.
> >>
> >> [1]
> https://patches.dpdk.org/project/dpdk/patch/20170404162807.20157-4-
> >> olivier.matz@6wind.com/
> >>
> >> Suggested-by: Ruifeng Wang <ruifeng.wang@arm.com>
> >> Signed-off-by: Feifei Wang <feifei.wang2@arm.com>
> >> ---
> >>   lib/mbuf/rte_mbuf.h | 22 +++-------------------
> >>   1 file changed, 3 insertions(+), 19 deletions(-)
> >>
> >> diff --git a/lib/mbuf/rte_mbuf.h b/lib/mbuf/rte_mbuf.h
> >> index 286b32b788..42e9b50d51 100644
> >> --- a/lib/mbuf/rte_mbuf.h
> >> +++ b/lib/mbuf/rte_mbuf.h
> >> @@ -1328,7 +1328,7 @@ static inline int
> >> __rte_pktmbuf_pinned_extbuf_decref(struct rte_mbuf *m)
> >>    *
> >>    * This function does the same than a free, except that it does
> not
> >>    * return the segment to its pool.
> >> - * It decreases the reference counter, and if it reaches 0, it is
> >> + * It decreases the reference counter, and if it reaches 1, it is
> > No, the original description is correct.
> > However, the reference counter is set to 1 when put back in the pool,
> as a shortcut so it isn't needed to be set back to 1 when allocated
> from the pool.
> 
> Ok.
> 
> for 'else if (__rte_mbuf_refcnt_update(m, -1) == 0)' case, it is easy
> to
> understand.
> 
> but for '(likely(rte_mbuf_refcnt_read(m) == 1))' case, I think this
> will
> create misleading. dpdk users maybe difficult to understand why the
> code
> can not match the function description.
> 
> Maybe we need some explanation here?

Agree. It is quite counterintuitive (but a clever optimization!) that the reference counter is 1 instead of 0 when free.

Something like:

static __rte_always_inline struct rte_mbuf *
rte_pktmbuf_prefree_seg(struct rte_mbuf *m)
{
	__rte_mbuf_sanity_check(m, 0);

	if (likely(rte_mbuf_refcnt_read(m) == 1)) {
+		/* This branch is a performance optimized variant of the branch below.
+		 * If the reference counter would reach 0 when decrementing it,
+		 * do not decrement it to 0 and then initialize it to 1;
+		 * just leave it at 1, thereby avoiding writing to memory.
+		 */

		if (!RTE_MBUF_DIRECT(m)) {
			rte_pktmbuf_detach(m);
			if (RTE_MBUF_HAS_EXTBUF(m) &&
			    RTE_MBUF_HAS_PINNED_EXTBUF(m) &&
			    __rte_pktmbuf_pinned_extbuf_decref(m))
				return NULL;
		}

		if (m->next != NULL)
			m->next = NULL;
		if (m->nb_segs != 1)
			m->nb_segs = 1;
+		/* No need to initialize the reference counter; it is already 1. */

		return m;

	} else if (__rte_mbuf_refcnt_update(m, -1) == 0) {

		if (!RTE_MBUF_DIRECT(m)) {
			rte_pktmbuf_detach(m);
			if (RTE_MBUF_HAS_EXTBUF(m) &&
			    RTE_MBUF_HAS_PINNED_EXTBUF(m) &&
			    __rte_pktmbuf_pinned_extbuf_decref(m))
				return NULL;
		}

		if (m->next != NULL)
			m->next = NULL;
		if (m->nb_segs != 1)
			m->nb_segs = 1;
+		/* Initialize the reference counter to 1, so
+		 * incrementing it is unnecessary when allocating the mbuf.
+		 */
		rte_mbuf_refcnt_set(m, 1);

		return m;
	}
	return NULL;
}


Alternatively, add a function to do the initialization work:

static __rte_always_inline struct rte_mbuf *
rte_pktmbuf_prefree_seg_last_ref(struct rte_mbuf *m, const bool init_refcnt)
{
	if (!RTE_MBUF_DIRECT(m)) {
		rte_pktmbuf_detach(m);
		if (RTE_MBUF_HAS_EXTBUF(m) &&
		    RTE_MBUF_HAS_PINNED_EXTBUF(m) &&
		    __rte_pktmbuf_pinned_extbuf_decref(m))
			return NULL;
	}

	if (m->next != NULL)
		m->next = NULL;
	if (m->nb_segs != 1)
		m->nb_segs = 1;

+	/* The reference counter must be initialized to 1 when the mbuf is free,
+	 * so incrementing to 1 is unnecessary when allocating the mbuf.
+	 */
	if (init_refcnt)
		rte_mbuf_refcnt_set(m, 1);
}

static __rte_always_inline struct rte_mbuf *
rte_pktmbuf_prefree_seg(struct rte_mbuf *m)
{
	__rte_mbuf_sanity_check(m, 0);

	if (likely(rte_mbuf_refcnt_read(m) == 1)) {
+		/* This branch is a performance optimized variant of the branch below.
+		 * If the reference counter would reach 0 when decrementing it,
+		 * do not decrement it to 0 and then initialize it to 1;
+		 * just leave it at 1, thereby avoiding writing to memory.
+		 */
		return rte_pktmbuf_prefree_seg_last_ref(m, false);
	} else if (__rte_mbuf_refcnt_update(m, -1) == 0) {
		return rte_pktmbuf_prefree_seg_last_ref(m, true);
	}
	return NULL;
}


And while we're at it, we could add unlikely() to the second comparison:
if (unlikely(__rte_mbuf_refcnt_update(m, -1) == 0))

Re: [PATCH v1] mbuf: remove the redundant code for mbuf prefree

[Bruce Richardson]

On Tue, Dec 05, 2023 at 09:04:08AM +0100, Morten Brørup wrote:
> > From: Feifei Wang [mailto:feifei.wang2@arm.com]
> > Sent: Tuesday, 5 December 2023 04.13
> > 
> > 在 2023/12/4 15:41, Morten Brørup 写道:
> > >> From: Feifei Wang [mailto:feifei.wang2@arm.com]
> > >> Sent: Monday, 4 December 2023 03.39
> > >>
> > >> For 'rte_pktmbuf_prefree_seg' function, 'rte_mbuf_refcnt_read(m) ==
> > 1'
> > >> and '__rte_mbuf_refcnt_update(m, -1) == 0' are the same cases where
> > >> mbuf's refcnt value should be 1. Thus we can simplify the code and
> > >> remove the redundant part.
> > >>
> > >> Furthermore, according to [1], when the mbuf is stored inside the
> > >> mempool, the m->refcnt value should be 1. And then it is detached
> > >> from its parent for an indirect mbuf. Thus change the description of
> > >> 'rte_pktmbuf_prefree_seg' function.
> > >>
> > >> [1]
> > https://patches.dpdk.org/project/dpdk/patch/20170404162807.20157-4-
> > >> olivier.matz@6wind.com/
> > >>
> > >> Suggested-by: Ruifeng Wang <ruifeng.wang@arm.com>
> > >> Signed-off-by: Feifei Wang <feifei.wang2@arm.com>
> > >> ---
> > >>   lib/mbuf/rte_mbuf.h | 22 +++-------------------
> > >>   1 file changed, 3 insertions(+), 19 deletions(-)
> > >>
> > >> diff --git a/lib/mbuf/rte_mbuf.h b/lib/mbuf/rte_mbuf.h
> > >> index 286b32b788..42e9b50d51 100644
> > >> --- a/lib/mbuf/rte_mbuf.h
> > >> +++ b/lib/mbuf/rte_mbuf.h
> > >> @@ -1328,7 +1328,7 @@ static inline int
> > >> __rte_pktmbuf_pinned_extbuf_decref(struct rte_mbuf *m)
> > >>    *
> > >>    * This function does the same than a free, except that it does
> > not
> > >>    * return the segment to its pool.
> > >> - * It decreases the reference counter, and if it reaches 0, it is
> > >> + * It decreases the reference counter, and if it reaches 1, it is
> > > No, the original description is correct.
> > > However, the reference counter is set to 1 when put back in the pool,
> > as a shortcut so it isn't needed to be set back to 1 when allocated
> > from the pool.
> > 
> > Ok.
> > 
> > for 'else if (__rte_mbuf_refcnt_update(m, -1) == 0)' case, it is easy
> > to
> > understand.
> > 
> > but for '(likely(rte_mbuf_refcnt_read(m) == 1))' case, I think this
> > will
> > create misleading. dpdk users maybe difficult to understand why the
> > code
> > can not match the function description.
> > 
> > Maybe we need some explanation here?
> 
> Agree. It is quite counterintuitive (but a clever optimization!) that the reference counter is 1 instead of 0 when free.
> 
> Something like:
> 
> static __rte_always_inline struct rte_mbuf *
> rte_pktmbuf_prefree_seg(struct rte_mbuf *m)
> {
> 	__rte_mbuf_sanity_check(m, 0);
> 
> 	if (likely(rte_mbuf_refcnt_read(m) == 1)) {
> +		/* This branch is a performance optimized variant of the branch below.
> +		 * If the reference counter would reach 0 when decrementing it,
> +		 * do not decrement it to 0 and then initialize it to 1;
> +		 * just leave it at 1, thereby avoiding writing to memory.
> +		 */
>
Even more than avoiding writing to memory, we know that with a refcount of
1, this thread is the only thread using this mbuf, so we don't need to use
atomic operations at all. The decrement we avoid is an especially expensive
one because of the atomic aspect of it.

/Bruce