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

[yangshiguang <yangshiguang1011@163.com>]

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?

>-- 
>Cheers,
>Harry / Hyeonggon