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

[Stephen Hemminger <stephen@networkplumber.org>]

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