[v2,0/3] mempool: fix mempool virt populate with small chunks
mbox series

Message ID 20200117145754.11682-1-olivier.matz@6wind.com
Headers show
Series
  • mempool: fix mempool virt populate with small chunks
Related show

Message

Olivier Matz Jan. 17, 2020, 2:57 p.m. UTC
rte_mempool_populate_virt() sometimes fail, when it calls
rte_mempool_populate_iova() with an area which is too small to store one
object. This should not be an error.

I prepared a v2 which implements an ABI compatibility through symbol
versioning, as suggested [1]. It looks a bit overkill to me, but it
was an interresting exercice.

v2 changes:

- The initial requirement is to fix an issue at mempool creation. As
  the fix will probably be backported in 19.11.x, the first patch
  in the patchset does not break API/ABI.

- Using symbol versioning helps to preserve ABI, but for the API
  the breakage has to be announced 1 release in advance, so there
  is a separate patch for this.

- There is a 3rd patch for 20.05 that makes the new API public and
  implements ABI versioning (if ok, I'll remove it from the patchset in
  next version and send it separately)

- It appears that returning -ENOBUFS instead of -EINVAL is not ideal
  because, in theory, mempool_ops_alloc_once() could also return
  -ENOBUFS, and it would be forwarded to the caller by
  rte_mempool_populate_iova() too, and misinterpreted as "there is not
  enough room".

  Returning 0 instead of -ENOBUFS was initially suggested by Anatoly,
  and it does not suffer from this problem. It is doable if we properly
  document that the memory chunk is not added to the mempool when
  returning 0. It has an impact on populate_virt(), which has to be
  versioned too.

There are some checkpatch warnings, but I'm not sure how if I
should solve them:
- it complains about forward declaration in .c, but without it, it does
  not compile due to additional warnings in cflags
- it complains that modified symbols should be marked as experimental

Thanks to David for helping me to test and fix the ABI part of the
patchset.

[1] http://patchwork.dpdk.org/patch/64369/

Olivier Matz (3):
  mempool: fix mempool virt populate with small chunks
  doc: announce API change for mempool IOVA populate
  mempool: return 0 if area is too small on populate

 doc/guides/rel_notes/deprecation.rst       |  5 ++
 examples/ntb/ntb_fwd.c                     |  2 +-
 lib/librte_mempool/meson.build             |  1 +
 lib/librte_mempool/rte_mempool.c           | 88 ++++++++++++++++++++--
 lib/librte_mempool/rte_mempool.h           | 14 +++-
 lib/librte_mempool/rte_mempool_version.map |  7 ++
 6 files changed, 105 insertions(+), 12 deletions(-)

Comments

Thomas Monjalon Jan. 20, 2020, 12:02 p.m. UTC | #1
17/01/2020 15:57, Olivier Matz:
> rte_mempool_populate_virt() sometimes fail, when it calls
> rte_mempool_populate_iova() with an area which is too small to store one
> object. This should not be an error.
> 
> I prepared a v2 which implements an ABI compatibility through symbol
> versioning, as suggested [1]. It looks a bit overkill to me, but it
> was an interresting exercice.
[...]
> Olivier Matz (3):
>   mempool: fix mempool virt populate with small chunks
>   doc: announce API change for mempool IOVA populate
>   mempool: return 0 if area is too small on populate

Applied patches 1 & 2 (with commit log fixed)
Patch 3 is for 20.05.