Re: [PATCH] mm: slub: annotate kmem_cache_node->list_lock as raw_spinlock
From: Qi Zheng
Date: Wed Apr 12 2023 - 12:49:11 EST
On 2023/4/12 21:09, Waiman Long wrote:
On 4/12/23 04:32, Qi Zheng wrote:
On 2023/4/12 15:30, Qi Zheng wrote:
On 2023/4/12 14:50, Vlastimil Babka wrote:
On 4/12/23 08:44, Zhang, Qiang1 wrote:
On 2023/4/11 22:19, Vlastimil Babka wrote:
On 4/11/23 16:08, Qi Zheng wrote:
On 2023/4/11 21:40, Vlastimil Babka wrote:
On 4/11/23 15:08, Qi Zheng wrote:
The list_lock can be held in the critical section of
raw_spinlock, and then lockdep will complain about it
like below:
=============================
[ BUG: Invalid wait context ]
6.3.0-rc6-next-20230411 #7 Not tainted
-----------------------------
swapper/0/1 is trying to lock:
ffff888100055418 (&n->list_lock){....}-{3:3}, at:
___slab_alloc+0x73d/0x1330
other info that might help us debug this:
context-{5:5}
2 locks held by swapper/0/1:
#0: ffffffff824e8160
(rcu_tasks.cbs_gbl_lock){....}-{2:2}, at:
cblist_init_generic+0x22/0x2d0
#1: ffff888136bede50 (&ACCESS_PRIVATE(rtpcp,
lock)){....}-{2:2}, at: cblist_init_generic+0x232/0x2d0
stack backtrace:
CPU: 0 PID: 1 Comm: swapper/0 Not tainted
6.3.0-rc6-next-20230411 #7
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996),
BIOS 1.14.0-2 04/01/2014
Call Trace:
<TASK>
dump_stack_lvl+0x77/0xc0
__lock_acquire+0xa65/0x2950
? arch_stack_walk+0x65/0xf0
? arch_stack_walk+0x65/0xf0
? unwind_next_frame+0x602/0x8d0
lock_acquire+0xe0/0x300
? ___slab_alloc+0x73d/0x1330
? find_usage_forwards+0x39/0x50
? check_irq_usage+0x162/0xa70
? __bfs+0x10c/0x2c0
_raw_spin_lock_irqsave+0x4f/0x90
? ___slab_alloc+0x73d/0x1330
___slab_alloc+0x73d/0x1330
? fill_pool+0x16b/0x2a0
? look_up_lock_class+0x5d/0x160
? register_lock_class+0x48/0x500
? __lock_acquire+0xabc/0x2950
? fill_pool+0x16b/0x2a0
kmem_cache_alloc+0x358/0x3b0
? __lock_acquire+0xabc/0x2950
fill_pool+0x16b/0x2a0
? __debug_object_init+0x292/0x560
? lock_acquire+0xe0/0x300
? cblist_init_generic+0x232/0x2d0
__debug_object_init+0x2c/0x560
This "__debug_object_init" is because INIT_WORK() is called in
cblist_init_generic(), so..
cblist_init_generic+0x147/0x2d0
rcu_init_tasks_generic+0x15/0x190
kernel_init_freeable+0x6e/0x3e0
? rest_init+0x1e0/0x1e0
kernel_init+0x1b/0x1d0
? rest_init+0x1e0/0x1e0
ret_from_fork+0x1f/0x30
</TASK>
The fill_pool() can only be called in the !PREEMPT_RT kernel
or in the preemptible context of the PREEMPT_RT kernel, so
the above warning is not a real issue, but it's better to
annotate kmem_cache_node->list_lock as raw_spinlock to get
rid of such issue.
+ CC some RT and RCU people
Thanks.
AFAIK raw_spinlock is not just an annotation, but on RT it
changes the
implementation from preemptible mutex to actual spin lock, so
it would be
Yeah.
rather unfortunate to do that for a spurious warning. Can it be
somehow
fixed in a better way?
... probably a better fix is to drop locks and call INIT_WORK(),
or make
the cblist_init_generic() lockless (or part lockless), given it's
just
initializing the cblist, it's probably doable. But I haven't taken a
careful look yet.
This is just one of the paths that triggers an invalid wait, the
following paths can also trigger:
[ 129.914547] [ BUG: Invalid wait context ]
[ 129.914775] 6.3.0-rc1-yocto-standard+ #2 Not tainted
[ 129.915044] -----------------------------
[ 129.915272] kworker/2:0/28 is trying to lock:
[ 129.915516] ffff88815660f570 (&c->lock){-.-.}-{3:3}, at:
___slab_alloc+0x68/0x12e0
[ 129.915967] other info that might help us debug this:
[ 129.916241] context-{5:5}
[ 129.916392] 3 locks held by kworker/2:0/28:
[ 129.916642] #0: ffff888100084d48
((wq_completion)events){+.+.}-{0:0}, at: process_one_work+0x515/0xba0
[ 129.917145] #1: ffff888100c17dd0
((work_completion)(&(&krcp->monitor_work)->work)){+.+.}-{0:0}, at:
process_on0
[ 129.917758] #2: ffff8881565f8508 (krc.lock){....}-{2:2}, at:
kfree_rcu_monitor+0x29f/0x810
[ 129.918207] stack backtrace:
[ 129.918374] CPU: 2 PID: 28 Comm: kworker/2:0 Not tainted
6.3.0-rc1-yocto-standard+ #2
[ 129.918784] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009),
BIOS rel-1.16.1-0-g3208b098f51a-prebuilt.qemu.o4
[ 129.919397] Workqueue: events kfree_rcu_monitor
[ 129.919662] Call Trace:
[ 129.919812] <TASK>
[ 129.919941] dump_stack_lvl+0x64/0xb0
[ 129.920171] dump_stack+0x10/0x20
[ 129.920372] __lock_acquire+0xeb8/0x3a80
[ 129.920603] ? ret_from_fork+0x2c/0x50
[ 129.920824] ? __pfx___lock_acquire+0x10/0x10
[ 129.921068] ? unwind_next_frame.part.0+0x1ba/0x3c0
[ 129.921343] ? ret_from_fork+0x2c/0x50
[ 129.921573] ? __this_cpu_preempt_check+0x13/0x20
[ 129.921847] lock_acquire+0x194/0x480
[ 129.922060] ? ___slab_alloc+0x68/0x12e0
[ 129.922293] ? __pfx_lock_acquire+0x10/0x10
[ 129.922529] ? __pfx_mark_lock.part.0+0x10/0x10
[ 129.922778] ? __kasan_check_read+0x11/0x20
[ 129.922998] ___slab_alloc+0x9a/0x12e0
[ 129.923222] ? ___slab_alloc+0x68/0x12e0
[ 129.923452] ? __pfx_mark_lock.part.0+0x10/0x10
[ 129.923706] ? __kasan_check_read+0x11/0x20
[ 129.923937] ? fill_pool+0x22a/0x370
[ 129.924161] ? __lock_acquire+0xf5b/0x3a80
[ 129.924387] ? fill_pool+0x22a/0x370
[ 129.924590] __slab_alloc.constprop.0+0x5b/0x90
[ 129.924832] kmem_cache_alloc+0x296/0x3d0
[ 129.925073] ? fill_pool+0x22a/0x370
[ 129.925291] fill_pool+0x22a/0x370
[ 129.925495] ? __pfx_fill_pool+0x10/0x10
[ 129.925718] ? __pfx___lock_acquire+0x10/0x10
[ 129.926034] ? __kasan_check_read+0x11/0x20
[ 129.926269] ? check_chain_key+0x200/0x2b0
[ 129.926503] __debug_object_init+0x82/0x8c0
[ 129.926734] ? __pfx_lock_release+0x10/0x10
[ 129.926984] ? __pfx___debug_object_init+0x10/0x10
[ 129.927249] ? __kasan_check_read+0x11/0x20
[ 129.927498] ? do_raw_spin_unlock+0x9c/0x100
[ 129.927758] debug_object_activate+0x2d1/0x2f0
[ 129.928022] ? __pfx_debug_object_activate+0x10/0x10
[ 129.928300] ? __this_cpu_preempt_check+0x13/0x20
[ 129.928583] __call_rcu_common.constprop.0+0x94/0xeb0
[ 129.928897] ? __this_cpu_preempt_check+0x13/0x20
[ 129.929186] ? __pfx_rcu_work_rcufn+0x10/0x10
[ 129.929459] ? __pfx___call_rcu_common.constprop.0+0x10/0x10
[ 129.929803] ? __pfx_lock_acquired+0x10/0x10
[ 129.930067] ? __pfx_do_raw_spin_trylock+0x10/0x10
[ 129.930363] ? kfree_rcu_monitor+0x29f/0x810
[ 129.930627] call_rcu+0xe/0x20
[ 129.930821] queue_rcu_work+0x4f/0x60
[ 129.931050] kfree_rcu_monitor+0x5d3/0x810
[ 129.931302] ? __pfx_kfree_rcu_monitor+0x10/0x10
[ 129.931587] ? __this_cpu_preempt_check+0x13/0x20
[ 129.931878] process_one_work+0x607/0xba0
[ 129.932129] ? __pfx_process_one_work+0x10/0x10
[ 129.932408] ? worker_thread+0xd6/0x710
[ 129.932653] worker_thread+0x2d4/0x710
[ 129.932888] ? __pfx_worker_thread+0x10/0x10
[ 129.933154] kthread+0x18b/0x1c0
[ 129.933363] ? __pfx_kthread+0x10/0x10
[ 129.933598] ret_from_fork+0x2c/0x50
[ 129.933825] </TASK>
Maybe no need to convert ->list_lock to raw_spinlock.
--- a/lib/debugobjects.c
+++ b/lib/debugobjects.c
@@ -562,10 +562,10 @@ __debug_object_init(void *addr, const struct
debug_obj_descr *descr, int onstack
unsigned long flags;
/*
- * On RT enabled kernels the pool refill must happen in
preemptible
+ * The pool refill must happen in preemptible
* context:
*/
- if (!IS_ENABLED(CONFIG_PREEMPT_RT) || preemptible())
+ if (preemptible())
fill_pool();
+CC Peterz
Aha so this is in fact another case where the code is written with
actual differences between PREEMPT_RT and !PREEMPT_RT in mind, but
CONFIG_PROVE_RAW_LOCK_NESTING always assumes PREEMPT_RT?
Maybe we should make CONFIG_PROVE_RAW_LOCK_NESTING depend on
CONFIG_PREEMPT_RT:
I found a discussion [1] of why CONFIG_PROVE_RAW_LOCK_NESTING didn't
depend on CONFIG_PREEMPT_RT before in the commit history:
```
>>> We now always get a "Invalid wait context" warning with
>>> CONFIG_PROVE_RAW_LOCK_NESTING=y, see the full warning below:
>>>
>>> [ 0.705900] =============================
>>> [ 0.706002] [ BUG: Invalid wait context ]
>>> [ 0.706180] 5.13.0+ #4 Not tainted
>>> [ 0.706349] -----------------------------
>> I believe the purpose of CONFIG_PROVE_RAW_LOCK_NESTING is experimental
>> and it is turned off by default. Turning it on can cause problem as
>> shown in your lockdep splat. Limiting it to just PREEMPT_RT will
defeat
>> its purpose to find potential spinlock nesting problem in
non-PREEMPT_RT
>> kernel.
> As far as I know, a spinlock can nest another spinlock. In
> non-PREEMPT_RT kernel
> spin_lock and raw_spin_lock are same , so here acquiring a spin_lock
in hardirq
> context is acceptable, the warning is not needed. My knowledge on this
> is not enough,
> Will dig into this.
>
>> The point is to fix the issue found,
> Agree. I thought there was a spinlock usage issue, but by checking
> deactivate_slab context,
> looks like the spinlock usage is well. Maybe I'm missing something?
Yes, spinlock and raw spinlock are the same in non-RT kernel. They are
only different in RT kernel. However, non-RT kernel is also more heavily
tested than the RT kernel counterpart. The purpose of this config option
is to expose spinlock nesting problem in more areas of the code. If you
look at the config help text of PROVE_RAW_LOCK_NESTING:
help
Enable the raw_spinlock vs. spinlock nesting checks which
ensure
that the lock nesting rules for PREEMPT_RT enabled kernels are
not violated.
NOTE: There are known nesting problems. So if you enable this
option expect lockdep splats until these problems have been
fully
addressed which is work in progress. This config switch
allows to
identify and analyze these problems. It will be removed and the
check permanentely enabled once the main issues have been
fixed.
If unsure, select N.
So lockdep splat is expected. It will take time to address all the
issues found.
```
Also +Waiman Long.
I believe the purpose of not making PROVE_RAW_LOCK_NESTING depending on
PREEMPT_RT is to allow people to discover this kind of nest locking
problem without enabling PREEMPT_RT.
Anyway, I don't think you can change list_lock to a raw spinlock.
According to mm/slub.c:
* Lock order:
* 1. slab_mutex (Global Mutex)
* 2. node->list_lock (Spinlock)
* 3. kmem_cache->cpu_slab->lock (Local lock)
* 4. slab_lock(slab) (Only on some arches)
* 5. object_map_lock (Only for debugging)
For PREEMPT_RT, local lock is a per-cpu spinlock (rt_mutex). So
list_lock has to be spinlock also.
Got it. Thanks for such a detailed explanation!
Cheers,
Longman
--
Thanks,
Qi