questions about pinned external buffers

questions about pinned external buffers

[Morten Brørup]

Pinned external buffers were introduced with this patch:
https://git.dpdk.org/dpdk/commit/lib/librte_mbuf/rte_mbuf.h?id=6ef1107ad4c6d4fcb6be627367ee0b97bb13e822

Question 1:
Please confirm that the mbuf's pinned external buffer's refcnt is supposed to be 1 when an mbuf is returned to the mempool?

If so, the below assertion should be updated for completion:

rte_mbuf_raw_free(struct rte_mbuf *m)
{
	RTE_ASSERT(!RTE_MBUF_CLONED(m) &&
-			(!RTE_MBUF_HAS_EXTBUF(m) || RTE_MBUF_HAS_PINNED_EXTBUF(m)));
+			(!RTE_MBUF_HAS_EXTBUF(m) ||
+			(RTE_MBUF_HAS_PINNED_EXTBUF(m) &&
+			rte_mbuf_ext_refcnt_read(m->shinfo) == 1)));
	__rte_mbuf_raw_sanity_check(m);
	rte_mempool_put(m->pool, m);
}

The increased performance cost should be acceptable for debug builds (i.e. with assertions enabled).

Question 2:
Could this assertion be moved to __rte_mbuf_raw_sanity_check()?

I'm working on a new rte_mbuf_raw_free_bulk() function, for use with RTE_ETH_TX_OFFLOAD_MBUF_FAST_FREE; and if the assertion is not moved to __rte_mbuf_raw_sanity_check(), it needs to be copy-pasted into the new raw_free_bulk() function too.

RE: questions about pinned external buffers

[Slava Ovsiienko]

Hi, Morten

> Question 1:
> Please confirm that the mbuf's pinned external buffer's refcnt is supposed to
> be 1 when an mbuf is returned to the mempool?
...
> +			rte_mbuf_ext_refcnt_read(m->shinfo) == 1)));
I think we can add this extra check and neglect the light performance impact in debug version.

> Question 2:
> Could this assertion be moved to __rte_mbuf_raw_sanity_check()?
Looks like it could, but we should be careful about side effects in 
__rte_mbuf_raw_sanity_check(), it is exposed in rte_mbuf.h to users
(also as legacy MBUF_RAW_ALLOC_CHECK). But I have no strong objections.


With best regards,
Slava

> -----Original Message-----
> From: Morten Brørup <mb@smartsharesystems.com>
> Sent: Thursday, January 9, 2025 2:12 PM
> To: Shahaf Shuler <shahafs@nvidia.com>; Slava Ovsiienko
> <viacheslavo@nvidia.com>; dev@dpdk.org
> Cc: Olivier Matz <olivier.matz@6wind.com>; NBU-Contact-Thomas Monjalon
> (EXTERNAL) <thomas@monjalon.net>
> Subject: questions about pinned external buffers
> 
> Pinned external buffers were introduced with this patch:
> https://git.dpdk.org/dpdk/commit/lib/librte_mbuf/rte_mbuf.h?id=6ef1107ad4
> c6d4fcb6be627367ee0b97bb13e822
> 
> Question 1:
> Please confirm that the mbuf's pinned external buffer's refcnt is supposed to
> be 1 when an mbuf is returned to the mempool?
> 
> If so, the below assertion should be updated for completion:
> 
> rte_mbuf_raw_free(struct rte_mbuf *m)
> {
> 	RTE_ASSERT(!RTE_MBUF_CLONED(m) &&
> -			(!RTE_MBUF_HAS_EXTBUF(m) ||
> RTE_MBUF_HAS_PINNED_EXTBUF(m)));
> +			(!RTE_MBUF_HAS_EXTBUF(m) ||
> +			(RTE_MBUF_HAS_PINNED_EXTBUF(m) &&
> +			rte_mbuf_ext_refcnt_read(m->shinfo) == 1)));
> 	__rte_mbuf_raw_sanity_check(m);
> 	rte_mempool_put(m->pool, m);
> }
> 
> The increased performance cost should be acceptable for debug builds (i.e.
> with assertions enabled).
> 
> Question 2:
> Could this assertion be moved to __rte_mbuf_raw_sanity_check()?
> 
> I'm working on a new rte_mbuf_raw_free_bulk() function, for use with
> RTE_ETH_TX_OFFLOAD_MBUF_FAST_FREE; and if the assertion is not moved
> to __rte_mbuf_raw_sanity_check(), it needs to be copy-pasted into the new
> raw_free_bulk() function too.