* Intel FAST_FREE offload question @ 2025-02-21 16:58 Morten Brørup 2025-02-21 17:09 ` Bruce Richardson 2025-03-10 13:25 ` [PATCH] net/intel: allow fast-free to empty cache Bruce Richardson 0 siblings, 2 replies; 9+ messages in thread From: Morten Brørup @ 2025-02-21 16:58 UTC (permalink / raw) To: bruce.richardson, Anatoly Burakov, Vladimir Medvedkin, Ian Stokes, Jingjing Wu, Praveen Shetty Cc: dev 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Intel FAST_FREE offload question 2025-02-21 16:58 Intel FAST_FREE offload question Morten Brørup @ 2025-02-21 17:09 ` Bruce Richardson 2025-02-21 17:16 ` Morten Brørup 2025-03-10 13:25 ` [PATCH] net/intel: allow fast-free to empty cache Bruce Richardson 1 sibling, 1 reply; 9+ messages in thread From: Bruce Richardson @ 2025-02-21 17:09 UTC (permalink / raw) To: Morten Brørup Cc: Anatoly Burakov, Vladimir Medvedkin, Ian Stokes, Jingjing Wu, Praveen Shetty, dev 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. ^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: Intel FAST_FREE offload question 2025-02-21 17:09 ` Bruce Richardson @ 2025-02-21 17:16 ` Morten Brørup 0 siblings, 0 replies; 9+ messages in thread From: Morten Brørup @ 2025-02-21 17:16 UTC (permalink / raw) To: Bruce Richardson Cc: Anatoly Burakov, Vladimir Medvedkin, Ian Stokes, Jingjing Wu, Praveen Shetty, dev > 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] net/intel: allow fast-free to empty cache 2025-02-21 16:58 Intel FAST_FREE offload question Morten Brørup 2025-02-21 17:09 ` Bruce Richardson @ 2025-03-10 13:25 ` Bruce Richardson 2025-03-10 15:18 ` Morten Brørup 1 sibling, 1 reply; 9+ messages in thread From: Bruce Richardson @ 2025-03-10 13:25 UTC (permalink / raw) To: dev; +Cc: anatoly.burakov, mb, 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [PATCH] net/intel: allow fast-free to empty cache 2025-03-10 13:25 ` [PATCH] net/intel: allow fast-free to empty cache Bruce Richardson @ 2025-03-10 15:18 ` Morten Brørup 2025-03-10 15:27 ` Bruce Richardson 0 siblings, 1 reply; 9+ messages in thread From: Morten Brørup @ 2025-03-10 15:18 UTC (permalink / raw) To: Bruce Richardson, dev; +Cc: anatoly.burakov > 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> ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] net/intel: allow fast-free to empty cache 2025-03-10 15:18 ` Morten Brørup @ 2025-03-10 15:27 ` Bruce Richardson 2025-03-10 15:34 ` Morten Brørup 0 siblings, 1 reply; 9+ messages in thread From: Bruce Richardson @ 2025-03-10 15:27 UTC (permalink / raw) To: Morten Brørup; +Cc: dev, anatoly.burakov 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [PATCH] net/intel: allow fast-free to empty cache 2025-03-10 15:27 ` Bruce Richardson @ 2025-03-10 15:34 ` Morten Brørup 2025-03-10 15:53 ` Bruce Richardson 0 siblings, 1 reply; 9+ messages in thread From: Morten Brørup @ 2025-03-10 15:34 UTC (permalink / raw) To: Bruce Richardson; +Cc: dev, anatoly.burakov > 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] net/intel: allow fast-free to empty cache 2025-03-10 15:34 ` Morten Brørup @ 2025-03-10 15:53 ` Bruce Richardson 2025-03-10 17:37 ` Morten Brørup 0 siblings, 1 reply; 9+ messages in thread From: Bruce Richardson @ 2025-03-10 15:53 UTC (permalink / raw) To: Morten Brørup; +Cc: dev, anatoly.burakov 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. ^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [PATCH] net/intel: allow fast-free to empty cache 2025-03-10 15:53 ` Bruce Richardson @ 2025-03-10 17:37 ` Morten Brørup 0 siblings, 0 replies; 9+ messages in thread From: Morten Brørup @ 2025-03-10 17:37 UTC (permalink / raw) To: Bruce Richardson, Jingjing Wu, Praveen Shetty; +Cc: dev, anatoly.burakov > 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2025-03-10 17:37 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2025-02-21 16:58 Intel FAST_FREE offload question Morten Brørup 2025-02-21 17:09 ` Bruce Richardson 2025-02-21 17:16 ` Morten Brørup 2025-03-10 13:25 ` [PATCH] net/intel: allow fast-free to empty cache Bruce Richardson 2025-03-10 15:18 ` Morten Brørup 2025-03-10 15:27 ` Bruce Richardson 2025-03-10 15:34 ` Morten Brørup 2025-03-10 15:53 ` Bruce Richardson 2025-03-10 17:37 ` Morten Brørup
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).