Re: [RFC] ethdev: TX mbuf fast release optimization

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

On Thu, Jul 03, 2025 at 05:29:26PM +0200, Morten Brørup wrote:
> > From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> > Sent: Thursday, 3 July 2025 17.21
> > 
> > On Thu, Jul 03, 2025 at 05:12:46PM +0200, Morten Brørup wrote:
> > > > From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> > > > Sent: Thursday, 3 July 2025 16.14
> > > >
> > > > On Thu, Jul 03, 2025 at 03:59:14PM +0200, Morten Brørup wrote:
> > > > > For TX mbuf fast release offload, I propose to add the mbuf
> > mempool
> > > > > pointer to the ethdev tx queue configuration structure,
> > > > > so the ethdev TX burst operation doesn't need to fetch it from the
> > > > > first mbuf of each burst being fast free'd to the mempool.
> > > > >
> > > > > This modification of the struct rte_eth_txconf, and the
> > requirement
> > > > > to set the mempool pointer if the
> > RTE_ETH_TX_OFFLOAD_MBUF_FAST_FREE
> > > > > flag is set, will be an API+ABI change in 25.11.
> > > > > Should it be announced in the 25.07 release notes?
> > > > >
> > > > > Note: We could phase it in softly by letting the ethdev drivers
> > > > > check if the pointer has been set, and fall back to fetching it
> > > > > from mbuf[0] if not.
> > > > >
> > > > > /**
> > > > >  * A structure used to configure a Tx ring of an Ethernet port.
> > > > >  */
> > > > > struct rte_eth_txconf {
> > > > > 	struct rte_eth_thresh tx_thresh; /**< Tx ring threshold
> > registers.
> > > > */
> > > > > 	uint16_t tx_rs_thresh; /**< Drives the setting of RS bit on
> > TXDs.
> > > > */
> > > > > 	uint16_t tx_free_thresh; /**< Start freeing Tx buffers if
> > there
> > > > are
> > > > > 				      less free descriptors than this
> > value. */
> > > > >
> > > > > 	uint8_t tx_deferred_start; /**< Do not start queue with
> > > > rte_eth_dev_start(). */
> > > > > 	/**
> > > > > 	 * Per-queue Tx offloads to be set  using
> > RTE_ETH_TX_OFFLOAD_*
> > > > flags.
> > > > > 	 * Only offloads set on tx_queue_offload_capa or
> > tx_offload_capa
> > > > > 	 * fields on rte_eth_dev_info structure are allowed to be
> > set.
> > > > > 	 */
> > > > > 	uint64_t offloads;
> > > > >
> > > > > +	/**
> > > > > +	 * Per-queue mempool to release the mbufs to; required for
> > > > > +	 * RTE_ETH_TX_OFFLOAD_MBUF_FAST_FREE offload.
> > > > > +	 */
> > > > > +	struct rte_mempool *mp;
> > > > > +
> > > >
> > > > Is this really best put in the txconf struct? Would it not be better
> > > > just
> > > > stored in the queue structure on free or Tx of the first packet?
> > That
> > > > would
> > > > make it not change the ABI.
> > >
> > > It seems natural to me to specify the mempool at configuration time,
> > > because it is a configuration parameter.
> > > The txconf struct seemed like the best place to put it.
> > >
> > > Fetching it from the first mbuf of each burst at runtime, and possibly
> > caching it, seems like a workaround for the missing configuration
> > parameter.
> > >
> > Not suggesting from each burst, just the first mbuf seen. One time only.
> 
> That's what I meant by "possibly caching it". :-)
> 
> The check for the one-time cache update still takes up code space in the per-burst fast path. (Although very little.)
> 
> But I still prefer providing configuration information at configuration time, rather than snooping it at runtime.
> 
Ok, but then are we mandating that all existing apps now are required to
pass in a mempool value on Tx queue config? I'm concerned about how many
apps that may affect. On the other hand, if it is optional, does that not
slow down the fastpath more having to check for this optional value?

/Bruce