On 11/6/25 15:13, Christoph Hellwig wrote:
> 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?
I mean since this is (due to the semantics of mempools) not really causing a
failure to the caller (unlike the typical failure injection usage), but
forcing preallocated objecs use, I'm not sure we get much benefit (in terms
of testing caller's error paths) from the fine grained selection of the
first element where we inject fail, and failing immediately or never should
be sufficient.
>> > /*
>> > @@ -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.
Hm yeah would have to be some special case where something limits how many
such outstanding allocations can there be, otherwise it's just a cache to
make success more likely but not guaranteed.
>> 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.
Agree. If anyone comes up with a use case they can extend and actually test
these rollback paths.
>> 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.
Good.
>> > * 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.
But mempool_free_bulk() doesn't use the callback, it's up to the caller to
free anything the mempool didn't use for its refill.
