Re: [PATCH v4 4/4] mm/percpu: Avoid IO/FS reclaim in backing allocations

[Michal Hocko <mhocko@suse.com>]

On Fri 19-06-26 08:21:51, Kaitao Cheng wrote:
> 在 2026/6/19 02:03, Michal Hocko 写道:
> > On Thu 18-06-26 21:04:14, Kaitao Cheng wrote:
> >> From: Kaitao Cheng <chengkaitao@kylinos.cn>
> >>
> >> Commit 9a5b183941b5 ("mm, percpu: do not consider sleepable
> >> allocations atomic") allows sleepable GFP_NOIO and GFP_NOFS percpu
> >> allocations to take pcpu_alloc_mutex.  This avoids premature allocation
> >> failures, but it also makes the mutex visible to callers from constrained
> >> IO/FS contexts.
> >>
> >> Thread A calls pcpu_alloc_noprof() with GFP_KERNEL and takes
> >> pcpu_alloc_mutex. Since the internal allocation is not constrained by
> >> NOFS, it may enter FS reclaim while still holding pcpu_alloc_mutex,
> >> creating a dependency like: pcpu_alloc_mutex -> fs_reclaim -> FS lock
> >>
> >> At the same time, Thread B may already hold an FS lock and then call
> >> pcpu_alloc_noprof() with GFP_NOFS. It will try to acquire
> >> pcpu_alloc_mutex and block, creating the reverse dependency:
> >> FS lock -> pcpu_alloc_mutex
> >>
> >> This can still form a potential deadlock cycle.
> >>
> >> Avoid the dependency by restricting percpu backing allocations to GFP_NOIO.
> >> The public allocation still uses the caller's GFP context to decide whether
> >> it may block, but the internal memory allocations performed while
> >> pcpu_alloc_mutex is held cannot recurse into IO or FS reclaim.
> >>
> >> Fixes: 9a5b183941b5 ("mm, percpu: do not consider sleepable allocations atomic")
> >> Signed-off-by: Kaitao Cheng <chengkaitao@kylinos.cn>
> > 
> > This seems like the only viable short term fix but long term it would be
> > really better to make allocations outside of the lock.
> > Acked-by: Michal Hocko <mhocko@suse.com>
> > 
> > Minor nit
> >> @@ -1749,8 +1748,17 @@ void __percpu *pcpu_alloc_noprof(size_t size, size_t align, bool reserved,
> >>  	size_t bits, bit_align;
> >>  
> >>  	gfp = current_gfp_context(gfp);
> >> -	/* whitelisted flags that can be passed to the backing allocators */
> >> -	pcpu_gfp = gfp & (GFP_KERNEL | __GFP_NORETRY | __GFP_NOWARN);
> >> +	/*
> >> +	 * Allowlisted flags that can be passed to the backing allocators.
> >> +	 * Backing allocations under pcpu_alloc_mutex must not recurse into
> >> +	 * IO/FS reclaim.  Otherwise a GFP_KERNEL caller holding the mutex can
> >> +	 * block on reclaim while a GFP_NOIO/NOFS caller holding an IO/FS lock
> >> +	 * waits for the same mutex.
> >> +	 *
> >> +	 * Do not pass __GFP_NOFAIL.  A small percpu allocation may need many
> >> +	 * backing pages, making nofail reclaim too costly under NOIO/NOFS.
> >> +	 */
> >> +	pcpu_gfp = gfp & (GFP_NOIO | __GFP_NORETRY | __GFP_NOWARN);
> > 
> > GFP_NOIO, NOFS are negative masks in the sense that that are lacking
> > flags so the overal intention would be more readable IMHO in the
> > following form
> > 	pcpu_gfp = gfp & (GFP_KERNEL | __GFP_NORETRY | __GFP_NOWARN)
> > 	pcpu_gfp &= ~(__GFP_IO | __GFP_FS)
> 
> This looks a bit redundant. The newly added comment already makes the
> intent clear, and the extra code seems to serve only as another hint to
> readers, which is essentially the same role as the comment.
> 
> GFP_NOIO already excludes __GFP_IO and __GFP_FS, so its semantics are
> clear enough. It should not be misleading, and it is also more concise.

I will certainly not insist, but this is a generally used pattern to
drop IO and FS flags. So if you want to grep for the pattern you will
not miss this place. Comment _is_ useful but harder to grep for.

> 
> >>  	is_atomic = !gfpflags_allow_blocking(gfp);
> >>  	do_warn = !(gfp & __GFP_NOWARN);
> >>  
> >> -- 
> >> 2.50.1 (Apple Git-155)
> > 
> 
> -- 
> Thanks
> Kaitao Cheng

-- 
Michal Hocko
SUSE Labs