[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <a62a89ae-dc91-412e-85a5-f92064d4a79e@suse.com>
Date: Thu, 17 Jul 2025 10:07:24 +0800
From: Yadan Fan <ydfan@...e.com>
To: Andrew Morton <akpm@...ux-foundation.org>
Cc: linux-mm@...ck.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] mm: mempool: fix wake-up edge case bug for zero-minimum
pools
Hi Andrew,
On 7/17/25 05:19, Andrew Morton wrote:
> On Wed, 16 Jul 2025 23:37:30 +0800 Yadan Fan <ydfan@...e.com> wrote:
>
>> The mempool wake-up mechanism has a edge case bug that affects pools
>> created with min_nr=0. When a thread blocks waiting for memory from an
>> empty pool (curr_nr == 0), subsequent mempool_free() calls fail to wake
>> the waiting thread because the condition "curr_nr < min_nr" evaluates
>> to "0 < 0" which is false, this causes threads to sleep indefinitely.
>>
>> There is at least 2 places where the mempool created with min_nr=0:
>>
>> 1. lib/btree.c:191: mempool_create(0, btree_alloc, btree_free, NULL)
>> 2. drivers/md/dm-verity-fec.c:791:
>> mempool_init_slab_pool(&f->extra_pool, 0, f->cache)
>
> This is very old code. Can you suggest why this has taken so long to
> surface?
>
> Which is a roundabout way of asking "should this be backported into
> -stable kernels". For that we'd need to know how this issue is
> affecting our users.
There is no real issue yet, I just reviewed the codes here and found this,
I thought it may needs to fix so that I sent this patch.
>
>> Add an explicit check in mempool_free() to handle the min_nr=0 case:
>> when the pool has zero minimum reserves, is currently empty, and has
>> active waiters, wake them up. The wq_has_sleeper() avoids unnecessary
>> wake-ups when no threads are waiting.
>
> Do we need the separate test? What's wrong with the obvious approach
> of replacing the "<" with "<=" in the preceding test?
Simply changing to "<=" has problem since add_element() has
"BUG_ON(pool->curr_nr >= pool->min_nr);".
>
> And would the previous (ie, existing) test benefit from the
> wq_has_sleeper() check?
I think it could have benefit for the existing test, wq_has_sleeper() is
cost cheaper than wake_up().
I will submit a new patch containing it.
>
>> --- a/mm/mempool.c
>> +++ b/mm/mempool.c
>> @@ -545,6 +545,22 @@ void mempool_free(void *element, mempool_t *pool)
>> }
>> spin_unlock_irqrestore(&pool->lock, flags);
>> }
>> + /*
>> + * Handle the min_nr = 0 edge case:
>> + * For zero-minimum pools, curr_nr < min_nr (0 < 0) never succeeds,
>> + * so waiters sleeping on pool->wait would never be woken by the
>> + * normal wake-up path. This explicit check ensures that when
>> + * pool->min_nr == 0 and pool->curr_nr == 0, any active waiters
>> + * are properly awakened.
>> + * The wq_has_sleeper() avoids unnecessary wake-ups when no
>> + * threads are waiting.
>> + */
>> + if (unlikely(pool->min_nr == 0 &&
>> + READ_ONCE(pool->curr_nr) == 0 &&
>> + wq_has_sleeper(&pool->wait))) {
>> + wake_up(&pool->wait);
>> + }
>> +
>
> Something strange is happening with the whitespace here. I pretty much
> retyped the patch. Please have a chat with your email client ;)
>
Sorry for this, I may just messed up somehow my client configuration, will fix it.
Thanks,
Yadan
Powered by blists - more mailing lists