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 <m...@smartsharesystems.com> > Sent: Thursday, January 9, 2025 2:12 PM > To: Shahaf Shuler <shah...@nvidia.com>; Slava Ovsiienko > <viachesl...@nvidia.com>; dev@dpdk.org > Cc: Olivier Matz <olivier.m...@6wind.com>; NBU-Contact-Thomas Monjalon > (EXTERNAL) <tho...@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.
