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