Re: [PATCH] cgroup/dmem: return -ENOMEM on failed pool preallocation

[Guopeng Zhang <zhangguopeng@kylinos.cn>]

在 2026/5/11 21:03, Michal Koutný 写道:
> On Mon, May 11, 2026 at 09:31:50AM +0800, Guopeng Zhang <zhangguopeng@kylinos.cn> wrote:
>> get_cg_pool_unlocked() handles allocation failures under dmemcg_lock by
>> dropping the lock, preallocating a pool with GFP_KERNEL, and retrying the
>> locked lookup and creation path.
>>
>> If the fallback allocation fails too, pool remains NULL. Since the loop
>> condition is while (!pool), the function can keep retrying instead of
>> propagating the allocation failure to the caller.
> 
> This implies that it's OK when the function keeps retrying with
> allocpool != NULL (and repeated WARN_ON()s)?
Hi Michal,

Thanks for taking a look.

No, that was not what I meant to imply. The commit message was not precise
enough there.

The intended normal retry is only for the case where the GFP_NOWAIT
allocation under dmemcg_lock fails. In that case, get_cg_pool_unlocked()
drops the lock, preallocates one pool with GFP_KERNEL, and the next locked
retry consumes that preallocated pool and clears allocpool.

So allocpool != NULL together with another -ENOMEM return is not expected to
be a normal retry path. The WARN_ON(allocpool) branch looks defensive, and I
agree that repeatedly continuing from there would not be useful if it ever
fired.

>> Set pool to ERR_PTR(-ENOMEM) when the fallback allocation fails so the
>> loop exits through the existing common return path. The callers already
>> handle ERR_PTR() from get_cg_pool_unlocked(), so this restores the
>> expected error path.
> 
> If the callers can handle it, maybe there's no need to retry at all.
> Perhpas dmem fellows can step in here.My understanding is that the retry still has a purpose independent of the
callers' ability to handle ERR_PTR().

The first allocation attempt happens in alloc_pool_single() while
dmemcg_lock is held, so it uses GFP_NOWAIT. If that fails,
get_cg_pool_unlocked() drops the lock and preallocates one pool with the
default GFP_KERNEL context. The next locked retry then consumes that
preallocated pool instead of trying another GFP_NOWAIT allocation for that
pool.

So callers can handle the final ERR_PTR() result, but the fallback
preallocation gives the allocation a chance to succeed in a less
constrained context before reporting -ENOMEM. That said, whether this
retry policy is desirable is a dmem design question, so input from dmem
folks would be helpful.

>>
>> Fixes: b168ed458dde ("kernel/cgroup: Add "dmem" memory accounting cgroup")
>> Signed-off-by: Guopeng Zhang <zhangguopeng@kylinos.cn>
>> ---
>>  kernel/cgroup/dmem.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/kernel/cgroup/dmem.c b/kernel/cgroup/dmem.c
>> index 1ab1fb47f271..4753a67d0f0f 100644
>> --- a/kernel/cgroup/dmem.c
>> +++ b/kernel/cgroup/dmem.c
>> @@ -602,6 +602,7 @@ get_cg_pool_unlocked(struct dmemcg_state *cg, struct dmem_cgroup_region *region)
>>  				pool = NULL;
> 
> This 2nd pool zeroing seems pointless.
Agreed. 

Since Tejun has already applied the fix, I will wait for the discussion
before sending any follow-up. If we keep the current retry scheme, a small
cleanup can make this path clearer.

Thanks,
Guopeng