Re:Re: [PATCH] mm:slub:avoid wake up kswapd in set_track_prepare

[yangshiguang <yangshiguang1011@163.com>]

At 2025-08-04 07:39:26, "Harry Yoo" <harry.yoo@oracle.com> wrote:
>On Sat, Aug 02, 2025 at 04:44:54PM +0800, yangshiguang wrote:
>> 
>> 
>> At 2025-08-01 17:33:31, "Harry Yoo" <harry.yoo@oracle.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@163.com wrote:
>> >> From: yangshiguang <yangshiguang1011@163.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@163.com>
>> >> ---
>> >
>> >In general,
>> >Reviewed-by: Harry Yoo <harry.yoo@oracle.com>
>> >
>> 
>> Thanks.
>> 
>> >Let's add Fixes: 5cf909c553e9 ("mm/slub: use stackdepot to save stack
>> >trace in objects") and potentially Cc: stable@vger.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
>

Thank you for your detailed explanation.
I will update asap. Thanks again for the review.

>-- 
>Cheers,
>Harry / Hyeonggon