[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 at 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;
>  }
>  
>  /**