Re: [PATCH] sched: RCU-protect __set_task_cpu() in set_task_cpu()

[Oleg Nesterov]

On 06/06, Peter Zijlstra wrote:
>
> You're right, p->pi_lock for wakeups, rq->lock for runnable tasks.

Good, thanks.

Help! I have another question.

	try_to_wake_up:

		raw_spin_lock_irqsave(&p->pi_lock, flags);
		if (!(p->state & state))
			goto out;

		cpu = task_cpu(p);

		if (p->on_rq && ttwu_remote(p, wake_flags))
			goto stat;

This doesn't look a bit confusing, we can't trust "cpu = task_cpu" before
we check ->on_rq. OK, not a problem, this cpu number can only be used in
ttwu_stat(cpu).

But ttwu_stat(cpu) in turn does

	if (cpu != task_cpu(p))
		schedstat_inc(p, se.statistics.nr_wakeups_migrate);

Ignoring the theoretical races with pull_task/etc, how it is possible
that cpu != task_cpu(p) ? Another caller is try_to_wake_up_local(), it
obviously can't trigger this case.

This looks broken to me. Looking at its name, I guess nr_wakeups_migrate
should be incremented if ttwu does set_task_cpu(), correct?

IOW. Don't we need something like the (untested/ucompiled) patch below?
_If_ I am right, I can resend it with the changelog/etc but please feel
free to make another fix.

Oleg.

--- x/kernel/sched.c
+++ x/kernel/sched.c
@@ -2423,13 +2423,14 @@ static void update_avg(u64 *avg, u64 sam
 #endif
 
 static void
-ttwu_stat(struct task_struct *p, int cpu, int wake_flags)
+ttwu_stat(struct task_struct *p, bool migrate, int wake_flags)
 {
 #ifdef CONFIG_SCHEDSTATS
 	struct rq *rq = this_rq();
 
 #ifdef CONFIG_SMP
 	int this_cpu = smp_processor_id();
+	int cpu = task_cpu(p);
 
 	if (cpu == this_cpu) {
 		schedstat_inc(rq, ttwu_local);
@@ -2455,7 +2456,7 @@ ttwu_stat(struct task_struct *p, int cpu
 	if (wake_flags & WF_SYNC)
 		schedstat_inc(p, se.statistics.nr_wakeups_sync);
 
-	if (cpu != task_cpu(p))
+	if (migrate)
 		schedstat_inc(p, se.statistics.nr_wakeups_migrate);
 
 #endif /* CONFIG_SCHEDSTATS */
@@ -2630,6 +2631,7 @@ try_to_wake_up(struct task_struct *p, un
 {
 	unsigned long flags;
 	int cpu, success = 0;
+	bool migrate = false;
 
 	smp_wmb();
 	raw_spin_lock_irqsave(&p->pi_lock, flags);
@@ -2637,7 +2639,6 @@ try_to_wake_up(struct task_struct *p, un
 		goto out;
 
 	success = 1; /* we're going to change ->state */
-	cpu = task_cpu(p);
 
 	if (p->on_rq && ttwu_remote(p, wake_flags))
 		goto stat;
@@ -2674,13 +2675,15 @@ try_to_wake_up(struct task_struct *p, un
 		p->sched_class->task_waking(p);
 
 	cpu = select_task_rq(p, SD_BALANCE_WAKE, wake_flags);
-	if (task_cpu(p) != cpu)
+	if (task_cpu(p) != cpu) {
 		set_task_cpu(p, cpu);
+		migrate = true;
+	}
 #endif /* CONFIG_SMP */
 
 	ttwu_queue(p, cpu);
 stat:
-	ttwu_stat(p, cpu, wake_flags);
+	ttwu_stat(p, migrate, wake_flags);
 out:
 	raw_spin_unlock_irqrestore(&p->pi_lock, flags);
 
@@ -2716,7 +2719,7 @@ static void try_to_wake_up_local(struct 
 		ttwu_activate(rq, p, ENQUEUE_WAKEUP);
 
 	ttwu_do_wakeup(rq, p, 0);
-	ttwu_stat(p, smp_processor_id(), 0);
+	ttwu_stat(p, false, 0);
 out:
 	raw_spin_unlock(&p->pi_lock);
 }


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/