* [PATCH] mm:slub:avoid wake up kswapd in set_track_prepare @ 2025-08-01 6:51 yangshiguang1011 2025-08-01 9:33 ` Harry Yoo 0 siblings, 1 reply; 5+ messages in thread From: yangshiguang1011 @ 2025-08-01 6:51 UTC (permalink / raw) To: vbabka Cc: akpm, cl, rientjes, roman.gushchin, harry.yoo, linux-mm, linux-kernel, yangshiguang 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 ... Signed-off-by: yangshiguang <yangshiguang1011@163.com> --- 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); return handle; } -- 2.43.0 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] mm:slub:avoid wake up kswapd in set_track_prepare 2025-08-01 6:51 [PATCH] mm:slub:avoid wake up kswapd in set_track_prepare yangshiguang1011 @ 2025-08-01 9:33 ` Harry Yoo 2025-08-02 8:44 ` yangshiguang 0 siblings, 1 reply; 5+ messages in thread From: Harry Yoo @ 2025-08-01 9:33 UTC (permalink / raw) To: yangshiguang1011 Cc: vbabka, akpm, cl, rientjes, roman.gushchin, linux-mm, linux-kernel nit: the subject needs a whitespace between subsystems and the header. "mm: slub: avoid waking up kswapd in set_track_prepare()"? 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. > Signed-off-by: yangshiguang <yangshiguang1011@163.com> > --- In general, Reviewed-by: Harry Yoo <harry.yoo@oracle.com> 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) > 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. -- Cheers, Harry / Hyeonggon ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re:Re: [PATCH] mm:slub:avoid wake up kswapd in set_track_prepare 2025-08-01 9:33 ` Harry Yoo @ 2025-08-02 8:44 ` yangshiguang 2025-08-03 23:39 ` Harry Yoo 0 siblings, 1 reply; 5+ messages in thread From: yangshiguang @ 2025-08-02 8:44 UTC (permalink / raw) To: Harry Yoo Cc: vbabka, akpm, cl, rientjes, roman.gushchin, linux-mm, linux-kernel 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Re: [PATCH] mm:slub:avoid wake up kswapd in set_track_prepare 2025-08-02 8:44 ` yangshiguang @ 2025-08-03 23:39 ` Harry Yoo 2025-08-04 11:34 ` yangshiguang 0 siblings, 1 reply; 5+ messages in thread From: Harry Yoo @ 2025-08-03 23:39 UTC (permalink / raw) To: yangshiguang Cc: vbabka, akpm, cl, rientjes, roman.gushchin, linux-mm, linux-kernel, alexei.starovoitov 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 -- Cheers, Harry / Hyeonggon ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re:Re: [PATCH] mm:slub:avoid wake up kswapd in set_track_prepare 2025-08-03 23:39 ` Harry Yoo @ 2025-08-04 11:34 ` yangshiguang 0 siblings, 0 replies; 5+ messages in thread From: yangshiguang @ 2025-08-04 11:34 UTC (permalink / raw) To: Harry Yoo Cc: vbabka, akpm, cl, rientjes, roman.gushchin, linux-mm, linux-kernel, alexei.starovoitov 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2025-08-04 11:34 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2025-08-01 6:51 [PATCH] mm:slub:avoid wake up kswapd in set_track_prepare yangshiguang1011 2025-08-01 9:33 ` Harry Yoo 2025-08-02 8:44 ` yangshiguang 2025-08-03 23:39 ` Harry Yoo 2025-08-04 11:34 ` yangshiguang
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox