Add rq->nr_uninterruptible count to dest cpu's rq while CPU goes down.

When a CPU is about to go down, it moves all it's sleeping task to an active CPU, then nr_uninterruptible counts are also moved. When moving nr_uninterruptible count, currently it chooses a randomly picked CPU from the active CPU mask to keep the global nr_uninterruptible count intact. But, it would be precise to move nr_uninterruptible counts to the CPU where all the sleeping tasks were moved and it also might have subtle impact over rq's load calculation. So, this patch is prepared to address this issue.

Quoted 9 lines · Rakib Mullick

When a CPU is about to go down, it moves all it's sleeping task to an active CPU, then nr_uninterruptible counts are
also moved. When moving nr_uninterruptible count, currently it chooses a randomly picked CPU from the active CPU mask
to keep the global nr_uninterruptible count intact. But, it would be precise to move nr_uninterruptible counts to the
CPU where all the sleeping tasks were moved and it also might have subtle impact over rq's load calculation. So, this
patch is prepared to address this issue.

The Changelog is ill-formated. Other than that, the patch doesn't appear to actually do what it says. The sleeping tasks can be scattered to any number of cpus as decided by select_fallback_rq().

Furthermore there should be absolutely no impact on load calculation what so ever. nr_uninterruptible is only ever useful as a sum over all cpus, this total sum doesn't change regardless of where you put the value.

Worse, there's absolutely no relation to the tasks on the runqueue (sleeping or otherwise) and nr_uninterruptible, so coupling these actions makes no sense what so ever.

I'm not sure which parts are missing from Changelog to patch. And this patch assumes that, sleeping tasks won't be scattered. From select_fallback_rq(), sleeping tasks might get scattered due to various cases like. if CPU is down, task isn't allowed to move a particular CPU. Other than that, dest cpu supposed to be the same.

Quoted 9 lines

Furthermore there should be absolutely no impact on load calculation what so ever. nr_uninterruptible is only ever useful as a sum over all cpus, this total sum doesn't change regardless of where you put the value.

Worse, there's absolutely no relation to the tasks on the runqueue (sleeping or otherwise) and nr_uninterruptible, so coupling these actions makes no sense what so ever.

nr_uninterruptible is coupled with tasks on the runqueue to calculate nr_active numbers.

In calc_load_fold_active(), this nr_active numbers are used to calculate delta. This is how I understand this part and seeing some impact.

Quoted 2 lines · Rakib Mullick

nr_uninterruptible is coupled with tasks on the runqueue to calculate nr_active numbers.

It is not.. nr_uninterruptible is incremented on the cpu the task goes to sleep and decremented on the cpu doing the wakeup.

This means that nr_uninterruptible is a complete mess and any per-cpu value isn't meaningful at all.

It is quite possible to always have the inc on cpu0 and the decrement on cpu1, yielding results like:

{1000, -1000} for an effective nr_uninterruptible = 0. Taking either cpu down will then migrate whatever delta it has to another cpu, but there might only be a single task, yet the delta is +-1000.

Quoted 3 lines

In calc_load_fold_active(), this nr_active numbers are used to calculate delta. This is how I understand this part and seeing some impact.

You understand wrong, please re-read the comment added in commit 5167e8d5.

And also I think migrate_nr_uninterruptible() is meaning less too.

Quoted 1 line · Rakib Mullick

And also I think migrate_nr_uninterruptible() is meaning less too.

Hmm, I think I see a problem.. we forget to migrate the effective delta created by rq->calc_load_active.

Quoted 6 lines · Peter Zijlstra

Quoted 1 line · Rakib Mullick

And also I think migrate_nr_uninterruptible() is meaning less too.

Hmm, I think I see a problem.. we forget to migrate the effective delta created by rq->calc_load_active.

And rq->calc_load_active needs to be migrated to the proper dest_rq not like currently picking any random rq.

Quoted 9 lines · Rakib Mullick

Quoted 6 lines · Peter Zijlstra

Quoted 1 line · Rakib Mullick

And also I think migrate_nr_uninterruptible() is meaning less too.

Hmm, I think I see a problem.. we forget to migrate the effective delta created by rq->calc_load_active.

And rq->calc_load_active needs to be migrated to the proper dest_rq not like currently picking any random rq.

OK, so how about something like the below, it would also solve Paul's issue with that code.

Please do double check the logic, I've had all of 4 hours sleep and its far too warm for a brain to operate in any case.

---
Subject: sched: Fix load avg vs cpu-hotplug

Rabik and Paul reported two different issues related to the same few lines of code.

Rabik's issue is that the nr_uninterruptible migration code is wrong in that he sees artifacts due to this (Rabik please do expand in more detail).

Paul's issue is that this code as it stands relies on us using stop_machine() for unplug, we all would like to remove this assumption so that eventually we can remove this stop_machine() usage altogether.

The only reason we'd have to migrate nr_uninterruptible is so that we could use for_each_online_cpu() loops in favour of for_each_possible_cpu() loops, however since nr_uninterruptible() is the only such loop and its using possible lets not bother at all.

The problem Rabik sees is (probably) caused by the fact that by migrating nr_uninterruptible we screw rq->calc_load_active for both rqs involved.

So don't bother with fancy migration schemes (meaning we now have to keep using for_each_possible_cpu()) and instead fold any nr_active delta after we migrate all tasks away to make sure we don't have any skewed nr_active accounting.

Reported-by Rakib Mullick <[email protected]>

Reported-by Paul E. McKenney <[email protected]>

Signed-off-by Peter Zijlstra <[email protected]>

---
 kernel/sched/core.c | 31 ++++++++++---------------------
 1 file changed, 10 insertions(+), 21 deletions(-)

kernel/sched/core.c +0 -10

index 4376c9f..06d23c6 100644
@@ -5338,27 +5338,17 @@ void idle_task_exit(void)
 }
 
 /*
- * While a dead CPU has no uninterruptible tasks queued at this point,
- * it might still have a nonzero ->nr_uninterruptible counter, because
- * for performance reasons the counter is not stricly tracking tasks to
- * their home CPUs. So we just add the counter to another CPU's counter,
- * to keep the global sum constant after CPU-down:
- */
-static void migrate_nr_uninterruptible(struct rq *rq_src)
-{
-	struct rq *rq_dest = cpu_rq(cpumask_any(cpu_active_mask));
-

kernel/sched/core.c +6 -5

+ * Also see the comment "Global load-average calculations".
  */
-static void calc_global_load_remove(struct rq *rq)
+static void calc_load_migrate(struct rq *rq)
 {
-	atomic_long_sub(rq->calc_load_active, &calc_load_tasks);
-	rq->calc_load_active = 0;
+	long delta = calc_load_fold_active(rq);
+	if (delta)
+		atomic_long_add(delta, &calc_load_tasks);
 }
 
 /*

@@ -5652,8 +5642,7 @@ migration_call(struct notifier_block *nfb, unsigned
 long action, void *hcpu)
 		BUG_ON(rq->nr_running != 1); /* the migration thread */
 		raw_spin_unlock_irqrestore(&rq->lock, flags);
 
-		migrate_nr_uninterruptible(rq);
-		calc_global_load_remove(rq);
+		calc_load_migrate(rq);
 		break;
 #endif

}

Quoted 1 line

First of all, you've misspelled my name, it's Rakib not Rabik.

Quoted 22 lines

kernel/sched/core.c +0 -14

index 4376c9f..06d23c6 100644
@@ -5338,27 +5338,17 @@ void idle_task_exit(void)
 }
 
 /*
- * While a dead CPU has no uninterruptible tasks queued at this point,
- * it might still have a nonzero ->nr_uninterruptible counter, because
- * for performance reasons the counter is not stricly tracking tasks to
- * their home CPUs. So we just add the counter to another CPU's counter,
- * to keep the global sum constant after CPU-down:
- */
-static void migrate_nr_uninterruptible(struct rq *rq_src)
-{
-     struct rq *rq_dest = cpu_rq(cpumask_any(cpu_active_mask));
-
-     rq_dest->nr_uninterruptible += rq_src->nr_uninterruptible;
-     rq_src->nr_uninterruptible = 0;
-}
-

kernel/sched/core.c void idle_task_exit(void) · line 5344

Quoted 6 lines

kernel/sched/core.c +4 -2

-/*
- * remove the tasks which were accounted by rq from calc_load_tasks.
+ * Since this CPU is going 'away' for a while, fold any nr_active delta
+ * we might have. Assumes we're called after migrate_tasks() so that the
+ * nr_active count is stable.
+ *

But after migrate_tasks(), it's likely that rq->nr_running will be 1. Then, nr_active will be screwed. No?

Quoted 9 lines · Rakib Mullick

Quoted 6 lines · Peter Zijlstra

Quoted 1 line · Rakib Mullick

And also I think migrate_nr_uninterruptible() is meaning less too.

Hmm, I think I see a problem.. we forget to migrate the effective delta created by rq->calc_load_active.

And rq->calc_load_active needs to be migrated to the proper dest_rq not like currently picking any random rq.

OK, so how about something like the below, it would also solve Paul's issue with that code.

Please do double check the logic, I've had all of 4 hours sleep and its far too warm for a brain to operate in any case.

---
Subject: sched: Fix load avg vs cpu-hotplug

Rabik and Paul reported two different issues related to the same few lines of code.

Rabik's issue is that the nr_uninterruptible migration code is wrong in that he sees artifacts due to this (Rabik please do expand in more detail).

Paul's issue is that this code as it stands relies on us using stop_machine() for unplug, we all would like to remove this assumption so that eventually we can remove this stop_machine() usage altogether.

The only reason we'd have to migrate nr_uninterruptible is so that we could use for_each_online_cpu() loops in favour of for_each_possible_cpu() loops, however since nr_uninterruptible() is the only such loop and its using possible lets not bother at all.

The problem Rabik sees is (probably) caused by the fact that by migrating nr_uninterruptible we screw rq->calc_load_active for both rqs involved.

So don't bother with fancy migration schemes (meaning we now have to keep using for_each_possible_cpu()) and instead fold any nr_active delta after we migrate all tasks away to make sure we don't have any skewed nr_active accounting.

Reported-by Rakib Mullick <[email protected]>

Reported-by Paul E. McKenney <[email protected]>

Signed-off-by Peter Zijlstra <[email protected]>

---
 kernel/sched/core.c | 31 ++++++++++---------------------
 1 file changed, 10 insertions(+), 21 deletions(-)

kernel/sched/core.c +9 -19

index 4376c9f..06d23c6 100644
@@ -5338,27 +5338,17 @@ void idle_task_exit(void)
 }
 
 /*
- * While a dead CPU has no uninterruptible tasks queued at this point,
- * it might still have a nonzero ->nr_uninterruptible counter, because
- * for performance reasons the counter is not stricly tracking tasks to
- * their home CPUs. So we just add the counter to another CPU's counter,
- * to keep the global sum constant after CPU-down:
- */
-static void migrate_nr_uninterruptible(struct rq *rq_src)
-{
-	struct rq *rq_dest = cpu_rq(cpumask_any(cpu_active_mask));
-
-	rq_dest->nr_uninterruptible += rq_src->nr_uninterruptible;
-	rq_src->nr_uninterruptible = 0;
-}
-
-/*
- * remove the tasks which were accounted by rq from calc_load_tasks.
+ * Since this CPU is going 'away' for a while, fold any nr_active delta
+ * we might have. Assumes we're called after migrate_tasks() so that the
+ * nr_active count is stable.
+ *
+ * Also see the comment "Global load-average calculations".
  */
-static void calc_global_load_remove(struct rq *rq)
+static void calc_load_migrate(struct rq *rq)
 {
-	atomic_long_sub(rq->calc_load_active, &calc_load_tasks);
-	rq->calc_load_active = 0;
+	long delta = calc_load_fold_active(rq);
+	if (delta)
+		atomic_long_add(delta, &calc_load_tasks);
 }
 
 /*

kernel/sched/core.c +0 -0

 		break;
 #endif
 	}

On Mon, Aug 20, 2012 at 11:26:57AM +0200, Peter Zijlstra wrote:

Quoted 85 lines

OK, so how about something like the below, it would also solve Paul's issue with that code.

Please do double check the logic, I've had all of 4 hours sleep and its far too warm for a brain to operate in any case.

---
Subject: sched: Fix load avg vs cpu-hotplug

Rabik and Paul reported two different issues related to the same few lines of code.

Rabik's issue is that the nr_uninterruptible migration code is wrong in that he sees artifacts due to this (Rabik please do expand in more detail).

Paul's issue is that this code as it stands relies on us using stop_machine() for unplug, we all would like to remove this assumption so that eventually we can remove this stop_machine() usage altogether.

The only reason we'd have to migrate nr_uninterruptible is so that we could use for_each_online_cpu() loops in favour of for_each_possible_cpu() loops, however since nr_uninterruptible() is the only such loop and its using possible lets not bother at all.

The problem Rabik sees is (probably) caused by the fact that by migrating nr_uninterruptible we screw rq->calc_load_active for both rqs involved.

So don't bother with fancy migration schemes (meaning we now have to keep using for_each_possible_cpu()) and instead fold any nr_active delta after we migrate all tasks away to make sure we don't have any skewed nr_active accounting.

Reported-by Rakib Mullick <[email protected]>

Reported-by Paul E. McKenney <[email protected]>

Signed-off-by Peter Zijlstra <[email protected]>

---
 kernel/sched/core.c | 31 ++++++++++---------------------
 1 file changed, 10 insertions(+), 21 deletions(-)

kernel/sched/core.c +9 -19

index 4376c9f..06d23c6 100644
@@ -5338,27 +5338,17 @@ void idle_task_exit(void)
 }
 
 /*
- * While a dead CPU has no uninterruptible tasks queued at this point,
- * it might still have a nonzero ->nr_uninterruptible counter, because
- * for performance reasons the counter is not stricly tracking tasks to
- * their home CPUs. So we just add the counter to another CPU's counter,
- * to keep the global sum constant after CPU-down:
- */
-static void migrate_nr_uninterruptible(struct rq *rq_src)
-{
-	struct rq *rq_dest = cpu_rq(cpumask_any(cpu_active_mask));
-
-	rq_dest->nr_uninterruptible += rq_src->nr_uninterruptible;
-	rq_src->nr_uninterruptible = 0;
-}
-
-/*
- * remove the tasks which were accounted by rq from calc_load_tasks.
+ * Since this CPU is going 'away' for a while, fold any nr_active delta
+ * we might have. Assumes we're called after migrate_tasks() so that the
+ * nr_active count is stable.
+ *
+ * Also see the comment "Global load-average calculations".
  */
-static void calc_global_load_remove(struct rq *rq)
+static void calc_load_migrate(struct rq *rq)
 {
-	atomic_long_sub(rq->calc_load_active, &calc_load_tasks);
-	rq->calc_load_active = 0;
+	long delta = calc_load_fold_active(rq);
+	if (delta)
+		atomic_long_add(delta, &calc_load_tasks);
 }
 
 /*

kernel/sched/core.c migration_call(struct notifier_block *nfb, unsigned long action, void *hcpu) · line 5645

Quoted 6 lines

kernel/sched/core.c +1 -2

@@ -5652,8 +5642,7 @@ migration_call(struct notifier_block *nfb, unsigned long action, void *hcpu)
 		BUG_ON(rq->nr_running != 1); /* the migration thread */
 		raw_spin_unlock_irqrestore(&rq->lock, flags);
 
-		migrate_nr_uninterruptible(rq);
-		calc_global_load_remove(rq);
+		calc_load_migrate(rq);

Not sure that it matters, but...

This is called from the CPU_DYING notifier, which runs with irqs disabled, but in process context. As I understand it, this means that ->nr_running==1. If my understanding is correct (ha!), this means that this change sets ->calc_load_active to one (rather than zero as in the original) and that it subtracts one fewer from calc_load_tasks than did the original. Of course, I have no idea whether this matters.

If I am correct and if it does matter, one straightforward fix is to add a "CPU_DEAD" branch to the switch statement and move the "calc_load_migrate(rq)" to that new branch. Given that "rq" references the outgoing CPU, my guess is that locking is not needed, but you would know better than I.

Quoted 1 line · Paul E. McKenney

On Mon, Aug 20, 2012 at 11:26:57AM +0200, Peter Zijlstra wrote:

How about the following updated patch?

Thanx, Paul

------------------------------------------------------------------------

sched: Fix load avg vs cpu-hotplug

Rabik and Paul reported two different issues related to the same few lines of code.

Rabik's issue is that the nr_uninterruptible migration code is wrong in that he sees artifacts due to this (Rabik please do expand in more detail).

Paul's issue is that this code as it stands relies on us using stop_machine() for unplug, we all would like to remove this assumption so that eventually we can remove this stop_machine() usage altogether.

The only reason we'd have to migrate nr_uninterruptible is so that we could use for_each_online_cpu() loops in favour of for_each_possible_cpu() loops, however since nr_uninterruptible() is the only such loop and its using possible lets not bother at all.

The problem Rabik sees is (probably) caused by the fact that by migrating nr_uninterruptible we screw rq->calc_load_active for both rqs involved.

So don't bother with fancy migration schemes (meaning we now have to keep using for_each_possible_cpu()) and instead fold any nr_active delta after we migrate all tasks away to make sure we don't have any skewed nr_active accounting.

[ paulmck: Move call to calc_load_migration to CPU_DEAD to avoid miscounting noted by Rakib. ]

Reported-by Rakib Mullick <[email protected]>

Reported-by Paul E. McKenney <[email protected]>

Signed-off-by Peter Zijlstra <[email protected]>

Signed-off-by Paul E. McKenney <[email protected]>

kernel/sched/core.c +9 -19

index e841dfc..a8807f2 100644
@@ -5309,27 +5309,17 @@ void idle_task_exit(void)
 }
 
 /*
- * While a dead CPU has no uninterruptible tasks queued at this point,
- * it might still have a nonzero ->nr_uninterruptible counter, because
- * for performance reasons the counter is not stricly tracking tasks to
- * their home CPUs. So we just add the counter to another CPU's counter,
- * to keep the global sum constant after CPU-down:
- */
-static void migrate_nr_uninterruptible(struct rq *rq_src)
-{
-	struct rq *rq_dest = cpu_rq(cpumask_any(cpu_active_mask));
-
-	rq_dest->nr_uninterruptible += rq_src->nr_uninterruptible;
-	rq_src->nr_uninterruptible = 0;
-}
-
-/*
- * remove the tasks which were accounted by rq from calc_load_tasks.
+ * Since this CPU is going 'away' for a while, fold any nr_active delta
+ * we might have. Assumes we're called after migrate_tasks() so that the
+ * nr_active count is stable.
+ *
+ * Also see the comment "Global load-average calculations".
  */
-static void calc_global_load_remove(struct rq *rq)
+static void calc_load_migrate(struct rq *rq)
 {
-	atomic_long_sub(rq->calc_load_active, &calc_load_tasks);
-	rq->calc_load_active = 0;
+	long delta = calc_load_fold_active(rq);
+	if (delta)
+		atomic_long_add(delta, &calc_load_tasks);
 }
 
 /*

@@ -5622,9 +5612,18 @@ migration_call(struct notifier_block *nfb, unsigned
 long action, void *hcpu)
 		migrate_tasks(cpu);
 		BUG_ON(rq->nr_running != 1); /* the migration thread */
 		raw_spin_unlock_irqrestore(&rq->lock, flags);

Hello Paul,

Quoted 5 lines · Paul E. McKenney

Quoted 1 line · Paul E. McKenney

On Mon, Aug 20, 2012 at 11:26:57AM +0200, Peter Zijlstra wrote:

How about the following updated patch?

Actually, I was waiting for Peter's update.

Quoted 87 lines

Thanx, Paul

------------------------------------------------------------------------

sched: Fix load avg vs cpu-hotplug

Rabik and Paul reported two different issues related to the same few lines of code.

Rabik's issue is that the nr_uninterruptible migration code is wrong in that he sees artifacts due to this (Rabik please do expand in more detail).

Paul's issue is that this code as it stands relies on us using stop_machine() for unplug, we all would like to remove this assumption so that eventually we can remove this stop_machine() usage altogether.

The only reason we'd have to migrate nr_uninterruptible is so that we could use for_each_online_cpu() loops in favour of for_each_possible_cpu() loops, however since nr_uninterruptible() is the only such loop and its using possible lets not bother at all.

The problem Rabik sees is (probably) caused by the fact that by migrating nr_uninterruptible we screw rq->calc_load_active for both rqs involved.

So don't bother with fancy migration schemes (meaning we now have to keep using for_each_possible_cpu()) and instead fold any nr_active delta after we migrate all tasks away to make sure we don't have any skewed nr_active accounting.

[ paulmck: Move call to calc_load_migration to CPU_DEAD to avoid miscounting noted by Rakib. ]

Reported-by Rakib Mullick <[email protected]>

Reported-by Paul E. McKenney <[email protected]>

Signed-off-by Peter Zijlstra <[email protected]>

Signed-off-by Paul E. McKenney <[email protected]>

kernel/sched/core.c +10 -19

index e841dfc..a8807f2 100644
@@ -5309,27 +5309,17 @@ void idle_task_exit(void)
 }
 
 /*
- * While a dead CPU has no uninterruptible tasks queued at this point,
- * it might still have a nonzero ->nr_uninterruptible counter, because
- * for performance reasons the counter is not stricly tracking tasks to
- * their home CPUs. So we just add the counter to another CPU's counter,
- * to keep the global sum constant after CPU-down:
- */
-static void migrate_nr_uninterruptible(struct rq *rq_src)
-{
-	struct rq *rq_dest = cpu_rq(cpumask_any(cpu_active_mask));
-
-	rq_dest->nr_uninterruptible += rq_src->nr_uninterruptible;
-	rq_src->nr_uninterruptible = 0;
-}
-
-/*
- * remove the tasks which were accounted by rq from calc_load_tasks.
+ * Since this CPU is going 'away' for a while, fold any nr_active delta
+ * we might have. Assumes we're called after migrate_tasks() so that the
+ * nr_active count is stable.
+ *
+ * Also see the comment "Global load-average calculations".
  */
-static void calc_global_load_remove(struct rq *rq)
+static void calc_load_migrate(struct rq *rq)
 {
-	atomic_long_sub(rq->calc_load_active, &calc_load_tasks);
-	rq->calc_load_active = 0;
+	long delta = calc_load_fold_active(rq);
+	if (delta)
+		atomic_long_add(delta, &calc_load_tasks);
 }
 
 /*

@@ -5622,9 +5612,18 @@ migration_call(struct notifier_block *nfb, unsigned
 long action, void *hcpu)
 		migrate_tasks(cpu);
 		BUG_ON(rq->nr_running != 1); /* the migration thread */
 		raw_spin_unlock_irqrestore(&rq->lock, flags);
+		break;
 

kernel/sched/core.c migration_call(struct notifier_block *nfb, unsigned long action, void *hcpu) · line 5622

Quoted 8 lines

kernel/sched/core.c +6 -2

-		migrate_nr_uninterruptible(rq);
-		calc_global_load_remove(rq);
+	case CPU_DEAD:
+		{
+			struct rq *dest_rq;
+
+			local_irq_save(flags);
+			dest_rq = cpu_rq(smp_processor_id());

Use of smp_processor_id() as dest cpu isn't clear to me, this processor is about to get down, isn't it?

kernel/sched/core.c migration_call(struct notifier_block *nfb, unsigned long action, void *hcpu) · line 5624

Quoted 2 lines

kernel/sched/core.c +2 -0

+			raw_spin_lock(&dest_rq->lock);
+			calc_load_migrate(rq);

Well, calc_load_migrate() has no impact cause rq->nr_running == 1 at this point. It's been already pointed out previously.

Quoted 9 lines · Rakib Mullick

Hello Paul,

Quoted 5 lines · Paul E. McKenney

Quoted 1 line · Paul E. McKenney

On Mon, Aug 20, 2012 at 11:26:57AM +0200, Peter Zijlstra wrote:

How about the following updated patch?

Actually, I was waiting for Peter's update.

I was too, but chatted with Peter.

Quoted 94 lines

Quoted 94 lines

Thanx, Paul

------------------------------------------------------------------------

sched: Fix load avg vs cpu-hotplug

Rabik and Paul reported two different issues related to the same few lines of code.

Rabik's issue is that the nr_uninterruptible migration code is wrong in that he sees artifacts due to this (Rabik please do expand in more detail).

Paul's issue is that this code as it stands relies on us using stop_machine() for unplug, we all would like to remove this assumption so that eventually we can remove this stop_machine() usage altogether.

The only reason we'd have to migrate nr_uninterruptible is so that we could use for_each_online_cpu() loops in favour of for_each_possible_cpu() loops, however since nr_uninterruptible() is the
only such loop and its using possible lets not bother at all.

The problem Rabik sees is (probably) caused by the fact that by migrating nr_uninterruptible we screw rq->calc_load_active for both rqs involved.

So don't bother with fancy migration schemes (meaning we now have to keep using for_each_possible_cpu()) and instead fold any nr_active delta
after we migrate all tasks away to make sure we don't have any skewed nr_active accounting.

[ paulmck: Move call to calc_load_migration to CPU_DEAD to avoid miscounting noted by Rakib. ]

Reported-by Rakib Mullick <[email protected]>

Reported-by Paul E. McKenney <[email protected]>

Signed-off-by Peter Zijlstra <[email protected]>

Signed-off-by Paul E. McKenney <[email protected]>

kernel/sched/core.c +0 -3

index e841dfc..a8807f2 100644
@@ -5309,27 +5309,17 @@ void idle_task_exit(void)
 }
 
 /*
- * While a dead CPU has no uninterruptible tasks queued at this point,
- * it might still have a nonzero ->nr_uninterruptible counter, because
- * for performance reasons the counter is not stricly tracking tasks

to
- * their home CPUs. So we just add the counter to another CPU's counter,
- * to keep the global sum constant after CPU-down:
- */

kernel/sched/core.c +1 -10

-static void migrate_nr_uninterruptible(struct rq *rq_src)
-{
-	struct rq *rq_dest = cpu_rq(cpumask_any(cpu_active_mask));
-
-	rq_dest->nr_uninterruptible += rq_src->nr_uninterruptible;
-	rq_src->nr_uninterruptible = 0;
-}
-
-/*
- * remove the tasks which were accounted by rq from calc_load_tasks.
+ * Since this CPU is going 'away' for a while, fold any nr_active

delta
+ * we might have. Assumes we're called after migrate_tasks() so that the
+ * nr_active count is stable.
+ *
+ * Also see the comment "Global load-average calculations". */

kernel/sched/core.c +4 -3

-static void calc_global_load_remove(struct rq *rq)
+static void calc_load_migrate(struct rq *rq)
 {
-	atomic_long_sub(rq->calc_load_active, &calc_load_tasks);
-	rq->calc_load_active = 0;
+	long delta = calc_load_fold_active(rq);
+	if (delta)
+		atomic_long_add(delta, &calc_load_tasks);
 }
 
 /*

@@ -5622,9 +5612,18 @@ migration_call(struct notifier_block *nfb,

unsigned
long action, void *hcpu)

kernel/sched/core.c +1 -0

 		migrate_tasks(cpu);
 		BUG_ON(rq->nr_running != 1); /* the migration thread */
 		raw_spin_unlock_irqrestore(&rq->lock, flags);
+		break;

Quoted 11 lines

kernel/sched/core.c migration_call(struct notifier_block *nfb, unsigned long action, void *hcpu) · line 5622

Quoted 8 lines

kernel/sched/core.c +6 -2

-		migrate_nr_uninterruptible(rq);
-		calc_global_load_remove(rq);
+	case CPU_DEAD:
+		{
+			struct rq *dest_rq;
+
+			local_irq_save(flags);
+			dest_rq = cpu_rq(smp_processor_id());

Use of smp_processor_id() as dest cpu isn't clear to me, this processor is about to get down, isn't it?

Nope. The CPU_DEAD notifier happens after the outgoing CPU has been fully offlined, and so it must run on some other CPU.

Quoted 5 lines

kernel/sched/core.c migration_call(struct notifier_block *nfb, unsigned long action, void *hcpu) · line 5624

Quoted 2 lines

kernel/sched/core.c +2 -0

+			raw_spin_lock(&dest_rq->lock);
+			calc_load_migrate(rq);

Well, calc_load_migrate() has no impact cause rq->nr_running == 1 at this point. It's been already pointed out previously.

Even after the outgoing CPU is fully gone? I would hope that the value would be zero.

Quoted 122 lines · Paul E. McKenney

Quoted 9 lines · Rakib Mullick

Hello Paul,

Quoted 5 lines · Paul E. McKenney

Quoted 1 line · Paul E. McKenney

On Mon, Aug 20, 2012 at 11:26:57AM +0200, Peter Zijlstra wrote:

How about the following updated patch?

Actually, I was waiting for Peter's update.

I was too, but chatted with Peter.

Quoted 94 lines

Quoted 94 lines

Thanx, Paul

------------------------------------------------------------------------

sched: Fix load avg vs cpu-hotplug

Rabik and Paul reported two different issues related to the same few lines of code.

Rabik's issue is that the nr_uninterruptible migration code is wrong in that he sees artifacts due to this (Rabik please do expand in more detail).

Paul's issue is that this code as it stands relies on us using stop_machine() for unplug, we all would like to remove this assumption so that eventually we can remove this stop_machine() usage altogether.

The only reason we'd have to migrate nr_uninterruptible is so that we could use for_each_online_cpu() loops in favour of for_each_possible_cpu() loops, however since nr_uninterruptible() is the
only such loop and its using possible lets not bother at all.

The problem Rabik sees is (probably) caused by the fact that by migrating nr_uninterruptible we screw rq->calc_load_active for both rqs involved.

So don't bother with fancy migration schemes (meaning we now have to keep using for_each_possible_cpu()) and instead fold any nr_active delta
after we migrate all tasks away to make sure we don't have any skewed nr_active accounting.

[ paulmck: Move call to calc_load_migration to CPU_DEAD to avoid miscounting noted by Rakib. ]

Reported-by Rakib Mullick <[email protected]>

Reported-by Paul E. McKenney <[email protected]>

Signed-off-by Peter Zijlstra <[email protected]>

Signed-off-by Paul E. McKenney <[email protected]>

kernel/sched/core.c +0 -3

index e841dfc..a8807f2 100644
@@ -5309,27 +5309,17 @@ void idle_task_exit(void)
 }
 
 /*
- * While a dead CPU has no uninterruptible tasks queued at this point,
- * it might still have a nonzero ->nr_uninterruptible counter, because
- * for performance reasons the counter is not stricly tracking tasks

to
- * their home CPUs. So we just add the counter to another CPU's counter,
- * to keep the global sum constant after CPU-down:
- */

kernel/sched/core.c +1 -10

-static void migrate_nr_uninterruptible(struct rq *rq_src)
-{
-	struct rq *rq_dest = cpu_rq(cpumask_any(cpu_active_mask));
-
-	rq_dest->nr_uninterruptible += rq_src->nr_uninterruptible;
-	rq_src->nr_uninterruptible = 0;
-}
-
-/*
- * remove the tasks which were accounted by rq from calc_load_tasks.
+ * Since this CPU is going 'away' for a while, fold any nr_active

delta
+ * we might have. Assumes we're called after migrate_tasks() so that the
+ * nr_active count is stable.
+ *
+ * Also see the comment "Global load-average calculations". */

kernel/sched/core.c +4 -3

-static void calc_global_load_remove(struct rq *rq)
+static void calc_load_migrate(struct rq *rq)
 {
-	atomic_long_sub(rq->calc_load_active, &calc_load_tasks);
-	rq->calc_load_active = 0;
+	long delta = calc_load_fold_active(rq);
+	if (delta)
+		atomic_long_add(delta, &calc_load_tasks);
 }
 
 /*

@@ -5622,9 +5612,18 @@ migration_call(struct notifier_block *nfb,

unsigned
long action, void *hcpu)

kernel/sched/core.c +1 -0

 		migrate_tasks(cpu);
 		BUG_ON(rq->nr_running != 1); /* the migration thread */
 		raw_spin_unlock_irqrestore(&rq->lock, flags);
+		break;

Quoted 11 lines

kernel/sched/core.c migration_call(struct notifier_block *nfb, unsigned long action, void *hcpu) · line 5622

Quoted 8 lines

kernel/sched/core.c +6 -2

-		migrate_nr_uninterruptible(rq);
-		calc_global_load_remove(rq);
+	case CPU_DEAD:
+		{
+			struct rq *dest_rq;
+
+			local_irq_save(flags);
+			dest_rq = cpu_rq(smp_processor_id());

Use of smp_processor_id() as dest cpu isn't clear to me, this processor is about to get down, isn't it?

Nope. The CPU_DEAD notifier happens after the outgoing CPU has been fully offlined, and so it must run on some other CPU.

Quoted 9 lines

Quoted 5 lines

kernel/sched/core.c migration_call(struct notifier_block *nfb, unsigned long action, void *hcpu) · line 5624

Quoted 2 lines

kernel/sched/core.c +2 -0

+			raw_spin_lock(&dest_rq->lock);
+			calc_load_migrate(rq);

Well, calc_load_migrate() has no impact cause rq->nr_running == 1 at this point. It's been already pointed out previously.

Even after the outgoing CPU is fully gone? I would hope that the value would be zero.

Perhaps, yes and it doesn't make any difference. And so, at this point doing calc_load_migrate()... I'm not sure. But, I'm sure that, this is not what I had in my mind.

The patch I sent it was to move rq->nr_uninterruptible to the dest_rq where all the tasks were moved. Then, Peter says that patch isn't correct, cause tasks might get spread out amongst more than one CPU due to tasks affinity (*if* task was affined). But, we can easily expect that, admin is smart enough to not put a CPU offline, where s/he put a task to explicitly run on. I agree that, my patch isn't 100% correct but it's better than current which is almost 100% wrong which picks up a random CPU-rq to move rq->nr_uninterruptible count. So, simply moving ->nr_uninterruptible to dest_rq (where tasks were moved) should be fine. And when dest_rq's time will come to go idle i.e calc_load_enter_idle() or calc_load_account_active() (if no_hz=n), active count will get folded, we do not need to explicitly fold nr_active count when CPU is going to die.

OK, but I thought that Peter said that ->nr_uninterruptible was meaningful only when summed across all CPUs. If that is the case, it shouldn't matter where the counts are moved.

Commit-ID:  f319da0c6894fcf55e21320e40506418a2aad629
Gitweb:     http://git.kernel.org/tip/f319da0c6894fcf55e21320e40506418a2aad629
Author:     Peter Zijlstra <[email protected]>
AuthorDate: Mon, 20 Aug 2012 11:26:57 +0200
Committer:  Ingo Molnar <[email protected]>
CommitDate: Tue, 4 Sep 2012 14:30:18 +0200

sched: Fix load avg vs cpu-hotplug

Rabik and Paul reported two different issues related to the same few lines of code.

Rabik's issue is that the nr_uninterruptible migration code is wrong in that he sees artifacts due to this (Rabik please do expand in more detail).

Paul's issue is that this code as it stands relies on us using stop_machine() for unplug, we all would like to remove this assumption so that eventually we can remove this stop_machine() usage altogether.

The only reason we'd have to migrate nr_uninterruptible is so that we could use for_each_online_cpu() loops in favour of for_each_possible_cpu() loops, however since nr_uninterruptible() is the only such loop and its using possible lets not bother at all.

The problem Rabik sees is (probably) caused by the fact that by migrating nr_uninterruptible we screw rq->calc_load_active for both rqs involved.

So don't bother with fancy migration schemes (meaning we now have to keep using for_each_possible_cpu()) and instead fold any nr_active delta after we migrate all tasks away to make sure we don't have any skewed nr_active accounting.

Quoted 29 lines · tip-bot for Peter Zijlstra

Commit-ID:  f319da0c6894fcf55e21320e40506418a2aad629
Gitweb:     http://git.kernel.org/tip/f319da0c6894fcf55e21320e40506418a2aad629
Author:     Peter Zijlstra <[email protected]>
AuthorDate: Mon, 20 Aug 2012 11:26:57 +0200
Committer:  Ingo Molnar <[email protected]>
CommitDate: Tue, 4 Sep 2012 14:30:18 +0200

sched: Fix load avg vs cpu-hotplug

Rabik and Paul reported two different issues related to the same few lines of code.

Rabik's issue is that the nr_uninterruptible migration code is wrong in that he sees artifacts due to this (Rabik please do expand in more detail).

Paul's issue is that this code as it stands relies on us using stop_machine() for unplug, we all would like to remove this assumption so that eventually we can remove this stop_machine() usage altogether.

The only reason we'd have to migrate nr_uninterruptible is so that we could use for_each_online_cpu() loops in favour of for_each_possible_cpu() loops, however since nr_uninterruptible() is the only such loop and its using possible lets not bother at all.

The problem Rabik sees is (probably) caused by the fact that by migrating nr_uninterruptible we screw rq->calc_load_active for both rqs involved.

Quoted 4 lines

So don't bother with fancy migration schemes (meaning we now have to keep using for_each_possible_cpu()) and instead fold any nr_active delta after we migrate all tasks away to make sure we don't have any skewed nr_active accounting.

Oh argh.. this patch isn't actually right.. I actually removed it from my series but forgot to update the tarball.

Ingo can you still make it go away or should I do a delta?

Quoted 2 lines

Oh argh.. this patch isn't actually right.. I actually removed it from my series but forgot to update the tarball.

Sigh.

Quoted 1 line

Ingo can you still make it go away or should I do a delta?

Please do a delta.

Quoted 4 lines · Ingo Molnar

Quoted 2 lines

Oh argh.. this patch isn't actually right.. I actually removed it from my series but forgot to update the tarball.

Sigh.

Yeah, sorry about that, jet-lag makes me do stupid at a higher rate than usual :/

Quoted 3 lines

Quoted 1 line

Ingo can you still make it go away or should I do a delta?

Please do a delta.

Ok, will do.

Quoted 1 line

Please do a delta.

Quoted 1 line · Peter Zijlstra

Quoted 1 line

Please do a delta.

OK, so I suppose something like the below ought to do. Paul its slightly different than the one in your tree, given the changelog below, do you see anything wrong with it?

Rakib, again, sorry for getting your name wrong, and this time for getting it merged :/

---
Subject: sched: Fix load avg vs cpu-hotplug mk-II

Commit f319da0c68 ("sched: Fix load avg vs cpu-hotplug") was a known broken version that got in by accident.

In particular, the problem is that at the point it calls calc_load_migrate() nr_running := 1 (the stopper thread), so move the call to CPU_DEAD where we're sure that nr_running := 0.

Also note that we can call calc_load_migrate() without serialization, we know the state of rq is stable since its cpu is dead, and we modify the global state using appropriate atomic ops.

Suggested-by Paul E. McKenney <[email protected]>

Signed-off-by Peter Zijlstra <[email protected]>

---
 kernel/sched/core.c | 2 ++
 1 file changed, 2 insertions(+)

kernel/sched/core.c +2 -0

index c46a011..8c089cb 100644
@@ -5086,7 +5086,9 @@ migration_call(struct notifier_block *nfb, unsigned long action, void *hcpu)
 		migrate_tasks(cpu);
 		BUG_ON(rq->nr_running != 1); /* the migration thread */
 		raw_spin_unlock_irqrestore(&rq->lock, flags);
+		break;
 
+	case CPU_DEAD:
 		calc_load_migrate(rq);
 		break;
 #endif

Quoted 1 line · Peter Zijlstra

Quoted 1 line

Please do a delta.

OK, so I suppose something like the below ought to do. Paul its slightly different than the one in your tree, given the changelog below, do you see anything wrong with it?

Rakib, again, sorry for getting your name wrong, and this time for getting it merged :/