[PATCH] sched: fix exec_start/task_hot on migrated tasks

[PATCH] sched: fix exec_start/task_hot on migrated tasks

[Ben Segall]

task_hot checks exec_start on any runnable task, but if it has been
migrated since the it last ran, then exec_start is a clock_task from
another cpu. If the old cpu's clock_task was sufficiently far ahead of
this cpu's then the task will not be considered for another migration
until it has run. Instead reset exec_start whenever a task is migrated,
since it is presumably no longer hot anyway.

Signed-off-by: Ben Segall <bsegall@google.com>
---
 kernel/sched/fair.c |    3 +++
 1 file changed, 3 insertions(+)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 28ccf50..9f8dfeb 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -4544,6 +4544,9 @@ migrate_task_rq_fair(struct task_struct *p, int next_cpu)
 		atomic_long_add(se->avg.load_avg_contrib,
 						&cfs_rq->removed_load);
 	}
+
+	/* We have migrated, no longer consider this task hot */
+	se.exec_start = 0;
 }
 #endif /* CONFIG_SMP */
 

Re: [PATCH] sched: fix exec_start/task_hot on migrated tasks

[Peter Zijlstra]

[-- Attachment #1: Type: text/plain, Size: 1180 bytes --]

On Thu, May 15, 2014 at 03:59:20PM -0700, Ben Segall wrote:
> task_hot checks exec_start on any runnable task, but if it has been
> migrated since the it last ran, then exec_start is a clock_task from
> another cpu. If the old cpu's clock_task was sufficiently far ahead of
> this cpu's then the task will not be considered for another migration
> until it has run. Instead reset exec_start whenever a task is migrated,
> since it is presumably no longer hot anyway.
> 
> Signed-off-by: Ben Segall <bsegall@google.com>
> ---
>  kernel/sched/fair.c |    3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 28ccf50..9f8dfeb 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -4544,6 +4544,9 @@ migrate_task_rq_fair(struct task_struct *p, int next_cpu)
>  		atomic_long_add(se->avg.load_avg_contrib,
>  						&cfs_rq->removed_load);
>  	}
> +
> +	/* We have migrated, no longer consider this task hot */
> +	se.exec_start = 0;
>  }

0 isn't strictly the right thing to do here, since the clock can wrap,
but being wrong every ~585 years isn't too big an issue for this.

Thanks!

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] sched: fix exec_start/task_hot on migrated tasks

[Peter Zijlstra]

[-- Attachment #1: Type: text/plain, Size: 1062 bytes --]

On Thu, May 15, 2014 at 03:59:20PM -0700, Ben Segall wrote:
> task_hot checks exec_start on any runnable task, but if it has been
> migrated since the it last ran, then exec_start is a clock_task from
> another cpu. If the old cpu's clock_task was sufficiently far ahead of
> this cpu's then the task will not be considered for another migration
> until it has run. Instead reset exec_start whenever a task is migrated,
> since it is presumably no longer hot anyway.
> 
> Signed-off-by: Ben Segall <bsegall@google.com>
> ---
>  kernel/sched/fair.c |    3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 28ccf50..9f8dfeb 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -4544,6 +4544,9 @@ migrate_task_rq_fair(struct task_struct *p, int next_cpu)
>  		atomic_long_add(se->avg.load_avg_contrib,
>  						&cfs_rq->removed_load);
>  	}
> +
> +	/* We have migrated, no longer consider this task hot */
> +	se.exec_start = 0;

se->exec_start compiles loads better

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] sched: fix exec_start/task_hot on migrated tasks

[Preeti Murthy]

Hi,

On Fri, May 16, 2014 at 1:34 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Thu, May 15, 2014 at 03:59:20PM -0700, Ben Segall wrote:
>> task_hot checks exec_start on any runnable task, but if it has been
>> migrated since the it last ran, then exec_start is a clock_task from
>> another cpu. If the old cpu's clock_task was sufficiently far ahead of
>> this cpu's then the task will not be considered for another migration
>> until it has run. Instead reset exec_start whenever a task is migrated,
>> since it is presumably no longer hot anyway.
>>
>> Signed-off-by: Ben Segall <bsegall@google.com>
>> ---
>>  kernel/sched/fair.c |    3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
>> index 28ccf50..9f8dfeb 100644
>> --- a/kernel/sched/fair.c
>> +++ b/kernel/sched/fair.c
>> @@ -4544,6 +4544,9 @@ migrate_task_rq_fair(struct task_struct *p, int next_cpu)
>>               atomic_long_add(se->avg.load_avg_contrib,
>>                                               &cfs_rq->removed_load);
>>       }
>> +
>> +     /* We have migrated, no longer consider this task hot */
>> +     se.exec_start = 0;
>>  }
>
> 0 isn't strictly the right thing to do here, since the clock can wrap,
> but being wrong every ~585 years isn't too big an issue for this.

I don't understand this. Will setting it to 0 not indicate beginning
of ticking? So when you find out for how long the task has run, the
difference would be larger than what would have been had you
let exec_start be at its previous value of the old cpu's clock_task right?

Will not setting exec_start to the clock_task of the destination rq
during migration be better? This would be the closest we could
come to estimating the amount of time the task has run on this new
cpu while deciding task_hot or not no?

Regards
Preeti U Murthy

>
> Thanks!

Re: [PATCH] sched: fix exec_start/task_hot on migrated tasks

[Peter Zijlstra]

[-- Attachment #1: Type: text/plain, Size: 1320 bytes --]

On Fri, May 16, 2014 at 07:27:32PM +0530, Preeti Murthy wrote:
> > 0 isn't strictly the right thing to do here, since the clock can wrap,
> > but being wrong every ~585 years isn't too big an issue for this.
> 
> I don't understand this. Will setting it to 0 not indicate beginning
> of ticking? 

In modular spaces there is no beginnings nor endings.

> So when you find out for how long the task has run, the
> difference would be larger than what would have been had you
> let exec_start be at its previous value of the old cpu's clock_task right?

Nope, see the modular space thing, once every ~585 years we'll wrap the
clock and 0 is within range of recently ran.

> Will not setting exec_start to the clock_task of the destination rq
> during migration be better? This would be the closest we could
> come to estimating the amount of time the task has run on this new
> cpu while deciding task_hot or not no?

Setting it to the exact clock_task of the destination rq would make it
hot on that rq, even though it hasn't yet ran there, so you'd have to do
something like: rq_clock_task(dst_rq) - sysctl_sched_migration_cost.

But seeing as how that is far more work, and all this is heuristics
anyhow and an extra fail term of 1/585 years is far below the current
fail rate, all is well.

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] sched: fix exec_start/task_hot on migrated tasks

[bsegall]

Peter Zijlstra <peterz@infradead.org> writes:

> On Thu, May 15, 2014 at 03:59:20PM -0700, Ben Segall wrote:
>> task_hot checks exec_start on any runnable task, but if it has been
>> migrated since the it last ran, then exec_start is a clock_task from
>> another cpu. If the old cpu's clock_task was sufficiently far ahead of
>> this cpu's then the task will not be considered for another migration
>> until it has run. Instead reset exec_start whenever a task is migrated,
>> since it is presumably no longer hot anyway.
>> 
>> Signed-off-by: Ben Segall <bsegall@google.com>
>> ---
>>  kernel/sched/fair.c |    3 +++
>>  1 file changed, 3 insertions(+)
>> 
>> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
>> index 28ccf50..9f8dfeb 100644
>> --- a/kernel/sched/fair.c
>> +++ b/kernel/sched/fair.c
>> @@ -4544,6 +4544,9 @@ migrate_task_rq_fair(struct task_struct *p, int next_cpu)
>>  		atomic_long_add(se->avg.load_avg_contrib,
>>  						&cfs_rq->removed_load);
>>  	}
>> +
>> +	/* We have migrated, no longer consider this task hot */
>> +	se.exec_start = 0;
>
> se->exec_start compiles loads better
Welp. Indeed it does.


From: Ben Segall <bsegall@google.com>
Date: Thu, 15 May 2014 15:38:10 -0700
Subject: [PATCH] sched: fix exec_start/task_hot on migrated tasks

task_hot checks exec_start on any runnable task, but if it has been
migrated since the it last ran, then exec_start is a clock_task from
another cpu. If the old cpu's clock_task was sufficiently far ahead of
this cpu's then the task will not be considered for another migration
until it has run. Instead reset exec_start whenever a task is migrated,
since it is presumably no longer hot anyway.

Signed-off-by: Ben Segall <bsegall@google.com>
---
 kernel/sched/fair.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 28ccf50..dd3fa14 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -4544,6 +4544,9 @@ migrate_task_rq_fair(struct task_struct *p, int next_cpu)
                atomic_long_add(se->avg.load_avg_contrib,
                                                &cfs_rq->removed_load);
        }
+
+       /* We have migrated, no longer consider this task hot */
+       se->exec_start = 0;
 }
 #endif /* CONFIG_SMP */
 

Re: [PATCH] sched: fix exec_start/task_hot on migrated tasks

[Preeti U Murthy]

On 05/16/2014 07:47 PM, Peter Zijlstra wrote:
> On Fri, May 16, 2014 at 07:27:32PM +0530, Preeti Murthy wrote:
>>> 0 isn't strictly the right thing to do here, since the clock can wrap,
>>> but being wrong every ~585 years isn't too big an issue for this.
>>
>> I don't understand this. Will setting it to 0 not indicate beginning
>> of ticking? 
> 
> In modular spaces there is no beginnings nor endings.
> 
>> So when you find out for how long the task has run, the
>> difference would be larger than what would have been had you
>> let exec_start be at its previous value of the old cpu's clock_task right?
> 
> Nope, see the modular space thing, once every ~585 years we'll wrap the
> clock and 0 is within range of recently ran.
> 
>> Will not setting exec_start to the clock_task of the destination rq
>> during migration be better? This would be the closest we could
>> come to estimating the amount of time the task has run on this new
>> cpu while deciding task_hot or not no?
> 
> Setting it to the exact clock_task of the destination rq would make it
> hot on that rq, even though it hasn't yet ran there, so you'd have to do
> something like: rq_clock_task(dst_rq) - sysctl_sched_migration_cost.
> 
> But seeing as how that is far more work, and all this is heuristics
> anyhow and an extra fail term of 1/585 years is far below the current
> fail rate, all is well.
> 

Ok now I understand this.Thanks!

Reviewed-by: Preeti U Murthy <preeti@linux.vnet.ibm.com>

[tip:sched/core] sched: Fix exec_start/task_hot on migrated tasks

[tip-bot for Ben Segall]

Commit-ID:  763e54552ad8eebb213db1fc591724067d83d95d
Gitweb:     http://git.kernel.org/tip/763e54552ad8eebb213db1fc591724067d83d95d
Author:     Ben Segall <bsegall@google.com>
AuthorDate: Thu, 15 May 2014 15:59:20 -0700
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Mon, 19 May 2014 22:02:39 +0900

sched: Fix exec_start/task_hot on migrated tasks

task_hot checks exec_start on any runnable task, but if it has been
migrated since the it last ran, then exec_start is a clock_task from
another cpu. If the old cpu's clock_task was sufficiently far ahead of
this cpu's then the task will not be considered for another migration
until it has run. Instead reset exec_start whenever a task is migrated,
since it is presumably no longer hot anyway.

Cc: Ingo Molnar <mingo@redhat.com>
Signed-off-by: Ben Segall <bsegall@google.com>
[peterz: made it compile]
Signed-off-by: Peter Zijlstra <peterz@infradead.org>
Link: http://lkml.kernel.org/r/20140515225920.7179.13924.stgit@sword-of-the-dawn.mtv.corp.google.com
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 kernel/sched/fair.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 28ccf50..dd3fa14 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -4544,6 +4544,9 @@ migrate_task_rq_fair(struct task_struct *p, int next_cpu)
 		atomic_long_add(se->avg.load_avg_contrib,
 						&cfs_rq->removed_load);
 	}
+
+	/* We have migrated, no longer consider this task hot */
+	se->exec_start = 0;
 }
 #endif /* CONFIG_SMP */
 

[tip:sched/core] sched: Fix exec_start/task_hot on migrated tasks

[tip-bot for Ben Segall]

Commit-ID:  3944a9274ef6cda0cc282daf0739832f661670f7
Gitweb:     http://git.kernel.org/tip/3944a9274ef6cda0cc282daf0739832f661670f7
Author:     Ben Segall <bsegall@google.com>
AuthorDate: Thu, 15 May 2014 15:59:20 -0700
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Thu, 22 May 2014 11:16:25 +0200

sched: Fix exec_start/task_hot on migrated tasks

task_hot checks exec_start on any runnable task, but if it has been
migrated since the it last ran, then exec_start is a clock_task from
another cpu. If the old cpu's clock_task was sufficiently far ahead of
this cpu's then the task will not be considered for another migration
until it has run. Instead reset exec_start whenever a task is migrated,
since it is presumably no longer hot anyway.

Signed-off-by: Ben Segall <bsegall@google.com>
[ Made it compile. ]
Signed-off-by: Peter Zijlstra <peterz@infradead.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Link: http://lkml.kernel.org/r/20140515225920.7179.13924.stgit@sword-of-the-dawn.mtv.corp.google.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 kernel/sched/fair.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 28ccf50..dd3fa14 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -4544,6 +4544,9 @@ migrate_task_rq_fair(struct task_struct *p, int next_cpu)
 		atomic_long_add(se->avg.load_avg_contrib,
 						&cfs_rq->removed_load);
 	}
+
+	/* We have migrated, no longer consider this task hot */
+	se->exec_start = 0;
 }
 #endif /* CONFIG_SMP */