sched: fix the PREEMPT_ACTIVE check in __trace_sched_switch_state()

task_preempt_count() has nothing to do with the actual preempt counter, thread_info->saved_preempt_count is only valid right after switch_to().

__trace_sched_switch_state() can use preempt_count(), prev is still the current task when trace_sched_switch() is called.

Signed-off-by Oleg Nesterov <[email protected]>

---
 include/trace/events/sched.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

include/trace/events/sched.h +1 -1

index 0a68d5a..a7d67bc 100644
@@ -100,7 +100,7 @@ static inline long __trace_sched_switch_state(struct task_struct *p)
 	/*
 	 * For all intents and purposes a preempted task is a running task.
 	 */
-	if (task_preempt_count(p) & PREEMPT_ACTIVE)
+	if (preempt_count() & PREEMPT_ACTIVE)
 		state = TASK_RUNNING | TASK_STATE_MAX;
 #endif
 

Signature · 1 line

1.5.5.1

Quoted 5 lines · Oleg Nesterov

task_preempt_count() has nothing to do with the actual preempt counter, thread_info->saved_preempt_count is only valid right after switch_to().

__trace_sched_switch_state() can use preempt_count(), prev is still the current task when trace_sched_switch() is called.

This is true, but the paranoid in me would like a BUG_ON(p != current) right along with that to make sure we don't accidentally change this.

But yes, its good to get rid of task_preempt_count(), its horrible.

And note that another caller of task_preempt_count(), set_cpu(), is fine but it doesn't really need this helper.

And afaics we do not need ->saved_preempt_count at all, the trivial patch below makes it unnecessary, we can kill it and all its users.

Not only this will simplify the code, this will make (well, almost) the per-cpu preempt counter arch-agnostic.

Or I missed something?

Do you think this makes sense? If yes, I'll try to make the patches.

Quoted 6 lines · Oleg Nesterov

And note that another caller of task_preempt_count(), set_cpu(), is fine but it doesn't really need this helper.

And afaics we do not need ->saved_preempt_count at all, the trivial patch below makes it unnecessary, we can kill it and all its users.

Quoted 4 lines

Not only this will simplify the code, this will make (well, almost) the per-cpu preempt counter arch-agnostic.

Or I missed something?

Two things, per-cpu isn't always faster on some archs, and load-store archs have problems with PREEMPT_NEED_RESCHED, although arguably you can do per-cpu preempt count without that.

Quoted 1 line

Do you think this makes sense? If yes, I'll try to make the patches.

It penalizes everything but x86 I think.

Another problem is that finish_task_switch() itself runs with preempt enabled after finish_lock_switch(). If nothing else this means that ->sched_in() notifier can't trust its "cpu" arg.

Quoted 4 lines

Another problem is that finish_task_switch() itself runs with preempt enabled after finish_lock_switch(). If nothing else this means that ->sched_in() notifier can't trust its "cpu" arg.

OOPS, this obviously can't happen, ->preempt_notifiers must be empty. Remove this part from the changelog, please see v2.

I am not sure about finish_arch_post_lock_switch() ... but probably it should be fine without preempt_disable.

-------------------------------------------------------------------------------
Subject: [PATCH v2 1/2] sched: schedule_tail() should disable preemption

finish_task_switch() enables preemption, so post_schedule(rq) can be called on the wrong (and even dead) CPU. Afaics, nothing really bad can happen, but in this case we can wrongly clear rq->post_schedule on that CPU. And this simply looks wrong in any case.

Signed-off-by Oleg Nesterov <[email protected]>

---
 kernel/sched/core.c |   11 +++++------
 1 files changed, 5 insertions(+), 6 deletions(-)

kernel/sched/core.c +0 -0

index 703c7e6..3f267e8 100644

kernel/sched/core.c +2 -5

+	rq = this_rq();
 	finish_task_switch(rq, prev);
-
-	/*
-	 * FIXME: do we need to worry about rq being invalidated by the
-	 * task_switch?
-	 */
 	post_schedule(rq);
+	preempt_enable();
 
 	if (current->set_child_tid)
 		put_user(task_pid_vnr(current), current->set_child_tid);

В Ср, 08/10/2014 в 21:36 +0200, Oleg Nesterov пишет:

Quoted 7 lines

kernel/sched/core.c +1 -1

@@ -2277,15 +2277,14 @@ static inline void post_schedule(struct rq *rq)
 asmlinkage __visible void schedule_tail(struct task_struct *prev)
 	__releases(rq->lock)
 {
-	struct rq *rq = this_rq();
+	struct rq *rq;
 

kernel/sched/core.c static inline void post_schedule(struct rq *rq) · line 2283

Quoted 2 lines

kernel/sched/core.c +2 -0

+	/* finish_task_switch() drops rq->lock and enables preemtion */
+	preempt_disable();

Maybe, the code would look simpler if we change init_task_preempt_count() and create new tasks with preempt_count() == 2, so this preempt_disable() won't be necessary. But it's more or less subjectively.

Reviewed-by Kirill Tkhai <[email protected]>

but first we need to remove ->saved_preempt_count.

Quoted 1 line · Oleg Nesterov

but first we need to remove ->saved_preempt_count.

Why do you want to kill that?

Your earlier proposal would penalize every
!x86 arch by adding extra code to the scheduler core while they already automagically preserve their thread_info::preempt_count.

Quoted 3 lines

Your earlier proposal would penalize every
!x86 arch by adding extra code to the scheduler core while they already automagically preserve their thread_info::preempt_count.

Sure, and it can't be even compiled on !x86.

But this is simple, just we need a new helper, preempt_count_restore(), defined as nop in asm-generic/preempt.h. Well, perhaps another helper makes sense, preempt_count_raw() which simply reads the counter, but this is minor.

After the patch below we can remove ->saved_preempt_count. Including init_task_preempt_count(), it is no longer needed after the change in schedule_tail().

kernel/sched/core.c +1 -1

 	/* Here we just switch the register state and the stack. */
 	switch_to(prev, next, prev);
-
 	barrier();
+	preempt_count_restore(pc);
 	/*
 	 * this_rq must be evaluated again because prev may have moved
 	 * CPUs since it called schedule(), thus the 'rq' on its stack

Quoted 6 lines · Oleg Nesterov

Quoted 3 lines

Your earlier proposal would penalize every
!x86 arch by adding extra code to the scheduler core while they already automagically preserve their thread_info::preempt_count.

Sure, and it can't be even compiled on !x86.

Quoted 8 lines

But this is simple, just we need a new helper, preempt_count_restore(), defined as nop in asm-generic/preempt.h. Well, perhaps another helper makes sense, preempt_count_raw() which simply reads the counter, but this is minor.

After the patch below we can remove ->saved_preempt_count. Including init_task_preempt_count(), it is no longer needed after the change in schedule_tail().

Ah, right, this makes more sense.

Quoted 3 lines

(quoted diff) +0 -0

@@ -2333,10 +2336,12 @@ context_switch(struct rq *rq, struct task_struct *prev,
 #endif
 

kernel/sched/core.c context_switch(struct rq *rq, struct task_struct *prev, · line 2340

Quoted 3 lines

kernel/sched/core.c +2 -0

 	context_tracking_task_switch(prev, next);
+
+	pc = preempt_count();

The only problem here is that you can loose PREEMPT_NEED_RESCHED, I haven't thought about whether that is a problem here or not.

Quoted 4 lines · Peter Zijlstra

Quoted 3 lines · Oleg Nesterov

(quoted diff) +0 -0

@@ -2333,10 +2336,12 @@ context_switch(struct rq *rq, struct task_struct *prev,
 #endif
 

Quoted 6 lines

kernel/sched/core.c context_switch(struct rq *rq, struct task_struct *prev, · line 2340

Quoted 3 lines

kernel/sched/core.c +2 -0

 	context_tracking_task_switch(prev, next);
+
+	pc = preempt_count();

The only problem here is that you can loose PREEMPT_NEED_RESCHED, I haven't thought about whether that is a problem here or not.

Also, if you make that preempt_count_save(), to mirror the restore,

can both preserve PREEMPT_NEED_RESCHED and avoid emitting code on !x86.

kernel/sched/core.c +0 -1

@@ -2802,15 +2802,8 @@ need_resched:
 		rq->curr = next;
 		++*switch_count;
 
-		context_switch(rq, prev, next); /* unlocks the rq */

Quoted 4 lines · Oleg Nesterov

kernel/sched/core.c +0 -0

@@ -2802,15 +2802,8 @@ need_resched:
 		rq->curr = next;
 		++*switch_count;
 

kernel/sched/core.c need_resched: · line 2806

Quoted 11 lines

kernel/sched/core.c +2 -9

-		context_switch(rq, prev, next); /* unlocks the rq */
-		/*
-		 * The context switch have flipped the stack from under us
-		 * and restored the local variables which were saved when
-		 * this task called schedule() in the past. prev == current
-		 * is still correct, but it can be moved to another cpu/rq.
-		 */
-		cpu = smp_processor_id();
-		rq = cpu_rq(cpu);
+		rq = context_switch(rq, prev, next); /* unlocks the rq */
+		cpu = rq->cpu;

This won't compile on UP, cpu_of(rq) works though.

Quoted 6 lines · Peter Zijlstra

Quoted 4 lines · Oleg Nesterov

kernel/sched/core.c +0 -0

@@ -2802,15 +2802,8 @@ need_resched:
 		rq->curr = next;
 		++*switch_count;
 

Quoted 13 lines

kernel/sched/core.c need_resched: · line 2806

Quoted 11 lines

kernel/sched/core.c +2 -9

-		context_switch(rq, prev, next); /* unlocks the rq */
-		/*
-		 * The context switch have flipped the stack from under us
-		 * and restored the local variables which were saved when
-		 * this task called schedule() in the past. prev == current
-		 * is still correct, but it can be moved to another cpu/rq.
-		 */
-		cpu = smp_processor_id();
-		rq = cpu_rq(cpu);
+		rq = context_switch(rq, prev, next); /* unlocks the rq */
+		cpu = rq->cpu;

This won't compile on UP, cpu_of(rq) works though.

Heh ;) and I swear I was going to write cpu_of().

and I agree with BUG_ON() in __trace_sched_switch() if CONFIG_SCHED_DEBUG.

I am travelling till next Wednesday, will resend everything once I return.

Quoted 2 lines · Oleg Nesterov

I am travelling till next Wednesday, will resend everything once I return.

Not towards DUS by any chance?

I fixed up the patches, they should be in
here:
https://git.kernel.org/cgit/linux/kernel/git/peterz/queue.git/log/?h=sched/core