Re: [PATCH 1/2] mm/percpu: Preserve NOFS/NOIO scope during chunk create and populate

[Dennis Zhou <dennis@kernel.org>]

Hello,

On Sat, May 30, 2026 at 08:47:34PM +0800, Kaitao Cheng wrote:
> 在 2026/5/29 17:38, Pedro Falcato 写道:
> > On Fri, May 29, 2026 at 10:25:28AM +0100, Pedro Falcato wrote:
> >> On Thu, May 28, 2026 at 09:29:16PM +0800, Kaitao Cheng wrote:
> >>> From: Kaitao Cheng <chengkaitao@kylinos.cn>
> >>>
> >>> pcpu_alloc_noprof() derives pcpu_gfp from the caller supplied GFP mask and
> >>> passes it to the backing percpu allocators. This preserves GFP_NOFS and
> >>> GFP_NOIO for pcpu_alloc_pages() and for the initial pcpu_chunk allocation.
> >>>
> >>> However, the chunk creation and population slow paths also call helpers
> >>> which do not take a GFP mask and perform internal allocations with
> >>> GFP_KERNEL. For example, pcpu_create_chunk() calls pcpu_get_vm_areas(),
> >>> and population can allocate temporary metadata or page tables while mapping
> >>> backing pages. As a result, a caller which explicitly uses GFP_NOFS or
> >>> GFP_NOIO can still enter FS or IO reclaim while creating or populating a
> >>> percpu chunk.
> >>>
> >>> This is problematic for callers which use GFP_NOFS or GFP_NOIO because
> >>> they are already holding filesystem or IO-path locks. If free chunks are
> >>> exhausted, the percpu allocation can take pcpu_alloc_mutex and then enter
> >>> unconstrained reclaim from these internal allocations, defeating the
> >>> caller's allocation context and potentially recreating reclaim lock
> >>> dependencies.
> >>>
> >>> Wrap chunk creation and population in a scoped NOIO or NOFS context when
> >>> pcpu_gfp has the corresponding constraints. Leave ordinary GFP_KERNEL
> >>> allocations unchanged so they retain full reclaim capability.
> >>>
> >>> Fixes: 9a5b183941b5 ("mm, percpu: do not consider sleepable allocations atomic")
> >>
> >> I assume you _did not_ observe this in production? As in no reclaim path should be
> >> insane^W daring enough to do pcpu allocations?
> > 
> > Oops, I mixed my issues up. This is purely a GFP flags issue. A quick
> > "git grep alloc_percpu_gfp" shows that the vast majority (all?) callers are
> > using some combination of GFP_KERNEL or GFP_ATOMIC + other GFP flags, but no
> > NOFS or NOIO as far as I can see. So you probably did not observe this?
> 
> Right, this issue has not been observed in production. It came from a
> question raised by AI code review, and after carefully reading the code,
> I found that there are indeed some synchronization concerns.
> 
> Here is one example of the scenario [PATCH 1/2] is trying to address:
> 
> blk-cgroup after 5d726c4dbeed ("blk-cgroup: fix possible deadlock while
> configuring policy"). blkg_conf_prep() now serializes against
> blkcg_deactivate_policy() with q->blkcg_mutex, and blkg_alloc() was
> changed to GFP_NOIO for that reason:
> 
>   CPU0: blkg_conf_prep()
>     mutex_lock(q->blkcg_mutex)
>     blkg_alloc(..., GFP_NOIO)
>       alloc_percpu_gfp(..., GFP_NOIO)
>         -> if percpu chunks are exhausted, chunk create/populate may do
>            internal GFP_KERNEL allocations
>         -> direct reclaim / writeback can issue IO to this queue
>         -> IO waits because the queue is frozen
> 
>   CPU1: blkcg_deactivate_policy()
>     blk_mq_freeze_queue(q)
>     mutex_lock(q->blkcg_mutex)
>       -> waits for CPU0
>     ... unfreeze only happens after q->blkcg_mutex is acquired/released
> 
> So the concern is that the caller deliberately uses GFP_NOIO because it
> may hold a lock which can be acquired after queue freeze, but the percpu
> slow path can temporarily lose that allocation context. The failure
> requires the slow path (new chunk creation or population), so it is not
> expected to be common.

This is likely just a miss for [1] where we switching to
gfpflags_allow_blocking(),

This seems like it's just a miss from [1] where we switched to a less
conservative approach than atomic == !GFP_KERNEL. If anything we just
need to allow the additional flags through. Maybe just add GFP_NOIO and
GFP_NOFS in pcpu_gfp via the whitelist.

[1] 9a5b183941b ("mm, percpu: do not consider sleepable allocations atomic")
> 
> >>> Signed-off-by: Kaitao Cheng <chengkaitao@kylinos.cn>
> >>> ---
> >>>  mm/percpu.c | 26 ++++++++++++++++++++++++++
> >>>  1 file changed, 26 insertions(+)
> >>>
> >>> diff --git a/mm/percpu.c b/mm/percpu.c
> >>> index 71a85d7245c7..1bb38467390b 100644
> >>> --- a/mm/percpu.c
> >>> +++ b/mm/percpu.c
> >>> @@ -1778,6 +1778,23 @@ static void pcpu_alloc_tag_free_hook(struct pcpu_chunk *chunk, int off, size_t s
> >>>  }
> >>>  #endif
> >>>  
> >>> +static unsigned int pcpu_memalloc_scope_save(gfp_t gfp)
> >>> +{
> >>> +	if (!(gfp & __GFP_IO))
> >>> +		return memalloc_noio_save();
> >>> +	if (!(gfp & __GFP_FS))
> >>> +		return memalloc_nofs_save();
> >>> +	return 0;
> >>> +}
> >>> +
> >>> +static void pcpu_memalloc_scope_restore(gfp_t gfp, unsigned int flags)
> >>> +{
> >>> +	if (!(gfp & __GFP_IO))
> >>> +		memalloc_noio_restore(flags);
> >>> +	else if (!(gfp & __GFP_FS))
> >>> +		memalloc_nofs_restore(flags);
> >>> +}
> >>
> >> I disagree with this. We already have gfp flags, they're already passed to pcpu_create_chunk()
> >> and pcpu_populate_chunk(). It's their job to respect the gfp flags and
> >> Do The Right Thing(tm). Can you fix the problematic places? It seems like it's
> >> mostly the vmalloc backend that's problematic.
> 
> I’ll try to do it.
> 
> Following your suggestion, including in [PATCH 2/2], I will also try a
> different approach and fix the issue by reducing the scope of the
> pcpu_alloc_mutex critical section.
> 

No please don't. The point of the percpu mutex is to ensure that only
one person is ever possibly creating a new chunk. If you drop the mutex,
then you have to deal with concurrent callers when available percpu
memory is low. Percpu memory is expensive and unmovable so the cost is
in the control plane to avoid excess fragmentation.

Thanks,
Dennis