On Wed, Nov 05, 2025 at 04:04:53PM +0100, Vlastimil Babka wrote:
> > + for (; i < count; i++) {
> > + if (!elem[i]) {
> > + if (should_fail_ex(&fail_mempool_alloc, 1,
> > + FAULT_NOWARN)) {
> > + pr_info("forcing pool usage for pool %pS\n",
> > + (void *)caller_ip);
> > + goto use_pool;
> > + }
>
> Would it be enough to do this failure injection attempt once and not in
> every iteration?
Well, that would only test failure handling for the first element. Or
you mean don't call it again if called once?
> > /*
> > @@ -445,10 +463,12 @@ void *mempool_alloc_noprof(mempool_t *pool, gfp_t
> > gfp_mask)
> > /* We must not sleep if !__GFP_DIRECT_RECLAIM */
> > if (!(gfp_mask & __GFP_DIRECT_RECLAIM)) {
> > spin_unlock_irqrestore(&pool->lock, flags);
> > - return NULL;
> > + if (i > 0)
> > + mempool_free_bulk(pool, elem + i, count - i);
>
> I don't understand why we are trying to free from i to count and not from 0
> to i? Seems buggy, there will likely be NULLs which might go through
> add_element() which assumes they are not NULL.
Yes, this looks like broken copy and paste. The again I'm not even
sure who calls into mempool without __GFP_DIRECT_RECLAIM reset, as
that's kinda pointless.
> Assuming this is fixed we might still have confusing API. We might be
> freeing away elements that were already in the array when
> mempool_alloc_bulk() was called. OTOH the pool might be missing less than i
> elements and mempool_free_bulk() will not do anything with the rest.
> Anything beyond i is untouched. The caller has no idea what's in the array
> after getting this -ENOMEM. (alloc_pages_bulk() returns the number of pages
> there).
> Maybe it's acceptable (your usecase I think doesn't even add a caller that
> can't block), but needs documenting clearly.
I'm tempted to just disallow !__GFP_DIRECT_RECLAIM bulk allocations.
That feature seems to being a lot of trouble for no real gain, as
we can't use mempool as a guaranteed allocator there, so it's kinda
pointless.
> So in theory callers waiting for many objects might wait indefinitely to
> find enough objects in the pool, while smaller callers succeed their
> allocations and deplete the pool. Mempools never provided some fair ordering
> of waiters, but this might make it worse deterministically instead of
> randomly. Guess it's not such a problem if all callers are comparable in
> number of objects.
Yeah, which is the use case.
> > * This function only sleeps if the free_fn callback sleeps.
>
> This part now only applies to mempool_free() ?
Both mempool_free and mempool_free_bulk.
