On 11/6/25 15:13, Christoph Hellwig wrote:

On Wed, Nov 05, 2025 at 04:04:53PM +0100, Vlastimil Babka wrote:

quoted

quoted

+	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.

quoted

quoted

 	/*

@@ -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.

quoted

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.

quoted

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.

quoted

quoted

  * 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.