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