On 21/01/14 13:48, Jon Medhurst (Tixy) wrote:
On Tue, 2014-01-21 at 13:03 +0000, Jon Medhurst wrote:
From: Chris Redpath chris.redpath@arm.com
If we migrate a sleeping task away from a CPU which has the tick stopped, then both the clock_task and decay_counter will be out of date for that CPU and we will not decay load correctly regardless of how often we update the blocked load.
This is only an issue for tasks which are not on a runqueue (because otherwise that CPU would be awake) and simultaneously the CPU the task previously ran on has had the tick stopped.
Signed-off-by: Chris Redpath chris.redpath@arm.com Signed-off-by: Jon Medhurst tixy@linaro.org
kernel/sched/fair.c | 44 +++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 43 insertions(+), 1 deletion(-)
diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c index 01cc427..86e82c8 100644 --- a/kernel/sched/fair.c +++ b/kernel/sched/fair.c @@ -4379,6 +4379,7 @@ unlock:
- load-balance).
*/ #ifdef CONFIG_FAIR_GROUP_SCHED +static int nohz_test_cpu(int cpu);
This forward declaration is for two versions of the functions defined later, but with the length of the file and all the #if's it's not totally obvious that there aren't combinations of options that will lead to this function not getting defined. A suggestion to make this a bit more robust and clean, would be to define the empty function at this location as well, e.g.
#ifdef CONFIG_NO_HZ_COMMON static int nohz_test_cpu(int cpu); #else static inline int nohz_test_cpu(int cpu) { return 0; } #endif
Then no need to add this dummy version later on.
OK, good spot.
/*
- Called immediately before a task is migrated to a new cpu; task_cpu(p) and
- cfs_rq_of(p) references at time of call are still valid and identify the
@@ -4390,7 +4391,6 @@ migrate_task_rq_fair(struct task_struct *p, int next_cpu) { struct sched_entity *se = &p->se; struct cfs_rq *cfs_rq = cfs_rq_of(se);
A spurious line removal seems to have crept in above.
ditto
/* * Load tracking: accumulate removed load so that it can be processed * when we next update owning cfs_rq under rq->lock. Tasks contribute @@ -4398,6 +4398,25 @@ migrate_task_rq_fair(struct task_struct *p, int next_cpu) * be negative here since on-rq tasks have decay-count == 0. */ if (se->avg.decay_count) {
/** If we migrate a sleeping task away from a CPU* which has the tick stopped, then both the clock_task* and decay_counter will be out of date for that CPU* and we will not decay load correctly.*/if (!se->on_rq && nohz_test_cpu(task_cpu(p))) {struct rq *rq = cpu_rq(task_cpu(p));unsigned long flags;/** Current CPU cannot be holding rq->lock in this* circumstance, but another might be. We must hold* rq->lock before we go poking around in its clocks*/raw_spin_lock_irqsave(&rq->lock, flags);update_rq_clock(rq);update_cfs_rq_blocked_load(cfs_rq, 0);raw_spin_unlock_irqrestore(&rq->lock, flags);}@@ -6329,6 +6348,17 @@ static struct { atomic_t nr_cpus; unsigned long next_balance; /* in jiffy units */ } nohz ____cacheline_aligned;
Nit-pick, there possibly should be a blank line here.
Also, this patch (while agreed to be correct) was clearly flagged as a non-optimal way to fix the issue. There isn't another way right now, but I'm on the hook to provide one there. I have a partial solution but I don't like that much either.
Is it OK to add a revert patch for this to linaro-stable later, once the proper solution is available in mainline?
+/*
- nohz_test_cpu used when load tracking is enabled. FAIR_GROUP_SCHED
- dependency below may be removed when load tracking guards are
- removed.#ifdef CONFIG_NO_HZ_COMMON
- */
+#ifdef CONFIG_FAIR_GROUP_SCHED +static int nohz_test_cpu(int cpu) +{
- return cpumask_test_cpu(cpu, nohz.idle_cpus_mask);
+} +#endif
#ifdef CONFIG_SCHED_HMP_LITTLE_PACKING /*
[...]
Do you want me to resend this patch updated or will you do it Tixy?
--Chris