Thanks Tixy, I really appreciate you looking at these.
On 14/03/14 09:58, Jon Medhurst (Tixy) wrote:
On Thu, 2014-03-13 at 17:19 +0000, Chris Redpath wrote:
In anticipation of modifying the up_threshold handling, make all instances use the same utility fn to check if a task is eligible for up-migration.
Signed-off-by: Chris Redpath chris.redpath@arm.com
kernel/sched/fair.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-)
diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c index 62a8808..1b9be59 100644 --- a/kernel/sched/fair.c +++ b/kernel/sched/fair.c @@ -6734,6 +6734,13 @@ static void nohz_idle_balance(int this_cpu, enum cpu_idle_type idle) { } #endif
#ifdef CONFIG_SCHED_HMP +static unsigned int hmp_task_eligible_for_up_migration(struct sched_entity *se) +{
- /* below hmp_up_threshold, never eligible */
- if (se->avg.load_avg_ratio < hmp_up_threshold)
return 0;- return 1;
+}
Guess there should be a blank line added here as well? (I can do that before adding this patch to the MP topic branch), but I have another comment below about a possible bug...
Yes, please add a line there.
/* Check if task should migrate to a faster cpu */ static unsigned int hmp_up_migration(int cpu, int *target_cpu, struct sched_entity *se) { @@ -6749,7 +6756,7 @@ static unsigned int hmp_up_migration(int cpu, int *target_cpu, struct sched_enti if (p->prio >= hmp_up_prio) return 0; #endif
- if (se->avg.load_avg_ratio < hmp_up_threshold)
if (!hmp_task_eligible_for_up_migration(se)) return 0;
/* Let the task load settle before doing another up migration */
@@ -7237,7 +7244,10 @@ static unsigned int hmp_idle_pull(int this_cpu) } orig = curr; curr = hmp_get_heaviest_task(curr, 1);
if (curr->avg.load_avg_ratio > hmp_up_threshold &&
/* check if heaviest eligible task on this* CPU is heavier than previous task*/if (hmp_task_eligible_for_up_migration(curr) &&This isn't strictly a direct replacement of like-for-like because you've defectively replaced:
curr->avg.load_avg_ratio > hmp_up_threshold
with !(se->avg.load_avg_ratio < hmp_up_threshold)
which gives different results when the values compared are equal, I don't know if that makes much practical difference? In fact, new use possibly make more sense, as before there was a situation where hmp_force_up_migration could make different decisions to hmp_idle_pull. (I'm not particularly familiar with the code so I may not be talking sense.)
Well spotted :) This is intentional, and your comment makes perfect sense. Putting the decision in a fn means I get the opportunity to express the logical intent in the name, which I like to do. As you spotted, the existing code behaved in both ways - up migration was aborted if the load was < threshold and idle pull only attempted if load was > threshold. There is clearly a difference in what load == threshold means here.
In all the documentation we imply that a task should be eligible for up-migration when we reach the threshold rather than pass it, and given that up-migration is the primary cause of big CPU use I thought that was the right model to choose.
In practise, I don't expect any difference at all :) If an idle pull was missed because the load was only ==threshold, it would likely have been up-migrated at or around the next scheduler tick anyway.
curr->avg.load_avg_ratio > ratio) { p = task_of(curr); target = rq;