Re: mbuf refcnt issue

[Stephen Hemminger <stephen@networkplumber.org>]

On Wed, 9 Apr 2025 23:22:50 +0000
"Lombardo, Ed" <Ed.Lombardo@netscout.com> wrote:

> Hi,
> I just finished modifying and testing our application to just do transmit of packets received on an NIC interface and let the rte_eth_tx_burst () free the mbuf and all works fine for both traffic types.  This proves to me that my implementation of processing the packets and queueing them to tx ring and transmit from the tx ring is not buggy, which I had carefully verified in gdb early on.  I still believe there is a problem with our application with many threads that can do rte_pktmbuf_free() on the same mbuf.  
> 
> I added these lines in my driver source file:
> #define RTE_LITRTE_MBUF_DEBUG 1
> #define RTE_LIBRTE_MEMPOOL_DEBUG 1
> #define RTE_ENABLE_ASSERT 1
> 
> I don't see any asserts occur during my tx packet testing.
> 
> The dpdk header files show the Atomic ifdef checks
> rte_build_config.h:#define RTE_MBUF_REFCNT_ATOMIC
> rte_mbuf_core.h:                         * or non-atomic) is controlled by the RTE_MBUF_REFCNT_ATOMIC flag.
> rte_mbuf.h:#ifdef RTE_MBUF_REFCNT_ATOMIC
> rte_mbuf.h:#else /* ! RTE_MBUF_REFCNT_ATOMIC */
> rte_mbuf.h:#endif /* RTE_MBUF_REFCNT_ATOMIC */
> 
> I verified in building our application with DPDK rte_mbuf.h header file that the atomic functions for mbuf refcnt read/writes are turned ON.  I added junk characters, and the compiler spotted syntax errors.
> 
> So, I am back to the question as to why I get mbuf issue with multiple threads processing the same mbuf?
> 
> Any more suggestions.
> 
> Thanks,
> Ed
> 
> -----Original Message-----
> From: Stephen Hemminger <stephen@networkplumber.org> 
> Sent: Wednesday, April 9, 2025 12:24 PM
> To: Lombardo, Ed <Ed.Lombardo@netscout.com>
> Cc: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>; users@dpdk.org
> Subject: Re: mbuf refcnt issue
> 
> External Email: This message originated outside of NETSCOUT. Do not click links or open attachments unless you recognize the sender and know the content is safe.
> 
> On Wed, 9 Apr 2025 04:46:09 +0000
> "Lombardo, Ed" <Ed.Lombardo@netscout.com> wrote:
> 
> > Hi Stephen,
> > I am looking a the rte_mbuf.h file for rte_pktmbuf_free() and it is not clear to me that it checks if the mbuf refcnt is 1 before decrementing it and allowing the mbuf and segments (if any) to be returned to free pool.
> > 
> > Could my application issue be I have tx threads that transmit packets and does rte_pktmbuf_free(), while one other thread will perform rte_pktmbuf_free() on the same mbuf?  I ensured I bump the mbuf refcnt to 2 before other threads can process the same mbuf.
> > 
> > Thanks,
> > Ed  
> 
> It doesn't need to check refcnt there. The check is done later (since mbuf can be multi segment).
> 
> rte_pktmbuf_free
>  -> rte_pktmbuf_free_seg
>     -> rte_pktmbuf_prefree_seg  
> 		
> 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)) {
>             normal fast path. breaks the chain.
> 	} else if (__rte_mbuf_refcnt_update(m, -1) == 0) {
>             refcnt > 1 logic
> 
> Note, the refcnt doesn't always go to zero when the mbuf is put back in the pool.
> The refcnt for a freed mbuf (in the pool) doesn't matter, it is free, it is dead.
> The refcnt is reset to 1 when mbuf is extracted from the pool.
> 
> 
> 


You might find something by poisoning the mbuf when it is freed, so that any attempt to use
the data would get junk. Also, add check at start of rte_pktmbuf_free_seg() to catch dup free.
Something like this, but only compile tested.
It will break some of the functional tests, because they tend to make bogus dummy mbufs.

diff --git a/lib/mbuf/rte_mbuf.h b/lib/mbuf/rte_mbuf.h
index 06ab7502a5..6088b34506 100644
--- a/lib/mbuf/rte_mbuf.h
+++ b/lib/mbuf/rte_mbuf.h
@@ -1423,10 +1423,11 @@ static inline int __rte_pktmbuf_pinned_extbuf_decref(struct rte_mbuf *m)
 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)) {
+	__rte_mbuf_sanity_check(m, 0);
 
+	unsigned int refcnt = rte_mbuf_refcnt_read(m);
+	if (likely(refcnt == 1)) {
 		if (!RTE_MBUF_DIRECT(m)) {
 			rte_pktmbuf_detach(m);
 			if (RTE_MBUF_HAS_EXTBUF(m) &&
@@ -1435,13 +1436,15 @@ rte_pktmbuf_prefree_seg(struct rte_mbuf *m)
 				return NULL;
 		}
 
+		m->refcnt = 0;
 		if (m->next != NULL)
 			m->next = NULL;
 		if (m->nb_segs != 1)
 			m->nb_segs = 1;
 
 		return m;
-
+	} else if (unlikely(refcnt == 0 || refcnt >= UINT16_MAX - 1)) {
+		rte_panic("mbuf refcnt underflow %u\n", refcnt);
 	} else if (__rte_mbuf_refcnt_update(m, -1) == 0) {
 
 		if (!RTE_MBUF_DIRECT(m)) {
@@ -1452,6 +1455,7 @@ rte_pktmbuf_prefree_seg(struct rte_mbuf *m)
 				return NULL;
 		}
 
+		m->refcnt = 0;
 		if (m->next != NULL)
 			m->next = NULL;
 		if (m->nb_segs != 1)