Re: [PATCH v4 2/2] mempool: introduce pile driver

[Stephen Hemminger <stephen@networkplumber.org>]

On Tue,  1 Sep 2026 06:43:31 +0000
Morten Brørup <mb@smartsharesystems.com> wrote:

> Added a new "pile" mempool driver, based on the high-performance lock-free
> "pile" stack variant.
> 
> Changed other stack drivers to call their specific push/pop functions,
> instead of calling the generic stack push/pop API.
> 
> Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
> ---

FYI - pile driver is a good funny name https://en.wikipedia.org/wiki/Pile_driver

Did a AI review with Fable 5.1 as experiment

Review of [PATCH v4 0/2] stack/mempool: introduce pile

Applied on top of d55ccd4 ("pci: remove deprecated catch-all flag"),
built with -Dwerror=true, and ran stack_pile_autotest, stack_autotest
and mempool_autotest; all pass on x86_64.

Patch 1/2: stack: introduce pile

Warning: the pile does not honour the capacity given to
rte_stack_create(). Bulk and solo elements are separate resources
(ceil(count / RTE_STACK_PILE_BULK_SIZE) bulk elements plus count solo
elements), so the pile can hold objects in both pools at once. A pile
created with count = 32 accepts 32 single-object pushes, then a
further push of 32 succeeds, giving 64 objects stored while
rte_stack_free_count() reports 0. Because count is rounded up to a
whole number of bulk elements, a single push can also exceed
capacity: a pile created with count = 40 accepts one push of 64. The
RTE_MIN() in __rte_stack_pile_count() hides this rather than
preventing it:

	return RTE_MIN((unsigned int)s->capacity,
			__rte_stack_lf_elems_count(&s->stack_pile.bulk) * RTE_STACK_PILE_BULK_SIZE +
			__rte_stack_lf_elems_count(&s->stack_pile.solo));

This is harmless for the mempool driver, which never holds more than
mp->size objects, but RTE_STACK_F_PILE is a public flag on an API
documented as a bounded stack, and callers that rely on push failing
at capacity (as test_stack_basic() does) get different behaviour
depending on how the objects were pushed. Either account for capacity
explicitly, or document in the RTE_STACK_F_PILE Doxygen and in
stack_lib.rst that the pile is only bounded when used through the
mempool driver.

Warning: the change to RTE_MEMPOOL_MAX_OPS_IDX in
lib/mempool/rte_mempool.h is unrelated to the stack library, is not
mentioned in the commit message, and is only needed by patch 2. After
this series there are 17 RTE_MEMPOOL_REGISTER_OPS() sites in the
tree, so without the bump the pile driver's constructor fails with
-ENOSPC. Please move it into the mempool patch (or a separate
preparatory mempool patch) and say why in the log.

Warning: no release note. A new public stack flag and a new mempool
driver both need an entry in doc/guides/rel_notes/release_26_11.rst.

Info: the deterministic tests only cover bulk sizes 1 and MAX_BULK.
The fragmentation path (__rte_stack_pile_pop_frag) and the bulk-pop
retry path are only reached through the random-size multi-thread
test, which does not check contents. A short sequence such as push 32
then pop 5, pop 27, and push 5 then push 32 then pop 37 would cover
both paths with a known expected result.

Info: in test_stack_push_pop() the "goto lifo" jumps backwards into
the body of the preceding if block. It is legal C, but restructuring
so that the LIFO check is a helper called from both branches would be
easier to read.

Patch 2/2: mempool: introduce pile driver

Warning: the changes to rte_mempool_register_ops() and
rte_mempool_set_ops_byname() (duplicate-name rejection, the strlen
limit changing from >= to >, the new -ENAMETOOLONG return, log level
changes) are unrelated to the pile driver and are not mentioned in
the commit message. The strlen change also alters behaviour: names of
exactly RTE_MEMPOOL_OPS_NAMESIZE - 1 characters were previously
rejected and are now accepted. These belong in a separate mempool
patch with their own explanation.

Review-Result: WARNING