Message-ID: <aI_zLqBtvp3gHk29@hyeyoo>
Date: Mon, 4 Aug 2025 08:39:26 +0900
From: Harry Yoo <harry.yoo@...cle.com>
To: yangshiguang <yangshiguang1011@....com>
Cc: vbabka@...e.cz, akpm@...ux-foundation.org, cl@...two.org,
        rientjes@...gle.com, roman.gushchin@...ux.dev, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org, alexei.starovoitov@...il.com
Subject: Re: Re: [PATCH] mm:slub:avoid wake up kswapd in set_track_prepare

On Sat, Aug 02, 2025 at 04:44:54PM +0800, yangshiguang wrote:
> 
> 
> At 2025-08-01 17:33:31, "Harry Yoo" <harry.yoo@...cle.com> wrote:
> >nit: the subject needs a whitespace between subsystems and the header.
> >"mm: slub: avoid waking up kswapd in set_track_prepare()"?
> >
> 
> Thanks for the reminder.
> 
> >On Fri, Aug 01, 2025 at 02:51:21PM +0800, yangshiguang1011@....com wrote:
> >> From: yangshiguang <yangshiguang1011@....com>
> >> 
> >> set_track_prepare() can incur lock recursion.
> >> The issue is that it is called from hrtimer_start_range_ns
> >> holding the per_cpu(hrtimer_bases)[n].lock, but when enabled
> >> CONFIG_DEBUG_OBJECTS_TIMERS, may wake up kswapd in set_track_prepare,
> >> and try to hold the per_cpu(hrtimer_bases)[n].lock.
> >> 
> >> So avoid waking up kswapd.The oops looks something like:
> >> 
> >> BUG: spinlock recursion on CPU#3, swapper/3/0
> >>  lock: 0xffffff8a4bf29c80, .magic: dead4ead, .owner: swapper/3/0, .owner_cpu: 3
> >> Hardware name: Qualcomm Technologies, Inc. Popsicle based on SM8850 (DT)
> >> Call trace:
> >> spin_bug+0x0
> >> _raw_spin_lock_irqsave+0x80
> >> hrtimer_try_to_cancel+0x94
> >> task_contending+0x10c
> >> enqueue_dl_entity+0x2a4
> >> dl_server_start+0x74
> >> enqueue_task_fair+0x568
> >> enqueue_task+0xac
> >> do_activate_task+0x14c
> >> ttwu_do_activate+0xcc
> >> try_to_wake_up+0x6c8
> >> default_wake_function+0x20
> >> autoremove_wake_function+0x1c
> >> __wake_up+0xac
> >> wakeup_kswapd+0x19c
> >> wake_all_kswapds+0x78
> >> __alloc_pages_slowpath+0x1ac
> >> __alloc_pages_noprof+0x298
> >> stack_depot_save_flags+0x6b0
> >> stack_depot_save+0x14
> >> set_track_prepare+0x5c
> >> ___slab_alloc+0xccc
> >> __kmalloc_cache_noprof+0x470
> >> __set_page_owner+0x2bc
> >> post_alloc_hook[jt]+0x1b8
> >> prep_new_page+0x28
> >> get_page_from_freelist+0x1edc
> >> __alloc_pages_noprof+0x13c
> >> alloc_slab_page+0x244
> >> allocate_slab+0x7c
> >> ___slab_alloc+0x8e8
> >> kmem_cache_alloc_noprof+0x450
> >> debug_objects_fill_pool+0x22c
> >> debug_object_activate+0x40
> >> enqueue_hrtimer[jt]+0xdc
> >> hrtimer_start_range_ns+0x5f8
> >> ...
> >
> >So some allocations can't even use __GFP_KSWAPD_RECLAIM (e.g., eb799279fb1
> >("debugobjects: Don't wake up kswapd from fill_pool()")) and
> >stack_depot_save() does not respect that.
> >
>
> yes,you are right.
>
> >> Signed-off-by: yangshiguang <yangshiguang1011@....com>
> >> ---
> >
> >In general,
> >Reviewed-by: Harry Yoo <harry.yoo@...cle.com>
> >
> 
> Thanks.
> 
> >Let's add Fixes: 5cf909c553e9 ("mm/slub: use stackdepot to save stack
> >trace in objects") and potentially Cc: stable@...r.kernel.org too?
> >(It's hard to imagine use both configs in production, though)
> >
> Ok,it is necessary.
> >>  mm/slub.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >> 
> >> diff --git a/mm/slub.c b/mm/slub.c
> >> index cf7c6032d5fd..14e3bac0c6ad 100644
> >> --- a/mm/slub.c
> >> +++ b/mm/slub.c
> >> @@ -969,7 +969,7 @@ static noinline depot_stack_handle_t set_track_prepare(void)
> >>  	unsigned int nr_entries;
> >>  
> >>  	nr_entries = stack_trace_save(entries, ARRAY_SIZE(entries), 3);
> >> -	handle = stack_depot_save(entries, nr_entries, GFP_NOWAIT);
> >> +	handle = stack_depot_save(entries, nr_entries, __GFP_NOWARN);
> >
> >In the future, perhaps it might be better to propagate gfp flags to
> >set_track_prepare() and pass it to stack_depot_save()? That's what KASAN
> >does.
> >
> 
> Thanks for your advice.This might be a good idea.
> If only CONFIG_DEBUG_OBJECTS_TIMERS is enabled, there is a
> risk of recursive lock. Can __GFP_KSWAPD_RECLAIM be removed
> in this case? Just like:
> 
> diff --git a/mm/slub.c b/mm/slub.c
> index cf7c6032d5fd..3b35b6cbdd40 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -967,9 +967,17 @@ static noinline depot_stack_handle_t set_track_prepare(void)
>         depot_stack_handle_t handle;
>         unsigned long entries[TRACK_ADDRS_COUNT];
>         unsigned int nr_entries;
> +       gfp_t flags = GFP_NOWAIT;
> +
> +#ifdef CONFIG_DEBUG_OBJECTS_TIMERS
> +       /*
> +        * Don't wake up kswapd, to avoid potential recursive lock.
> +        */
> +       flags &= ~__GFP_KSWAPD_RECLAIM;
> +#endif
>  
>         nr_entries = stack_trace_save(entries, ARRAY_SIZE(entries), 3);
> -       handle = stack_depot_save(entries, nr_entries, GFP_NOWAIT);
> +       handle = stack_depot_save(entries, nr_entries, flags);
>  
>         return handle;
>  }
> 
> What do you think of?

Oh, I should have been more clear.

I meant propagating gfp flags that's passed to the kmalloc() or
kmem_cache_alloc() interface to stack_depot_save(), something like:

__slab_alloc(gfpflags)
-> set_track(gfpflags)
-> set_track_prepare(gfpflags)
-> stack_depot_save(gfpflags)

Current code assumes that GFP_NOWAIT is safe to use regardless of
context. This is already problem as it turns out that assumption
does not hold when we use CONFIG_DEBUG_OBJECTS_TIMERS.

Also, in the near future we shouldn't even assume that current context can
use the __GFP_KSWAPD_RECLAIM flag, because the flag means we can spin on locks
and kmalloc_nolock() [1] users can't spin. (see gfpflags_allow_spinning()).

I think it'd be better to use the gfp flag passed by the user instead of
relying on the assumption that GFP_NOWAIT is safe to use in any context. 

[1] https://lore.kernel.org/linux-mm/20250718021646.73353-1-alexei.starovoitov@gmail.com

-- 
Cheers,
Harry / Hyeonggon

Powered by blists - more mailing lists