4 messages, 2 authors, 2021-11-30 · open the first message on its own page
From: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Date: 2021-11-27 16:34:30
While browsing through code I noticed outdated code/ documentation regarding migrate_disable() on non-PREEMPT_RT kernels. Sebastian
From: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Date: 2021-11-27 16:34:27
The initial implementation of migrate_disable() for mainline was a
wrapper around preempt_disable(). RT kernels substituted this with
a real migrate disable implementation.
Later on mainline gained true migrate disable support, but the
documentation was not updated.
Update the documentation, remove the claims about migrate_disable()
mapping to preempt_disable() on non-PREEMPT_RT kernels.
Fixes: 74d862b682f51 ("sched: Make migrate_disable/enable() independent of RT")
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
Documentation/locking/locktypes.rst | 9 +++------
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/Documentation/locking/locktypes.rst b/Documentation/locking/locktypes.rst
index ddada4a537493..4fd7b70fcde19 100644
--- a/Documentation/locking/locktypes.rst
+++ b/Documentation/locking/locktypes.rst@@ -439,11 +439,9 @@ not allow to acquire p->lock because get_cpu_ptr() implicitly disables spin_lock(&p->lock); p->count += this_cpu_read(var2); -On a non-PREEMPT_RT kernel migrate_disable() maps to preempt_disable() -which makes the above code fully equivalent. On a PREEMPT_RT kernel migrate_disable() ensures that the task is pinned on the current CPU which in turn guarantees that the per-CPU access to var1 and var2 are staying on -the same CPU. +the same CPU while the task remains preemptible. The migrate_disable() substitution is not valid for the following scenario::
@@ -456,9 +454,8 @@ The migrate_disable() substitution is not valid for the following p = this_cpu_ptr(&var1); p->val = func2(); -While correct on a non-PREEMPT_RT kernel, this breaks on PREEMPT_RT because -here migrate_disable() does not protect against reentrancy from a -preempting task. A correct substitution for this case is:: +This breaks because migrate_disable() does not protect against reentrancy from +a preempting task. A correct substitution for this case is:: func() {
--
2.34.0
From: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Date: 2021-11-27 16:34:27
The initial implementation of migrate_disable() for mainline was a
wrapper around preempt_disable(). RT kernels substituted this with
a real migrate disable implementation.
Later on mainline gained true migrate disable support, but neither
documentation nor affected code were updated.
Remove stale comments claiming that migrate_disable() is PREEMPT_RT
only.
Don't use __this_cpu_inc() in the !PREEMPT_RT path because preemption is
not disabled and the RMW operation can be preempted.
Fixes: 74d862b682f51 ("sched: Make migrate_disable/enable() independent of RT")
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
include/linux/bpf.h | 16 ++--------------
include/linux/filter.h | 3 ---
2 files changed, 2 insertions(+), 17 deletions(-)
diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index e7a163a3146b6..327a2bec06ca0 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h@@ -1352,28 +1352,16 @@ extern struct mutex bpf_stats_enabled_mutex; * kprobes, tracepoints) to prevent deadlocks on map operations as any of * these events can happen inside a region which holds a map bucket lock * and can deadlock on it. - * - * Use the preemption safe inc/dec variants on RT because migrate disable - * is preemptible on RT and preemption in the middle of the RMW operation - * might lead to inconsistent state. Use the raw variants for non RT - * kernels as migrate_disable() maps to preempt_disable() so the slightly - * more expensive save operation can be avoided. */ static inline void bpf_disable_instrumentation(void) { migrate_disable(); - if (IS_ENABLED(CONFIG_PREEMPT_RT)) - this_cpu_inc(bpf_prog_active); - else - __this_cpu_inc(bpf_prog_active); + this_cpu_inc(bpf_prog_active); } static inline void bpf_enable_instrumentation(void) { - if (IS_ENABLED(CONFIG_PREEMPT_RT)) - this_cpu_dec(bpf_prog_active); - else - __this_cpu_dec(bpf_prog_active); + this_cpu_dec(bpf_prog_active); migrate_enable(); }
diff --git a/include/linux/filter.h b/include/linux/filter.h
index 24b7ed2677afd..534f678ca50fa 100644
--- a/include/linux/filter.h
+++ b/include/linux/filter.h@@ -640,9 +640,6 @@ static __always_inline u32 bpf_prog_run(const struct bpf_prog *prog, const void * This uses migrate_disable/enable() explicitly to document that the * invocation of a BPF program does not require reentrancy protection * against a BPF program which is invoked from a preempting task. - * - * For non RT enabled kernels migrate_disable/enable() maps to - * preempt_disable/enable(), i.e. it disables also preemption. */ static inline u32 bpf_prog_run_pin_on_cpu(const struct bpf_prog *prog, const void *ctx)
--
2.34.0
From: patchwork-bot+netdevbpf@kernel.org
Date: 2021-11-30 14:52:16
Hello: This series was applied to bpf/bpf.git (master) by Daniel Borkmann [off-list ref]: On Sat, 27 Nov 2021 17:31:58 +0100 you wrote:
While browsing through code I noticed outdated code/ documentation regarding migrate_disable() on non-PREEMPT_RT kernels. Sebastian
Here is the summary with links:
- [doc,1/2] Documentation/locking/locktypes: Update migrate_disable() bits.
https://git.kernel.org/bpf/bpf/c/6a631c0432dc
- [net,2/2] bpf: Make sure bpf_disable_instrumentation() is safe vs preemption.
https://git.kernel.org/bpf/bpf/c/79364031c5b4
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html