bpf: Reject bpf_timer for PREEMPT_RT
Subsystems (4)
v2 cover bpf: Reject bpf_timer for PREEMPT_RT
While running './test_progs -t timer' to validate the test case from "selftests/bpf: Introduce experimental bpf_in_interrupt()"[0] for PREEMPT_RT, I encountered a kernel panic.
To address this, reject bpf_timer usage in the verifier when PREEMPT_RT is enabled, and skip the corresponding timer selftests.
Changes:
v1 -> v2:
* Skip test case 'timer_interrupt'.
Links:
[0] https://lore.kernel.org/bpf/[email protected]/
Leon Hwang (2):
bpf: Reject bpf_timer for PREEMPT_RT
selftests/bpf: Skip timer cases when bpf_timer is not supported
kernel/bpf/verifier.c | 4 ++++ tools/testing/selftests/bpf/prog_tests/free_timer.c | 4 ++++ tools/testing/selftests/bpf/prog_tests/timer.c | 8 ++++++++ tools/testing/selftests/bpf/prog_tests/timer_crash.c | 4 ++++ tools/testing/selftests/bpf/prog_tests/timer_lockup.c | 4 ++++ tools/testing/selftests/bpf/prog_tests/timer_mim.c | 4 ++++ 6 files changed, 28 insertions(+)
--
2.51.0
PATCH v2 1/2 bpf: Reject bpf_timer for PREEMPT_RT
When enable CONFIG_PREEMPT_RT, the kernel will panic when run timer selftests by './test_progs -t timer':
[ 35.955287] BUG: sleeping function called from invalid context at kernel/locking/spinlock_rt.c:48
[ 35.955312] in_atomic(): 1, irqs_disabled(): 1, non_block: 0, pid: 120, name: test_progs
[ 35.955315] preempt_count: 1, expected: 0
[ 35.955316] RCU nest depth: 0, expected: 0
[ 35.955317] 2 locks held by test_progs/120:
[ 35.955319] #0: ffffffff8f1c3720 (rcu_read_lock_trace){....}-{0:0}, at: bpf_prog_test_run_syscall+0xc9/0x240
[ 35.955358] #1: ffff9155fbd331c8 ((&c->lock)){+.+.}-{3:3}, at: ___slab_alloc+0xb0/0xd20
[ 35.955388] irq event stamp: 100
[ 35.955389] hardirqs last enabled at (99): [<ffffffff8dfcd890>] do_syscall_64+0x30/0x2d0
[ 35.955414] hardirqs last disabled at (100): [<ffffffff8d4a9baa>] __bpf_async_init+0xca/0x310
[ 35.955428] softirqs last enabled at (0): [<ffffffff8d296cbb>] copy_process+0x9db/0x2000
[ 35.955449] softirqs last disabled at (0): [<0000000000000000>] 0x0
[ 35.955482] CPU: 1 UID: 0 PID: 120 Comm: test_progs Tainted: G OE 6.17.0-rc1-gc5f5af560d8a #30 PREEMPT_{RT,(full)}
[ 35.955487] Tainted: [O]=OOT_MODULE, [E]=UNSIGNED_MODULE
[ 35.955488] Hardware name: QEMU Ubuntu 24.04 PC (i440FX + PIIX, 1996), BIOS 1.16.3-debian-1.16.3-2 04/01/2014
[ 35.955491] Call Trace:
[ 35.955493] <TASK>
[ 35.955499] dump_stack_lvl+0x73/0xb0
[ 35.955514] dump_stack+0x14/0x20
[ 35.955518] __might_resched+0x167/0x230
[ 35.955537] rt_spin_lock+0x66/0x180
[ 35.955543] ? ___slab_alloc+0xb0/0xd20
[ 35.955549] ? bpf_map_kmalloc_node+0x7c/0x200
[ 35.955560] ___slab_alloc+0xb0/0xd20
[ 35.955575] ? __lock_acquire+0x43d/0x2590
[ 35.955601] __kmalloc_node_noprof+0x10b/0x410
[ 35.955605] ? __kmalloc_node_noprof+0x10b/0x410
[ 35.955607] ? bpf_map_kmalloc_node+0x7c/0x200
[ 35.955616] bpf_map_kmalloc_node+0x7c/0x200
[ 35.955624] __bpf_async_init+0xf8/0x310
[ 35.955633] bpf_timer_init+0x37/0x40
[ 35.955637] bpf_prog_2287350dd5909839_start_cb+0x5d/0x91
[ 35.955642] bpf_prog_0d54653d8a74e954_start_timer+0x65/0x8a
[ 35.955650] bpf_prog_test_run_syscall+0x111/0x240
[ 35.955660] __sys_bpf+0x81c/0x2ab0
[ 35.955665] ? __might_fault+0x47/0x90
[ 35.955700] __x64_sys_bpf+0x1e/0x30
[ 35.955703] x64_sys_call+0x171d/0x20d0
[ 35.955715] do_syscall_64+0x6a/0x2d0
[ 35.955722] entry_SYSCALL_64_after_hwframe+0x76/0x7e
[ 35.955728] RIP: 0033:0x7fee4261225d
[ 35.955734] Code: ff c3 66 2e 0f 1f 84 00 00 00 00 00 90 f3 0f 1e fa 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 8b bb 0d 00 f7 d8 64 89 01 48
[ 35.955736] RSP: 002b:00007fee424e5bd8 EFLAGS: 00000202 ORIG_RAX: 0000000000000141
[ 35.955742] RAX: ffffffffffffffda RBX: 00007fee424e6cdc RCX: 00007fee4261225d
[ 35.955744] RDX: 0000000000000050 RSI: 00007fee424e5c20 RDI: 000000000000000a
[ 35.955745] RBP: 00007fee424e5bf0 R08: 0000000000000003 R09: 00007fee424e5c20
[ 35.955747] R10: 00007fffc266f910 R11: 0000000000000202 R12: 00007fee424e66c0
[ 35.955748] R13: ffffffffffffff08 R14: 0000000000000016 R15: 00007fffc266f650
[ 35.955766] </TASK>In order to avoid such panic, reject bpf_timer in verifier when PREEMPT_RT is enabled.
--- kernel/bpf/verifier.c | 4 ++++ 1 file changed, 4 insertions(+)
kernel/bpf/verifier.c +4 -0
@@ -8552,6 +8552,10 @@ static int process_timer_func(struct bpf_verifier_env *env, int regno,verifier_bug (env,"Two map pointers in a timer helper" );return -EFAULT ; } +if (IS_ENABLED (CONFIG_PREEMPT_RT )) { +verbose (env,"bpf_timer cannot be used for PREEMPT_RT.\n" ); +return -EOPNOTSUPP ; + } meta->map_uid = reg->map_uid; meta->map_ptr = map;return 0 ;
Signature · 1 line
2.51.0
Eduard Zingerman
maintainerQuoted 25 lines · Leon Hwang
When enable
CONFIG_PREEMPT_RT, the kernel will panic when run timer selftests by './test_progs -t timer':[ 35.955287] BUG: sleeping function called from invalid context at kernel/locking/spinlock_rt.c:48 [ 35.955312] in_atomic(): 1, irqs_disabled(): 1, non_block: 0, pid: 120, name: test_progs [ 35.955315] preempt_count: 1, expected: 0 [ 35.955316] RCU nest depth: 0, expected: 0 [ 35.955317] 2 locks held by test_progs/120: [ 35.955319] #0: ffffffff8f1c3720 (rcu_read_lock_trace){....}-{0:0}, at: bpf_prog_test_run_syscall+0xc9/0x240 [ 35.955358] #1: ffff9155fbd331c8 ((&c->lock)){+.+.}-{3:3}, at: ___slab_alloc+0xb0/0xd20 [ 35.955388] irq event stamp: 100 [ 35.955389] hardirqs last enabled at (99): [<ffffffff8dfcd890>] do_syscall_64+0x30/0x2d0 [ 35.955414] hardirqs last disabled at (100): [<ffffffff8d4a9baa>] __bpf_async_init+0xca/0x310 [ 35.955428] softirqs last enabled at (0): [<ffffffff8d296cbb>] copy_process+0x9db/0x2000 [ 35.955449] softirqs last disabled at (0): [<0000000000000000>] 0x0 [ 35.955482] CPU: 1 UID: 0 PID: 120 Comm: test_progs Tainted: G OE 6.17.0-rc1-gc5f5af560d8a #30 PREEMPT_{RT,(full)} [ 35.955487] Tainted: [O]=OOT_MODULE, [E]=UNSIGNED_MODULE [ 35.955488] Hardware name: QEMU Ubuntu 24.04 PC (i440FX + PIIX, 1996), BIOS 1.16.3-debian-1.16.3-2 04/01/2014 [ 35.955491] Call Trace: [ 35.955493] <TASK> [ 35.955499] dump_stack_lvl+0x73/0xb0 [ 35.955514] dump_stack+0x14/0x20 [ 35.955518] __might_resched+0x167/0x230 [ 35.955537] rt_spin_lock+0x66/0x180 [ 35.955543] ? ___slab_alloc+0xb0/0xd20
Quoted 8 lines
[ 35.955549] ? bpf_map_kmalloc_node+0x7c/0x200 [ 35.955560] ___slab_alloc+0xb0/0xd20 [ 35.955575] ? __lock_acquire+0x43d/0x2590 [ 35.955601] __kmalloc_node_noprof+0x10b/0x410 [ 35.955605] ? __kmalloc_node_noprof+0x10b/0x410 [ 35.955607] ? bpf_map_kmalloc_node+0x7c/0x200 [ 35.955616] bpf_map_kmalloc_node+0x7c/0x200 [ 35.955624] __bpf_async_init+0xf8/0x310
The error is reported because of the kmalloc call in the __bpf_async_init, right? Instead of disabling timers for PREEMPT_RT, would it be possible to switch implementation to use kernel/bpf/memalloc.c:bpf_mem_alloc() instead?
Quoted 19 lines
[ 35.955633] bpf_timer_init+0x37/0x40 [ 35.955637] bpf_prog_2287350dd5909839_start_cb+0x5d/0x91 [ 35.955642] bpf_prog_0d54653d8a74e954_start_timer+0x65/0x8a [ 35.955650] bpf_prog_test_run_syscall+0x111/0x240 [ 35.955660] __sys_bpf+0x81c/0x2ab0 [ 35.955665] ? __might_fault+0x47/0x90 [ 35.955700] __x64_sys_bpf+0x1e/0x30 [ 35.955703] x64_sys_call+0x171d/0x20d0 [ 35.955715] do_syscall_64+0x6a/0x2d0 [ 35.955722] entry_SYSCALL_64_after_hwframe+0x76/0x7e [ 35.955728] RIP: 0033:0x7fee4261225d [ 35.955734] Code: ff c3 66 2e 0f 1f 84 00 00 00 00 00 90 f3 0f 1e fa 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 8b bb 0d 00 f7 d8 64 89 01 48 [ 35.955736] RSP: 002b:00007fee424e5bd8 EFLAGS: 00000202 ORIG_RAX: 0000000000000141 [ 35.955742] RAX: ffffffffffffffda RBX: 00007fee424e6cdc RCX: 00007fee4261225d [ 35.955744] RDX: 0000000000000050 RSI: 00007fee424e5c20 RDI: 000000000000000a [ 35.955745] RBP: 00007fee424e5bf0 R08: 0000000000000003 R09: 00007fee424e5c20 [ 35.955747] R10: 00007fffc266f910 R11: 0000000000000202 R12: 00007fee424e66c0 [ 35.955748] R13: ffffffffffffff08 R14: 0000000000000016 R15: 00007fffc266f650 [ 35.955766] </TASK>
[...]
Eduard Zingerman
maintainerQuoted 3 lines · Eduard Zingerman
Quoted 2 lines · Leon Hwang
When enable
CONFIG_PREEMPT_RT, the kernel will panic when run timer selftests by './test_progs -t timer':
Related discussions:
- https://lore.kernel.org/bpf/b634rejnvxqu6knjqlijosxrcnxbbpagt4de4pl6env6dwldz2@hoofqufparh5/T/
- https://lore.kernel.org/bpf/lhmdi6npaxqeuaumjhmq24ckpul7ufopwzxjbsezhepguqkxag@wolz4r2fazu2/T/
CC'ing Peilin.
Quoted 57 lines
Quoted 23 lines
[ 35.955287] BUG: sleeping function called from invalid context at kernel/locking/spinlock_rt.c:48 [ 35.955312] in_atomic(): 1, irqs_disabled(): 1, non_block: 0, pid: 120, name: test_progs [ 35.955315] preempt_count: 1, expected: 0 [ 35.955316] RCU nest depth: 0, expected: 0 [ 35.955317] 2 locks held by test_progs/120: [ 35.955319] #0: ffffffff8f1c3720 (rcu_read_lock_trace){....}-{0:0}, at: bpf_prog_test_run_syscall+0xc9/0x240 [ 35.955358] #1: ffff9155fbd331c8 ((&c->lock)){+.+.}-{3:3}, at: ___slab_alloc+0xb0/0xd20 [ 35.955388] irq event stamp: 100 [ 35.955389] hardirqs last enabled at (99): [<ffffffff8dfcd890>] do_syscall_64+0x30/0x2d0 [ 35.955414] hardirqs last disabled at (100): [<ffffffff8d4a9baa>] __bpf_async_init+0xca/0x310 [ 35.955428] softirqs last enabled at (0): [<ffffffff8d296cbb>] copy_process+0x9db/0x2000 [ 35.955449] softirqs last disabled at (0): [<0000000000000000>] 0x0 [ 35.955482] CPU: 1 UID: 0 PID: 120 Comm: test_progs Tainted: G OE 6.17.0-rc1-gc5f5af560d8a #30 PREEMPT_{RT,(full)} [ 35.955487] Tainted: [O]=OOT_MODULE, [E]=UNSIGNED_MODULE [ 35.955488] Hardware name: QEMU Ubuntu 24.04 PC (i440FX + PIIX, 1996), BIOS 1.16.3-debian-1.16.3-2 04/01/2014 [ 35.955491] Call Trace: [ 35.955493] <TASK> [ 35.955499] dump_stack_lvl+0x73/0xb0 [ 35.955514] dump_stack+0x14/0x20 [ 35.955518] __might_resched+0x167/0x230 [ 35.955537] rt_spin_lock+0x66/0x180 [ 35.955543] ? ___slab_alloc+0xb0/0xd20Quoted 8 lines
[ 35.955549] ? bpf_map_kmalloc_node+0x7c/0x200 [ 35.955560] ___slab_alloc+0xb0/0xd20 [ 35.955575] ? __lock_acquire+0x43d/0x2590 [ 35.955601] __kmalloc_node_noprof+0x10b/0x410 [ 35.955605] ? __kmalloc_node_noprof+0x10b/0x410 [ 35.955607] ? bpf_map_kmalloc_node+0x7c/0x200 [ 35.955616] bpf_map_kmalloc_node+0x7c/0x200 [ 35.955624] __bpf_async_init+0xf8/0x310The error is reported because of the kmalloc call in the
__bpf_async_init, right? Instead of disabling timers forPREEMPT_RT, would it be possible to switch implementation to use kernel/bpf/memalloc.c:bpf_mem_alloc() instead?Quoted 19 lines
[ 35.955633] bpf_timer_init+0x37/0x40 [ 35.955637] bpf_prog_2287350dd5909839_start_cb+0x5d/0x91 [ 35.955642] bpf_prog_0d54653d8a74e954_start_timer+0x65/0x8a [ 35.955650] bpf_prog_test_run_syscall+0x111/0x240 [ 35.955660] __sys_bpf+0x81c/0x2ab0 [ 35.955665] ? __might_fault+0x47/0x90 [ 35.955700] __x64_sys_bpf+0x1e/0x30 [ 35.955703] x64_sys_call+0x171d/0x20d0 [ 35.955715] do_syscall_64+0x6a/0x2d0 [ 35.955722] entry_SYSCALL_64_after_hwframe+0x76/0x7e [ 35.955728] RIP: 0033:0x7fee4261225d [ 35.955734] Code: ff c3 66 2e 0f 1f 84 00 00 00 00 00 90 f3 0f 1e fa 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 8b bb 0d 00 f7 d8 64 89 01 48 [ 35.955736] RSP: 002b:00007fee424e5bd8 EFLAGS: 00000202 ORIG_RAX: 0000000000000141 [ 35.955742] RAX: ffffffffffffffda RBX: 00007fee424e6cdc RCX: 00007fee4261225d [ 35.955744] RDX: 0000000000000050 RSI: 00007fee424e5c20 RDI: 000000000000000a [ 35.955745] RBP: 00007fee424e5bf0 R08: 0000000000000003 R09: 00007fee424e5c20 [ 35.955747] R10: 00007fffc266f910 R11: 0000000000000202 R12: 00007fee424e66c0 [ 35.955748] R13: ffffffffffffff08 R14: 0000000000000016 R15: 00007fffc266f650 [ 35.955766] </TASK>[...]
Hi all,
Quoted 1 line
Quoted 1 line
Quoted 1 line
[ 35.955287] BUG: sleeping function called from invalid context at kernel/locking/spinlock_rt.c:48
FWIW, I was able to reproduce this pr_err() after enabling CONFIG_PREEMPT_RT and CONFIG_DEBUG_ATOMIC_SLEEP.
Quoted 6 lines · Eduard Zingerman
Quoted 3 lines · Eduard Zingerman
Quoted 2 lines · Leon Hwang
When enable
CONFIG_PREEMPT_RT, the kernel will panic when run timer selftests by './test_progs -t timer':Related discussions:
[1]
Quoted 2 lines
- https://lore.kernel.org/bpf/b634rejnvxqu6knjqlijosxrcnxbbpagt4de4pl6env6dwldz2@hoofqufparh5/T/
- https://lore.kernel.org/bpf/lhmdi6npaxqeuaumjhmq24ckpul7ufopwzxjbsezhepguqkxag@wolz4r2fazu2/T/
[...]
Quoted 3 lines
Quoted 3 lines
The error is reported because of the kmalloc call in the
__bpf_async_init, right? Instead of disabling timers forPREEMPT_RT, would it be possible to switch implementation to use kernel/bpf/memalloc.c:bpf_mem_alloc() instead?
Just in case - actually there was a patch that does this:
[2] https://lore.kernel.org/bpf/[email protected]/
It was then superseded by the patches you linked [1] above however, since per discussion in [2], "use bpf_mem_alloc() to skip memcg accounting because it can trigger hardlockups" is a workaround instead of a proper fix.
I wonder if this new issue on PREEMPT_RT would justify [2] over [1]? IIUC, until kmalloc_nolock() becomes available:
[1] (plus Leon's patch here) means no bpf_timer on PREEMPT_RT, but we still have memcg accounting for non-PREEMPT_RT; [2] means no memcg accounting.
Thanks,
Peilin Ye
Quoted 35 lines · Peilin Ye
Hi all,
Quoted 1 line
Quoted 1 line
Quoted 1 line
[ 35.955287] BUG: sleeping function called from invalid context at kernel/locking/spinlock_rt.c:48FWIW, I was able to reproduce this
pr_err()after enablingCONFIG_PREEMPT_RTandCONFIG_DEBUG_ATOMIC_SLEEP.Quoted 6 lines · Eduard Zingerman
Quoted 3 lines · Eduard Zingerman
Quoted 2 lines · Leon Hwang
When enable
CONFIG_PREEMPT_RT, the kernel will panic when run timer selftests by './test_progs -t timer':Related discussions:
[1]
Quoted 2 lines
- https://lore.kernel.org/bpf/b634rejnvxqu6knjqlijosxrcnxbbpagt4de4pl6env6dwldz2@hoofqufparh5/T/
- https://lore.kernel.org/bpf/lhmdi6npaxqeuaumjhmq24ckpul7ufopwzxjbsezhepguqkxag@wolz4r2fazu2/T/[...]
Quoted 3 lines
Quoted 3 lines
The error is reported because of the kmalloc call in the
__bpf_async_init, right? Instead of disabling timers forPREEMPT_RT, would it be possible to switch implementation to use kernel/bpf/memalloc.c:bpf_mem_alloc() instead?Just in case - actually there was a patch that does this:
[2] https://lore.kernel.org/bpf/[email protected]/
It was then superseded by the patches you linked [1] above however, since per discussion in [2], "use
bpf_mem_alloc()to skip memcg accounting because it can trigger hardlockups" is a workaround instead of a proper fix.
Quoted 6 lines
I wonder if this new issue on
PREEMPT_RTwould justify [2] over [1]? IIUC, untilkmalloc_nolock()becomes available:[1] (plus Leon's patch here) means no
bpf_timeronPREEMPT_RT, but we still have memcg accounting for non-PREEMPT_RT; [2] means no memcg accounting.
I didn't comment on the above statement earlier, because
I thought you meant "no memcg accounting _inline_",
but reading above it sounds that you think that bpf_mem_alloc() doesn't do memcg accounting at all ?
That's incorrect. bpf_mem_alloc() always uses memcg accounting, but the usage is nuanced. bpf_global_ma is counted towards root memcg, since it's created during boot. While hash map powered by bpf_mem_alloc is using memcg of the user that created that map.
Quoted 17 lines · Alexei Starovoitov
Quoted 9 lines · Peilin Ye
Just in case - actually there was a patch that does this:
[2] https://lore.kernel.org/bpf/[email protected]/
It was then superseded by the patches you linked [1] above however, since per discussion in [2], "use
bpf_mem_alloc()to skip memcg accounting because it can trigger hardlockups" is a workaround instead of a proper fix.Quoted 6 lines
I wonder if this new issue on
PREEMPT_RTwould justify [2] over [1]? IIUC, untilkmalloc_nolock()becomes available:[1] (plus Leon's patch here) means no
bpf_timeronPREEMPT_RT, but we still have memcg accounting for non-PREEMPT_RT; [2] means no memcg accounting.
Quoted 5 lines
I didn't comment on the above statement earlier, because I thought you meant "no memcg accounting _inline_", but reading above it sounds that you think that
bpf_mem_alloc()doesn't do memcg accounting at all ?
That's incorrect.bpf_mem_alloc()always uses memcg accounting
Ah, I see - kernel/bpf/memalloc.c:alloc_bulk() via irq_work. Thanks for the correction!
Quoted 3 lines
, but the usage is nuanced.
bpf_global_mais counted towards root memcg, since it's created during boot. While hash map powered bybpf_mem_allocis using memcg of the user that created that map.
- - -
IIUC, this "sleeping function called from invalid context" message on PREEMPT_RT is because ___slab_alloc() does local_lock_irqsave(), with IRQ disabled by __bpf_async_init():
__bpf_spin_lock_irqsave(&async->lock);
t = async->timer;
if (t) {
ret = -EBUSY;
goto out;
}
/* allocate hrtimer via map_kmalloc to use memcg accounting */
cb = bpf_map_kmalloc_node(map, size, __GFP_HIGH, map->numa_node);For my understanding, is/how is kmalloc_nolock() going to resolve this? Patch [3] changes ___slab_alloc() to:
/* must check again c->slab in case we got preempted and it changed */
- local_lock_irqsave(&s->cpu_slab->lock, flags);
+ local_lock_cpu_slab(s, &flags);
But for PREEMPT_RT, local_lock_cpu_slab() still does local_lock_irqsave(), and the comment says that we can't call it with IRQ disabled:
+ * On PREEMPT_RT an invocation is not possible from IRQ-off or preempt + * disabled context. The lock will always be acquired and if needed it + * block and sleep until the lock is available.
So it seems that we'll still have this "sleeping function called from invalid context" issue for PREEMPT_RT even if we make __bpf_async_init() use bpf_mem_alloc() (when the latter uses kmalloc_nolock())?
[3]
[PATCH v3 5/6] slab: Introduce kmalloc_nolock() and kfree_nolock().
https://lore.kernel.org/all/[email protected]/
Thanks,
Peilin Ye
Quoted 54 lines · Peilin Ye
Quoted 17 lines · Alexei Starovoitov
Quoted 9 lines · Peilin Ye
Just in case - actually there was a patch that does this:
[2] https://lore.kernel.org/bpf/[email protected]/
It was then superseded by the patches you linked [1] above however, since per discussion in [2], "use
bpf_mem_alloc()to skip memcg accounting because it can trigger hardlockups" is a workaround instead of a proper fix.Quoted 6 lines
I wonder if this new issue on
PREEMPT_RTwould justify [2] over [1]? IIUC, untilkmalloc_nolock()becomes available:[1] (plus Leon's patch here) means no
bpf_timeronPREEMPT_RT, but we still have memcg accounting for non-PREEMPT_RT; [2] means no memcg accounting.Quoted 5 lines
I didn't comment on the above statement earlier, because I thought you meant "no memcg accounting _inline_", but reading above it sounds that you think that
bpf_mem_alloc()doesn't do memcg accounting at all ?
That's incorrect.bpf_mem_alloc()always uses memcg accountingAh, I see - kernel/bpf/memalloc.c:alloc_bulk() via
irq_work. Thanks for the correction!Quoted 3 lines
, but the usage is nuanced.
bpf_global_mais counted towards root memcg, since it's created during boot. While hash map powered bybpf_mem_allocis using memcg of the user that created that map.- - -
IIUC, this "sleeping function called from invalid context" message onPREEMPT_RTis because___slab_alloc()doeslocal_lock_irqsave(), with IRQ disabled by__bpf_async_init():__bpf_spin_lock_irqsave(&async->lock); t = async->timer; if (t) { ret = -EBUSY; goto out; } /* allocate hrtimer via map_kmalloc to use memcg accounting */ cb = bpf_map_kmalloc_node(map, size, __GFP_HIGH, map->numa_node);For my understanding, is/how is
kmalloc_nolock()going to resolve this? Patch [3] changes___slab_alloc()to:/* must check again c->slab in case we got preempted and it changed */-
local_lock_irqsave(&s->cpu_slab->lock, flags);
+local_lock_cpu_slab(s, &flags);
Quoted 7 lines
But for
PREEMPT_RT,local_lock_cpu_slab()still doeslocal_lock_irqsave(), and the comment says that we can't call it with IRQ disabled:+ * On PREEMPT_RT an invocation is not possible from IRQ-off or preempt + * disabled context. The lock will always be acquired and if needed it + * block and sleep until the lock is available.
Of course. Not saying that s/kmalloc/kmalloc_nolock/ is a silver bullet. bpf_timer needs other work. __bpf_spin_lock_irqsave() needs to go, etc.
Quoted 22 lines · Peilin Ye
Hi all,
Quoted 1 line
Quoted 1 line
Quoted 1 line
[ 35.955287] BUG: sleeping function called from invalid context at kernel/locking/spinlock_rt.c:48FWIW, I was able to reproduce this
pr_err()after enablingCONFIG_PREEMPT_RTandCONFIG_DEBUG_ATOMIC_SLEEP.Quoted 6 lines · Eduard Zingerman
Quoted 3 lines · Eduard Zingerman
Quoted 2 lines · Leon Hwang
When enable
CONFIG_PREEMPT_RT, the kernel will panic when run timer selftests by './test_progs -t timer':Related discussions:
[1]
Quoted 2 lines
- https://lore.kernel.org/bpf/b634rejnvxqu6knjqlijosxrcnxbbpagt4de4pl6env6dwldz2@hoofqufparh5/T/
- https://lore.kernel.org/bpf/lhmdi6npaxqeuaumjhmq24ckpul7ufopwzxjbsezhepguqkxag@wolz4r2fazu2/T/[...]
Quoted 7 lines
Quoted 3 lines
Quoted 3 lines
The error is reported because of the kmalloc call in the
__bpf_async_init, right? Instead of disabling timers forPREEMPT_RT, would it be possible to switch implementation to use kernel/bpf/memalloc.c:bpf_mem_alloc() instead?Just in case - actually there was a patch that does this:
Though switch to bpf_mem_alloc() kinda fixes it,
it's too late for this release and it's not a complete fix for RT, so I think it's better to disable it in the verifier like this patch does.
Leon, pls respin targeting bpf tree.
Also trim the commit log. It's too verbose and not quite correct. "kernel will panic"
That's true only if you have panic-on-warn set.
Just say that the kernel will warn.
pw-bot: cr
Quoted 29 lines · Alexei Starovoitov
Quoted 28 lines · Peilin Ye
Hi all,
Quoted 1 line
Quoted 1 line
Quoted 1 line
[ 35.955287] BUG: sleeping function called from invalid context at kernel/locking/spinlock_rt.c:48FWIW, I was able to reproduce this
pr_err()after enablingCONFIG_PREEMPT_RTandCONFIG_DEBUG_ATOMIC_SLEEP.Quoted 6 lines · Eduard Zingerman
Quoted 3 lines · Eduard Zingerman
Quoted 2 lines · Leon Hwang
When enable
CONFIG_PREEMPT_RT, the kernel will panic when run timer selftests by './test_progs -t timer':Related discussions:
[1]
Quoted 2 lines
- https://lore.kernel.org/bpf/b634rejnvxqu6knjqlijosxrcnxbbpagt4de4pl6env6dwldz2@hoofqufparh5/T/
- https://lore.kernel.org/bpf/lhmdi6npaxqeuaumjhmq24ckpul7ufopwzxjbsezhepguqkxag@wolz4r2fazu2/T/[...]
Quoted 3 lines
Quoted 3 lines
The error is reported because of the kmalloc call in the
__bpf_async_init, right? Instead of disabling timers forPREEMPT_RT, would it be possible to switch implementation to use kernel/bpf/memalloc.c:bpf_mem_alloc() instead?Just in case - actually there was a patch that does this:
Quoted 7 lines
Quoted 1 line
Though switch to
bpf_mem_alloc()kinda fixes it,
it's too late for this release and it's not a complete fix for RT, so I think it's better to disable it in the verifier like this patch does.Leon, pls respin targeting bpf tree.
My original intention for targeting the bpf-next tree was to ensure that the new 'timer_interrupt' selftest is skipped when PREEMPT_RT is enabled.
If I respin the patch for the bpf tree, I have to drop the part that skips the timer_interrupt test case. Should I?
Quoted 5 lines
Also trim the commit log. It's too verbose and not quite correct. "kernel will panic"
That's true only if you have panic-on-warn set.
Just say that the kernel will warn.
Sure.
I’ll make it more concise and clarify that "the kernel will warn", rather than saying it will always panic.
Thanks,
Leon
Quoted 4 lines · Leon Hwang
If I respin the patch for the bpf tree, I have to drop the part that skips the
timer_interrupttest case. Should I?
of course.
Quoted 8 lines · Alexei Starovoitov
Quoted 4 lines · Leon Hwang
If I respin the patch for the bpf tree, I have to drop the part that skips the
timer_interrupttest case. Should I?of course.
Leon,
the fix made it all the way to bpf-next.
Please follow up to silence timer_interrupt test.
It shouldn't affect CI, since we don't test RT.
Quoted 9 lines · Alexei Starovoitov
Quoted 7 lines · Alexei Starovoitov
Quoted 4 lines · Leon Hwang
If I respin the patch for the bpf tree, I have to drop the part that skips the
timer_interrupttest case. Should I?
Quoted 8 lines
Quoted 1 line
of course.
Leon,
the fix made it all the way to bpf-next.
Please follow up to silencetimer_interrupttest.It shouldn't affect CI, since we don't test RT.
No problem.
Once the fix lands in bpf-next, I’ll test it against RT.
After it’s applied, I’ll follow up with a revert of the patch set in order to silence the timer_interrupt test.
Thanks,
Leon
PATCH v2 2/2 selftests/bpf: Skip timer cases when bpf_timer is not supported
In reply to Leon HwangWhen enable CONFIG_PREEMPT_RT, verifier will reject bpf_timer with returning -EOPNOTSUPP.
Therefore, skip test cases when errno is EOPNOTSUPP.
cd tools/testing/selftests/bpf
./test_progs -t timer
125 free_timer:SKIP 459 timer:SKIP 460/1 timer_crash/array:SKIP 460/2 timer_crash/hash:SKIP 460 timer_crash:SKIP 461 timer_interrupt:SKIP 462 timer_lockup:SKIP 463 timer_mim:SKIP
Summary: 6/0 PASSED, 7 SKIPPED, 0 FAILED
--- tools/testing/selftests/bpf/prog_tests/free_timer.c | 4 ++++ tools/testing/selftests/bpf/prog_tests/timer.c | 8 ++++++++ tools/testing/selftests/bpf/prog_tests/timer_crash.c | 4 ++++ tools/testing/selftests/bpf/prog_tests/timer_lockup.c | 4 ++++ tools/testing/selftests/bpf/prog_tests/timer_mim.c | 4 ++++ 5 files changed, 24 insertions(+)
tools/testing/selftests/bpf/prog_tests/free_timer.c +4 -0
@@ -124,6 +124,10 @@ void test_free_timer(void)int err; skel =free_timer__open_and_load (); +if (!skel && errno ==EOPNOTSUPP ) { +test__skip (); +return ; + }if (!ASSERT_OK_PTR (skel,"open_load" ))return ;
tools/testing/selftests/bpf/prog_tests/timer.c +8 -0
@@ -87,6 +87,10 @@ void serial_test_timer(void)int err; timer_skel =timer__open_and_load (); +if (!timer_skel && errno ==EOPNOTSUPP ) { +test__skip (); +return ; + }if (!ASSERT_OK_PTR (timer_skel,"timer_skel_load" ))return ;
@@ -104,6 +108,10 @@ void test_timer_interrupt(void)LIBBPF_OPTS (bpf_test_run_opts, opts); skel =timer_interrupt__open_and_load (); +if (!skel && errno ==EOPNOTSUPP ) { +test__skip (); +return ; + }if (!ASSERT_OK_PTR (skel,"timer_interrupt__open_and_load" ))return ;
tools/testing/selftests/bpf/prog_tests/timer_crash.c +4 -0
@@ -12,6 +12,10 @@ static void test_timer_crash_mode(int mode)struct timer_crash *skel; skel =timer_crash__open_and_load (); +if (!skel && errno ==EOPNOTSUPP ) { +test__skip (); +return ; + }if (!ASSERT_OK_PTR (skel,"timer_crash__open_and_load" ))return ; skel->bss->pid =getpid ();
tools/testing/selftests/bpf/prog_tests/timer_lockup.c +4 -0
@@ -59,6 +59,10 @@ void test_timer_lockup(void) } skel =timer_lockup__open_and_load (); +if (!skel && errno ==EOPNOTSUPP ) { +test__skip (); +return ; + }if (!ASSERT_OK_PTR (skel,"timer_lockup__open_and_load" ))return ;
tools/testing/selftests/bpf/prog_tests/timer_mim.c +4 -0
@@ -65,6 +65,10 @@ void serial_test_timer_mim(void) goto cleanup; timer_skel =timer_mim__open_and_load (); +if (!timer_skel && errno ==EOPNOTSUPP ) { +test__skip (); +return ; + }if (!ASSERT_OK_PTR (timer_skel,"timer_skel_load" )) goto cleanup;
Signature · 1 line
2.51.0