Re: [PATCH v2] mbuf: optimize segment prefree

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

On Wed, Oct 22, 2025 at 03:53:21PM +0200, Morten Brørup wrote:
> > From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> > Sent: Wednesday, 22 October 2025 11.08
> > 
> > On Mon, Oct 20, 2025 at 12:02:01PM +0000, Morten Brørup wrote:
> > > Refactored rte_pktmbuf_prefree_seg() for both performance and
> > readability.
> > >
> > > With the optimized RTE_MBUF_DIRECT() macro, the common likely code
> > path
> > > now fits within one instruction cache line on x86-64 when built with
> > GCC.
> > >
> > > Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
> > 
> > Reviewed-by: Bruce Richardson <bruce.richardson@intel.com>
> > 
> 
> [...]
> 
> > >  #define RTE_MBUF_DIRECT(mb) \
> > >  	(!((mb)->ol_flags & (RTE_MBUF_F_INDIRECT | RTE_MBUF_F_EXTERNAL)))
> > >
> > > +#if defined(RTE_TOOLCHAIN_GCC) && defined(RTE_ARCH_X86)
> > > +/* Optimization for code size.
> > > + * GCC only optimizes single-bit MSB tests this way, so we do it by
> > hand with multi-bit.
> > > + *
> > > + * The flags RTE_MBUF_F_INDIRECT and RTE_MBUF_F_EXTERNAL are both in
> > the MSB of the
> > > + * 64-bit ol_flags field, so we only compare this one byte instead
> > of all 64 bits.
> > > + * On little endian architecture, the MSB of a 64-bit integer is at
> > byte offest 7.
> > > + *
> > > + * Note: Tested using GCC version 16.0.0 20251019 (experimental).
> > > + *
> > > + * Without this optimization, GCC generates 17 bytes of
> > instructions:
> > > + *      movabs rax,0x6000000000000000       // 10 bytes
> > > + *      and    rax,QWORD PTR [rdi+0x18]     // 4 bytes
> > > + *      sete   al                           // 3 bytes
> > > + * With this optimization, GCC generates only 7 bytes of
> > instructions:
> > > + *      test   BYTE PTR [rdi+0x1f],0x60     // 4 bytes
> > > + *      sete   al                           // 3 bytes
> > > + */
> > > +#undef RTE_MBUF_DIRECT
> > > +#define RTE_MBUF_DIRECT(mb) \
> > > +	(!(((const uint8_t *)(mb))[offsetof(struct rte_mbuf, ol_flags) +
> > 7] & \
> > > +	(uint8_t)((RTE_MBUF_F_INDIRECT | RTE_MBUF_F_EXTERNAL) >> (7 *
> > 8))))
> > > +static_assert(((RTE_MBUF_F_INDIRECT | RTE_MBUF_F_EXTERNAL) >> (7 *
> > 8)) << (7 * 8) ==
> > > +	(RTE_MBUF_F_INDIRECT | RTE_MBUF_F_EXTERNAL),
> > > +	"RTE_MBUF_F_INDIRECT and/or RTE_MBUF_F_EXTERNAL are not in
> > MSB.");
> > > +#endif
> > > +
> > Couple of comments/thoughts/questions here.
> > 
> > * This looks like a compiler limitation that should be fixed in GCC. IF
> > we
> >   put this optimization in, how will we know when/if we can remove it
> > again
> >   in future? I'm not sure we want this hanging around forever.
> 
> Agree.
> There are plenty of hand crafted optimizations in DPDK, which are already obsolete;
> it seems no one has found a good way of identifying them. Including myself.
> 
> > * Can the static_assert - which just checks flags are in the MSB - be
> > * simplified to e.g.
> >   "((RTE_MBUF_F_INDIRECT | RTE_MBUF_F_EXTERNAL) << CHAR_BIT) == 0"
> >   or "__builtin_ctzll(...) > (7 * CHAR_BIT)"
> > * As in prev bullet, I tend to prefer use of CHAR_BIT over hard-coded
> > 8.
> 
> In v3, I have simplified both the static_assert and the optimized macro as you suggested on Slack,
> with some minor improvements.
> 
> > * Is it necessary to limit this to just GCC and x86? If it leads to the
> >   best code on x86, why not include for all compilers? What about non-
> > x86
> >   LE platforms?
> 
> I had already tested ARM64, where it didn't make a difference; now I have added a note about it.
> I also tested ARM32, which doesn't benefit either, but I didn't add a note about it.
> I also tested Loongarch (on Godbolt), which does benefit from it, so I added it.
> 
> Now, as I'm writing this email, Godbolt shows that RISC-V and POWER could also benefit.
> Maybe we should just replace the standard macro with the optimized macro. WDYT?
> 
I think that's not a bad idea. At least everything would be consistent.

/Bruce