by Oleg Nesterov

[permalink] [raw]

Subject: Re: [PATCH 0/1] stop the unbound recursion in preempt_schedule_context()

And, Frederic, this is off-topic but I am just curious...

I can't understand why exception_enter() calls context_tracking_user_exit()
unconditionally (unlike exception_exit), and why enter/exit need to check
context_tracking.state with irqs disabled.

IOW, any reason why the patch below is wrong?

Of course, context_tracking is per-cpu, so

if (context_tracking_in_user()) {
local_irq_save(flags);
...

can be preempted and change the state on another CPU. But this should
be fine because the task must see the same .state on all CPU's or the
whole preempt context-tracking logic is broken?

Oleg.

--- x/include/linux/context_tracking.h
+++ x/include/linux/context_tracking.h
@@ -29,15 +29,14 @@ static inline void user_exit(void)

static inline enum ctx_state exception_enter(void)
{
- enum ctx_state prev_ctx;
-
- if (!context_tracking_is_enabled())
- return 0;
-
- prev_ctx = this_cpu_read(context_tracking.state);
- context_tracking_user_exit();
+ if (context_tracking_is_enabled()) {
+ if (context_tracking_in_user()) {
+ context_tracking_user_exit();
+ return IN_USER;
+ }
+ }

- return prev_ctx;
+ return IN_KERNEL;
}

static inline void exception_exit(enum ctx_state prev_ctx)
--- x/kernel/context_tracking.c
+++ x/kernel/context_tracking.c
@@ -74,8 +74,8 @@ void context_tracking_user_enter(void)
/* Kernel threads aren't supposed to go to userspace */
WARN_ON_ONCE(!current->mm);

- local_irq_save(flags);
- if ( __this_cpu_read(context_tracking.state) != IN_USER) {
+ if (!context_tracking_in_user()) {
+ local_irq_save(flags);
if (__this_cpu_read(context_tracking.active)) {
trace_user_enter(0);
/*
@@ -102,8 +102,8 @@ void context_tracking_user_enter(void)
* is false because we know that CPU is not tickless.
*/
__this_cpu_write(context_tracking.state, IN_USER);
+ local_irq_restore(flags);
}
- local_irq_restore(flags);
}
NOKPROBE_SYMBOL(context_tracking_user_enter);

@@ -128,8 +128,8 @@ void context_tracking_user_exit(void)
if (in_interrupt())
return;

- local_irq_save(flags);
- if (__this_cpu_read(context_tracking.state) == IN_USER) {
+ if (context_tracking_in_user()) {
+ local_irq_save(flags);
if (__this_cpu_read(context_tracking.active)) {
/*
* We are going to run code that may use RCU. Inform
@@ -140,8 +140,8 @@ void context_tracking_user_exit(void)
trace_user_exit(0);
}
__this_cpu_write(context_tracking.state, IN_KERNEL);
+ local_irq_restore(flags);
}
- local_irq_restore(flags);
}
NOKPROBE_SYMBOL(context_tracking_user_exit);

2014-10-28 11:04:33

by tip-bot for Vasyl Gomonovych

[permalink] [raw]

Subject: [tip:sched/core] sched: stop the unbound recursion in preempt_schedule_context()

Commit-ID: 009f60e2763568cdcd75bd1cf360c7c7165e2e60
Gitweb: http://git.kernel.org/tip/009f60e2763568cdcd75bd1cf360c7c7165e2e60
Author: Oleg Nesterov <[email protected]>
AuthorDate: Sun, 5 Oct 2014 22:23:22 +0200
Committer: Ingo Molnar <[email protected]>
CommitDate: Tue, 28 Oct 2014 10:46:05 +0100

sched: stop the unbound recursion in preempt_schedule_context()

preempt_schedule_context() does preempt_enable_notrace() at the end
and this can call the same function again; exception_exit() is heavy
and it is quite possible that need-resched is true again.

1. Change this code to dec preempt_count() and check need_resched()
by hand.

2. As Linus suggested, we can use the PREEMPT_ACTIVE bit and avoid
the enable/disable dance around __schedule(). But in this case
we need to move into sched/core.c.

3. Cosmetic, but x86 forgets to declare this function. This doesn't
really matter because it is only called by asm helpers, still it
make sense to add the declaration into asm/preempt.h to match
preempt_schedule().

Reported-by: Sasha Levin <[email protected]>
Signed-off-by: Oleg Nesterov <[email protected]>
Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
Cc: Alexander Graf <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: Christoph Lameter <[email protected]>
Cc: Linus Torvalds <[email protected]>
Cc: Masami Hiramatsu <[email protected]>
Cc: Steven Rostedt <[email protected]>
Cc: Peter Anvin <[email protected]>
Cc: Andy Lutomirski <[email protected]>
Cc: Denys Vlasenko <[email protected]>
Cc: Chuck Ebbert <[email protected]>
Cc: Frederic Weisbecker <[email protected]>
Link: http://lkml.kernel.org/r/[email protected]
Signed-off-by: Ingo Molnar <[email protected]>
---
arch/x86/include/asm/preempt.h | 1 +
kernel/context_tracking.c | 40 ----------------------------------------
kernel/sched/core.c | 41 +++++++++++++++++++++++++++++++++++++++++
3 files changed, 42 insertions(+), 40 deletions(-)

diff --git a/arch/x86/include/asm/preempt.h b/arch/x86/include/asm/preempt.h
index 7024c12..4008734 100644
--- a/arch/x86/include/asm/preempt.h
+++ b/arch/x86/include/asm/preempt.h
@@ -105,6 +105,7 @@ static __always_inline bool should_resched(void)
# ifdef CONFIG_CONTEXT_TRACKING
extern asmlinkage void ___preempt_schedule_context(void);
# define __preempt_schedule_context() asm ("call ___preempt_schedule_context")
+ extern asmlinkage void preempt_schedule_context(void);
# endif
#endif

diff --git a/kernel/context_tracking.c b/kernel/context_tracking.c
index 5664985..937ecdf 100644
--- a/kernel/context_tracking.c
+++ b/kernel/context_tracking.c
@@ -107,46 +107,6 @@ void context_tracking_user_enter(void)
}
NOKPROBE_SYMBOL(context_tracking_user_enter);

-#ifdef CONFIG_PREEMPT
-/**
- * preempt_schedule_context - preempt_schedule called by tracing
- *
- * The tracing infrastructure uses preempt_enable_notrace to prevent
- * recursion and tracing preempt enabling caused by the tracing
- * infrastructure itself. But as tracing can happen in areas coming
- * from userspace or just about to enter userspace, a preempt enable
- * can occur before user_exit() is called. This will cause the scheduler
- * to be called when the system is still in usermode.
- *
- * To prevent this, the preempt_enable_notrace will use this function
- * instead of preempt_schedule() to exit user context if needed before
- * calling the scheduler.
- */
-asmlinkage __visible void __sched notrace preempt_schedule_context(void)
-{
- enum ctx_state prev_ctx;
-
- if (likely(!preemptible()))
- return;
-
- /*
- * Need to disable preemption in case user_exit() is traced
- * and the tracer calls preempt_enable_notrace() causing
- * an infinite recursion.
- */
- preempt_disable_notrace();
- prev_ctx = exception_enter();
- preempt_enable_no_resched_notrace();
-
- preempt_schedule();
-
- preempt_disable_notrace();
- exception_exit(prev_ctx);
- preempt_enable_notrace();
-}
-EXPORT_SYMBOL_GPL(preempt_schedule_context);
-#endif /* CONFIG_PREEMPT */
-
/**
* context_tracking_user_exit - Inform the context tracking that the CPU is
* exiting userspace mode and entering the kernel.
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index dde8adb..240157c 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -2951,6 +2951,47 @@ asmlinkage __visible void __sched notrace preempt_schedule(void)
}
NOKPROBE_SYMBOL(preempt_schedule);
EXPORT_SYMBOL(preempt_schedule);
+
+#ifdef CONFIG_CONTEXT_TRACKING
+/**
+ * preempt_schedule_context - preempt_schedule called by tracing
+ *
+ * The tracing infrastructure uses preempt_enable_notrace to prevent
+ * recursion and tracing preempt enabling caused by the tracing
+ * infrastructure itself. But as tracing can happen in areas coming
+ * from userspace or just about to enter userspace, a preempt enable
+ * can occur before user_exit() is called. This will cause the scheduler
+ * to be called when the system is still in usermode.
+ *
+ * To prevent this, the preempt_enable_notrace will use this function
+ * instead of preempt_schedule() to exit user context if needed before
+ * calling the scheduler.
+ */
+asmlinkage __visible void __sched notrace preempt_schedule_context(void)
+{
+ enum ctx_state prev_ctx;
+
+ if (likely(!preemptible()))
+ return;
+
+ do {
+ __preempt_count_add(PREEMPT_ACTIVE);
+ /*
+ * Needs preempt disabled in case user_exit() is traced
+ * and the tracer calls preempt_enable_notrace() causing
+ * an infinite recursion.
+ */
+ prev_ctx = exception_enter();
+ __schedule();
+ exception_exit(prev_ctx);
+
+ __preempt_count_sub(PREEMPT_ACTIVE);
+ barrier();
+ } while (need_resched());
+}
+EXPORT_SYMBOL_GPL(preempt_schedule_context);
+#endif /* CONFIG_CONTEXT_TRACKING */
+
#endif /* CONFIG_PREEMPT */

/*