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

[Bruce Richardson <bruce.richardson@intel.com>]

On Tue, Dec 05, 2023 at 09:04:08AM +0100, Morten Brørup wrote:
> > From: Feifei Wang [mailto:feifei.wang2@arm.com]
> > Sent: Tuesday, 5 December 2023 04.13
> > 
> > 在 2023/12/4 15:41, Morten Brørup 写道:
> > >> From: Feifei Wang [mailto:feifei.wang2@arm.com]
> > >> Sent: Monday, 4 December 2023 03.39
> > >>
> > >> For 'rte_pktmbuf_prefree_seg' function, 'rte_mbuf_refcnt_read(m) ==
> > 1'
> > >> and '__rte_mbuf_refcnt_update(m, -1) == 0' are the same cases where
> > >> mbuf's refcnt value should be 1. Thus we can simplify the code and
> > >> remove the redundant part.
> > >>
> > >> Furthermore, according to [1], when the mbuf is stored inside the
> > >> mempool, the m->refcnt value should be 1. And then it is detached
> > >> from its parent for an indirect mbuf. Thus change the description of
> > >> 'rte_pktmbuf_prefree_seg' function.
> > >>
> > >> [1]
> > https://patches.dpdk.org/project/dpdk/patch/20170404162807.20157-4-
> > >> olivier.matz@6wind.com/
> > >>
> > >> Suggested-by: Ruifeng Wang <ruifeng.wang@arm.com>
> > >> Signed-off-by: Feifei Wang <feifei.wang2@arm.com>
> > >> ---
> > >>   lib/mbuf/rte_mbuf.h | 22 +++-------------------
> > >>   1 file changed, 3 insertions(+), 19 deletions(-)
> > >>
> > >> diff --git a/lib/mbuf/rte_mbuf.h b/lib/mbuf/rte_mbuf.h
> > >> index 286b32b788..42e9b50d51 100644
> > >> --- a/lib/mbuf/rte_mbuf.h
> > >> +++ b/lib/mbuf/rte_mbuf.h
> > >> @@ -1328,7 +1328,7 @@ static inline int
> > >> __rte_pktmbuf_pinned_extbuf_decref(struct rte_mbuf *m)
> > >>    *
> > >>    * This function does the same than a free, except that it does
> > not
> > >>    * return the segment to its pool.
> > >> - * It decreases the reference counter, and if it reaches 0, it is
> > >> + * It decreases the reference counter, and if it reaches 1, it is
> > > No, the original description is correct.
> > > However, the reference counter is set to 1 when put back in the pool,
> > as a shortcut so it isn't needed to be set back to 1 when allocated
> > from the pool.
> > 
> > Ok.
> > 
> > for 'else if (__rte_mbuf_refcnt_update(m, -1) == 0)' case, it is easy
> > to
> > understand.
> > 
> > but for '(likely(rte_mbuf_refcnt_read(m) == 1))' case, I think this
> > will
> > create misleading. dpdk users maybe difficult to understand why the
> > code
> > can not match the function description.
> > 
> > Maybe we need some explanation here?
> 
> Agree. It is quite counterintuitive (but a clever optimization!) that the reference counter is 1 instead of 0 when free.
> 
> Something like:
> 
> 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)) {
> +		/* This branch is a performance optimized variant of the branch below.
> +		 * If the reference counter would reach 0 when decrementing it,
> +		 * do not decrement it to 0 and then initialize it to 1;
> +		 * just leave it at 1, thereby avoiding writing to memory.
> +		 */
>
Even more than avoiding writing to memory, we know that with a refcount of
1, this thread is the only thread using this mbuf, so we don't need to use
atomic operations at all. The decrement we avoid is an especially expensive
one because of the atomic aspect of it.

/Bruce