Intel FAST_FREE offload question

Intel FAST_FREE offload question

[Morten Brørup]

Intel NIC folks,

Why do the Intel network drivers, when using RTE_ETH_TX_OFFLOAD_MBUF_FAST_FREE, fall back to normal freeing when the mempool cache is empty (cache->len == 0)? It doesn't make sense to me.

Example:
https://git.dpdk.org/dpdk/tree/drivers/net/intel/common/tx.h#n146

Med venlig hilsen / Kind regards,
-Morten Brørup

Re: Intel FAST_FREE offload question

[Bruce Richardson]

On Fri, Feb 21, 2025 at 05:58:21PM +0100, Morten Brørup wrote:
> Intel NIC folks,
> 
> Why do the Intel network drivers, when using
> RTE_ETH_TX_OFFLOAD_MBUF_FAST_FREE, fall back to normal freeing when the
> mempool cache is empty (cache->len == 0)? It doesn't make sense to me.
> 
> Example:
> https://git.dpdk.org/dpdk/tree/drivers/net/intel/common/tx.h#n146
> 
Good question. I suspect that it may be a bug and that we meant to check
for size == 0 rather than len == 0.

RE: Intel FAST_FREE offload question

[Morten Brørup]

> From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> Sent: Friday, 21 February 2025 18.10
> 
> On Fri, Feb 21, 2025 at 05:58:21PM +0100, Morten Brørup wrote:
> > Intel NIC folks,
> >
> > Why do the Intel network drivers, when using
> > RTE_ETH_TX_OFFLOAD_MBUF_FAST_FREE, fall back to normal freeing when
> the
> > mempool cache is empty (cache->len == 0)? It doesn't make sense to
> me.
> >
> > Example:
> > https://git.dpdk.org/dpdk/tree/drivers/net/intel/common/tx.h#n146
> >
> Good question. I suspect that it may be a bug and that we meant to
> check
> for size == 0 rather than len == 0.

Then checking for cache == NULL suffices, because rte_mempool_default_cache() returns NULL if the cache size is 0:
https://elixir.bootlin.com/dpdk/v24.11.1/source/lib/mempool/rte_mempool.h#L1333

[PATCH] net/intel: allow fast-free to empty cache

[Bruce Richardson]

When freeing transmitted mbufs, there is no reason to send the freed
mbufs directly to the ring if the cache is empty - only if it is zero
size (in which case the cache pointer is NULL). Therefore, remove the
empty check and only check for a null cache pointer.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 drivers/net/intel/common/tx.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/intel/common/tx.h b/drivers/net/intel/common/tx.h
index d9cf4474fc..d361fe64ab 100644
--- a/drivers/net/intel/common/tx.h
+++ b/drivers/net/intel/common/tx.h
@@ -143,7 +143,7 @@ ci_tx_free_bufs_vec(struct ci_tx_queue *txq, ci_desc_done_fn desc_done, bool ctx
 		void **cache_objs;
 		struct rte_mempool_cache *cache = rte_mempool_default_cache(mp, rte_lcore_id());
 
-		if (!cache || cache->len == 0)
+		if (cache == NULL)
 			goto normal;
 
 		cache_objs = &cache->objs[cache->len];
-- 
2.43.0

RE: [PATCH] net/intel: allow fast-free to empty cache

[Morten Brørup]

> From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> Sent: Monday, 10 March 2025 14.26
> 
> When freeing transmitted mbufs, there is no reason to send the freed
> mbufs directly to the ring if the cache is empty - only if it is zero
> size (in which case the cache pointer is NULL). Therefore, remove the
> empty check and only check for a null cache pointer.
> 
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> ---
>  drivers/net/intel/common/tx.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/intel/common/tx.h
> b/drivers/net/intel/common/tx.h
> index d9cf4474fc..d361fe64ab 100644
> --- a/drivers/net/intel/common/tx.h
> +++ b/drivers/net/intel/common/tx.h
> @@ -143,7 +143,7 @@ ci_tx_free_bufs_vec(struct ci_tx_queue *txq,
> ci_desc_done_fn desc_done, bool ctx
>  		void **cache_objs;
>  		struct rte_mempool_cache *cache =
> rte_mempool_default_cache(mp, rte_lcore_id());
> 
> -		if (!cache || cache->len == 0)
> +		if (cache == NULL)
>  			goto normal;
> 
>  		cache_objs = &cache->objs[cache->len];
> --
> 2.43.0

Yep, it did look strange.
Reviewed-by: Morten Brørup <mb@smartsharesystems.com>

Re: [PATCH] net/intel: allow fast-free to empty cache

[Bruce Richardson]

On Mon, Mar 10, 2025 at 04:18:35PM +0100, Morten Brørup wrote:
> > From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> > Sent: Monday, 10 March 2025 14.26
> > 
> > When freeing transmitted mbufs, there is no reason to send the freed
> > mbufs directly to the ring if the cache is empty - only if it is zero
> > size (in which case the cache pointer is NULL). Therefore, remove the
> > empty check and only check for a null cache pointer.
> > 
> > Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> > ---
> >  drivers/net/intel/common/tx.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/net/intel/common/tx.h
> > b/drivers/net/intel/common/tx.h
> > index d9cf4474fc..d361fe64ab 100644
> > --- a/drivers/net/intel/common/tx.h
> > +++ b/drivers/net/intel/common/tx.h
> > @@ -143,7 +143,7 @@ ci_tx_free_bufs_vec(struct ci_tx_queue *txq,
> > ci_desc_done_fn desc_done, bool ctx
> >  		void **cache_objs;
> >  		struct rte_mempool_cache *cache =
> > rte_mempool_default_cache(mp, rte_lcore_id());
> > 
> > -		if (!cache || cache->len == 0)
> > +		if (cache == NULL)
> >  			goto normal;
> > 
> >  		cache_objs = &cache->objs[cache->len];
> > --
> > 2.43.0
> 
> Yep, it did look strange.
> Reviewed-by: Morten Brørup <mb@smartsharesystems.com>
> 
Thanks for review.

FYI: Given that this is in a sensitive area and we are now past rc2, I'm going
to postponse merging this patch till 25.07, rather than risk it in 25.03.

/Bruce

RE: [PATCH] net/intel: allow fast-free to empty cache

[Morten Brørup]

> From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> Sent: Monday, 10 March 2025 16.27
> 
> On Mon, Mar 10, 2025 at 04:18:35PM +0100, Morten Brørup wrote:
> > > From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> > > Sent: Monday, 10 March 2025 14.26
> > >
> > > When freeing transmitted mbufs, there is no reason to send the
> freed
> > > mbufs directly to the ring if the cache is empty - only if it is
> zero
> > > size (in which case the cache pointer is NULL). Therefore, remove
> the
> > > empty check and only check for a null cache pointer.
> > >
> > > Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> > > ---
> > >  drivers/net/intel/common/tx.h | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/net/intel/common/tx.h
> > > b/drivers/net/intel/common/tx.h
> > > index d9cf4474fc..d361fe64ab 100644
> > > --- a/drivers/net/intel/common/tx.h
> > > +++ b/drivers/net/intel/common/tx.h
> > > @@ -143,7 +143,7 @@ ci_tx_free_bufs_vec(struct ci_tx_queue *txq,
> > > ci_desc_done_fn desc_done, bool ctx
> > >  		void **cache_objs;
> > >  		struct rte_mempool_cache *cache =
> > > rte_mempool_default_cache(mp, rte_lcore_id());
> > >
> > > -		if (!cache || cache->len == 0)
> > > +		if (cache == NULL)
> > >  			goto normal;
> > >
> > >  		cache_objs = &cache->objs[cache->len];
> > > --
> > > 2.43.0
> >
> > Yep, it did look strange.
> > Reviewed-by: Morten Brørup <mb@smartsharesystems.com>
> >
> Thanks for review.

BTW, I recall other Intel drivers having the same "feature":
https://elixir.bootlin.com/dpdk/v24.11.1/source/drivers/net/iavf/iavf_rxtx_vec_avx512.c#L1876
https://elixir.bootlin.com/dpdk/v24.11.1/source/drivers/net/ice/ice_rxtx_vec_avx512.c#L891

> 
> FYI: Given that this is in a sensitive area and we are now past rc2,
> I'm going
> to postponse merging this patch till 25.07, rather than risk it in
> 25.03.

Agree.

> 
> /Bruce

Re: [PATCH] net/intel: allow fast-free to empty cache

[Bruce Richardson]

On Mon, Mar 10, 2025 at 04:34:23PM +0100, Morten Brørup wrote:
> > From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> > Sent: Monday, 10 March 2025 16.27
> > 
> > On Mon, Mar 10, 2025 at 04:18:35PM +0100, Morten Brørup wrote:
> > > > From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> > > > Sent: Monday, 10 March 2025 14.26
> > > >
> > > > When freeing transmitted mbufs, there is no reason to send the
> > freed
> > > > mbufs directly to the ring if the cache is empty - only if it is
> > zero
> > > > size (in which case the cache pointer is NULL). Therefore, remove
> > the
> > > > empty check and only check for a null cache pointer.
> > > >
> > > > Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> > > > ---
> > > >  drivers/net/intel/common/tx.h | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/net/intel/common/tx.h
> > > > b/drivers/net/intel/common/tx.h
> > > > index d9cf4474fc..d361fe64ab 100644
> > > > --- a/drivers/net/intel/common/tx.h
> > > > +++ b/drivers/net/intel/common/tx.h
> > > > @@ -143,7 +143,7 @@ ci_tx_free_bufs_vec(struct ci_tx_queue *txq,
> > > > ci_desc_done_fn desc_done, bool ctx
> > > >  		void **cache_objs;
> > > >  		struct rte_mempool_cache *cache =
> > > > rte_mempool_default_cache(mp, rte_lcore_id());
> > > >
> > > > -		if (!cache || cache->len == 0)
> > > > +		if (cache == NULL)
> > > >  			goto normal;
> > > >
> > > >  		cache_objs = &cache->objs[cache->len];
> > > > --
> > > > 2.43.0
> > >
> > > Yep, it did look strange.
> > > Reviewed-by: Morten Brørup <mb@smartsharesystems.com>
> > >
> > Thanks for review.
> 
> BTW, I recall other Intel drivers having the same "feature":
> https://elixir.bootlin.com/dpdk/v24.11.1/source/drivers/net/iavf/iavf_rxtx_vec_avx512.c#L1876
> https://elixir.bootlin.com/dpdk/v24.11.1/source/drivers/net/ice/ice_rxtx_vec_avx512.c#L891
> 

With the deduplication work I did earlier this release, this code should
now be shared between ixgbe, i40e, iavf and ice drivers, so we hopefully
can fix all 4 with one change.

RE: [PATCH] net/intel: allow fast-free to empty cache

[Morten Brørup]

> From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> Sent: Monday, 10 March 2025 16.53
> 
> On Mon, Mar 10, 2025 at 04:34:23PM +0100, Morten Brørup wrote:
> > > From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> > > Sent: Monday, 10 March 2025 16.27
> > >
> > > On Mon, Mar 10, 2025 at 04:18:35PM +0100, Morten Brørup wrote:
> > > > > From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> > > > > Sent: Monday, 10 March 2025 14.26
> > > > >
> > > > > When freeing transmitted mbufs, there is no reason to send the
> > > freed
> > > > > mbufs directly to the ring if the cache is empty - only if it
> is
> > > zero
> > > > > size (in which case the cache pointer is NULL). Therefore,
> remove
> > > the
> > > > > empty check and only check for a null cache pointer.
> > > > >
> > > > > Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> > > > > ---
> > > > >  drivers/net/intel/common/tx.h | 2 +-
> > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/net/intel/common/tx.h
> > > > > b/drivers/net/intel/common/tx.h
> > > > > index d9cf4474fc..d361fe64ab 100644
> > > > > --- a/drivers/net/intel/common/tx.h
> > > > > +++ b/drivers/net/intel/common/tx.h
> > > > > @@ -143,7 +143,7 @@ ci_tx_free_bufs_vec(struct ci_tx_queue
> *txq,
> > > > > ci_desc_done_fn desc_done, bool ctx
> > > > >  		void **cache_objs;
> > > > >  		struct rte_mempool_cache *cache =
> > > > > rte_mempool_default_cache(mp, rte_lcore_id());
> > > > >
> > > > > -		if (!cache || cache->len == 0)
> > > > > +		if (cache == NULL)
> > > > >  			goto normal;
> > > > >
> > > > >  		cache_objs = &cache->objs[cache->len];
> > > > > --
> > > > > 2.43.0
> > > >
> > > > Yep, it did look strange.
> > > > Reviewed-by: Morten Brørup <mb@smartsharesystems.com>
> > > >
> > > Thanks for review.
> >
> > BTW, I recall other Intel drivers having the same "feature":
> >
> https://elixir.bootlin.com/dpdk/v24.11.1/source/drivers/net/iavf/iavf_r
> xtx_vec_avx512.c#L1876
> >
> https://elixir.bootlin.com/dpdk/v24.11.1/source/drivers/net/ice/ice_rxt
> x_vec_avx512.c#L891
> >
> 
> With the deduplication work I did earlier this release, this code
> should
> now be shared between ixgbe, i40e, iavf and ice drivers, so we
> hopefully
> can fix all 4 with one change.

Excellent! Your deduplication already starts paying off. :-)
It looks like only the non-deduplicated idpf driver remains to be modified separately:
drivers/net/intel/idpf/idpf_common_rxtx_avx512.c