mbox series

[v6,0/4] mempool: fix mempool cache flushing algorithm

Message ID 20221009133737.795377-1-andrew.rybchenko@oktetlabs.ru (mailing list archive)
Headers
Series mempool: fix mempool cache flushing algorithm |

Message

Andrew Rybchenko Oct. 9, 2022, 1:37 p.m. UTC
  v6 changes (Andrew Rybchenko):

- Fix spelling

v5 changes (Andrew Rybchenko):

- Factor out cosmetic fixes into separate patches to make all
  patches smaller and easier to review
- Remove extra check as per review notes
- Factor out entire cache flushing into a separate patch.
  It is nice from logical changes separation point of view,
  easier to bisect and revert.

v4 changes:

- Updated patch title to reflect that the scope of the patch is only
mempool cache flushing.

- Do not replace rte_memcpy() with alternative copying method. This was
a pure optimization, not a fix.

- Elaborate even more on the bugs fixed by the modifications.

- Added 4th bullet item to the patch description, regarding
rte_mempool_ops_enqueue_bulk() with RTE_LIBRTE_MEMPOOL_DEBUG.

v3 changes:

- Actually remove my modifications of the rte_mempool_cache structure.

v2 changes:

- Not adding the new objects to the mempool cache before flushing it
also allows the memory allocated for the mempool cache to be reduced
from 3 x to 2 x RTE_MEMPOOL_CACHE_MAX_SIZE.
However, such this change would break the ABI, so it was removed in v2.

- The mempool cache should be cache line aligned for the benefit of the
copying method, which on some CPU architectures performs worse on data
crossing a cache boundary.
However, such this change would break the ABI, so it was removed in v2;
and yet another alternative copying method replaced the rte_memcpy().

Andrew Rybchenko (3):
  mempool: check driver enqueue result in one place
  mempool: avoid usage of term ring on put
  mempool: flush cache completely on overflow

Morten Brørup (1):
  mempool: fix cache flushing algorithm

 lib/mempool/rte_mempool.c |  5 ++++
 lib/mempool/rte_mempool.h | 55 ++++++++++++++++++++-------------------
 2 files changed, 33 insertions(+), 27 deletions(-)
  

Comments

Thomas Monjalon Oct. 10, 2022, 3:21 p.m. UTC | #1
> Andrew Rybchenko (3):
>   mempool: check driver enqueue result in one place
>   mempool: avoid usage of term ring on put
>   mempool: flush cache completely on overflow
> 
> Morten Brørup (1):
>   mempool: fix cache flushing algorithm

Applied only first 2 "cosmetic" patches as discussed with Andrew.
The goal is to make some performance tests
before merging the rest of the series.
  
Morten Brørup Oct. 11, 2022, 7:26 p.m. UTC | #2
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Monday, 10 October 2022 17.21
> 
> > Andrew Rybchenko (3):
> >   mempool: check driver enqueue result in one place
> >   mempool: avoid usage of term ring on put
> >   mempool: flush cache completely on overflow
> >
> > Morten Brørup (1):
> >   mempool: fix cache flushing algorithm
> 
> Applied only first 2 "cosmetic" patches as discussed with Andrew.
> The goal is to make some performance tests
> before merging the rest of the series.

I just came to think of this:

Don't test with RTE_ETH_TX_OFFLOAD_MBUF_FAST_FREE, because some PMD's bypass the mempool library and manipulate the mempool cache structure directly, e.g. https://elixir.bootlin.com/dpdk/latest/source/drivers/net/i40e/i40e_rxtx_vec_avx512.c#L903

The copy-pasted code in those PMDs should probably also be updated to reflect the updated mempool library behavior. :-(
  
Thomas Monjalon Oct. 26, 2022, 2:09 p.m. UTC | #3
10/10/2022 17:21, Thomas Monjalon:
> > Andrew Rybchenko (3):
> >   mempool: check driver enqueue result in one place
> >   mempool: avoid usage of term ring on put
> >   mempool: flush cache completely on overflow
> > 
> > Morten Brørup (1):
> >   mempool: fix cache flushing algorithm
> 
> Applied only first 2 "cosmetic" patches as discussed with Andrew.
> The goal is to make some performance tests
> before merging the rest of the series.

The last 2 patches are now merged in 22.11-rc2

There were some comments about improving alignment on cache.
Is it something we want in this release?
  
Morten Brørup Oct. 26, 2022, 2:26 p.m. UTC | #4
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Wednesday, 26 October 2022 16.09
> 
> 10/10/2022 17:21, Thomas Monjalon:
> > > Andrew Rybchenko (3):
> > >   mempool: check driver enqueue result in one place
> > >   mempool: avoid usage of term ring on put
> > >   mempool: flush cache completely on overflow
> > >
> > > Morten Brørup (1):
> > >   mempool: fix cache flushing algorithm
> >
> > Applied only first 2 "cosmetic" patches as discussed with Andrew.
> > The goal is to make some performance tests
> > before merging the rest of the series.
> 
> The last 2 patches are now merged in 22.11-rc2

Thank you.

> 
> There were some comments about improving alignment on cache.
> Is it something we want in this release?

I think so, yes. Jerin also agreed that it was a good idea.

I will send a patch in a few minutes.

-Morten