On 11/06/14 14:52, Jon Medhurst (Tixy) wrote:
On Wed, 2014-06-11 at 14:08 +0100, Chris Redpath wrote:
System services are generally started by init, whilst kernel threads are started by kthreadd. We do not want to give those tasks a head start, as this costs power for very little benefit. We do however wish to do that for tasks which the user launches.
Further, some tasks allocate per-cpu timers directly after launch which can lead to those tasks being always scheduled on a big CPU when there is no computational need to do so. Not promoting services to big CPUs on launch will prevent that unless a service allocates their per-cpu resources after a period of intense computation, which is not a common pattern.
Signed-off-by: Chris Redpath chris.redpath@arm.com
include/linux/sched.h | 4 ++++ kernel/sched/core.c | 6 +++--- kernel/sched/fair.c | 2 +- 3 files changed, 8 insertions(+), 4 deletions(-)
diff --git a/include/linux/sched.h b/include/linux/sched.h index a3c8b27..f862feb 100644 --- a/include/linux/sched.h +++ b/include/linux/sched.h @@ -950,6 +950,10 @@ struct sched_avg { u32 usage_avg_sum; };
+#ifdef CONFIG_SCHED_HMP +#define hmp_task_should_forkboost(task) ((task->parent && task->parent->pid > 2))
I wondered why '2', when init is PID 1. So I ran 'ps' on PC and on ARM board running Android and saw 2 is kthreadd, so '2' is correct, but can't help but think it would be nice to have a comment to that affect on the above #define, so, OK if I add this?
Yes, absolutely fine by me.
/*
- We want to avoid boosting any processes forked from init (PID 1)
- and kthreadd (assumed to be PID 2).
*/
+#endif
- #ifdef CONFIG_SCHEDSTATS struct sched_statistics { u64 wait_start;
diff --git a/kernel/sched/core.c b/kernel/sched/core.c index f583951..c327e57 100644 --- a/kernel/sched/core.c +++ b/kernel/sched/core.c @@ -1635,9 +1635,9 @@ static void __sched_fork(struct task_struct *p) #ifdef CONFIG_SCHED_HMP /* keep LOAD_AVG_MAX in sync with fair.c if load avg series is changed */ #define LOAD_AVG_MAX 47742
- if (p->mm) {
p->se.avg.hmp_last_up_migration = 0;p->se.avg.hmp_last_down_migration = 0;
- p->se.avg.hmp_last_up_migration = 0;
- p->se.avg.hmp_last_down_migration = 0;
The above change means that these values are now set to zero for kernel threads whereas before they were left uninitialised. That seems like the right thing, but wondered if that meant there was a bug before?
I think it should be zero-init anyway but I haven't checked.
- if (hmp_task_should_forkboost(p)) { p->se.avg.load_avg_ratio = 1023; p->se.avg.load_avg_contrib = (1023 * scale_load_down(p->se.load.weight));
diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c index 483dee8..81b5ef9 100644 --- a/kernel/sched/fair.c +++ b/kernel/sched/fair.c @@ -4384,7 +4384,7 @@ select_task_rq_fair(struct task_struct *p, int sd_flag, int wake_flags)
#ifdef CONFIG_SCHED_HMP /* always put non-kernel forking tasks on a big domain */
The above comment is completely true any more, how about we just drop it, as the if statement is now pretty self explanatory?
Agreed.
- if (p->mm && (sd_flag & SD_BALANCE_FORK)) {
- if (unlikely(sd_flag & SD_BALANCE_FORK) && hmp_task_should_forkboost(p)) { new_cpu = hmp_select_faster_cpu(p, prev_cpu); if (new_cpu != NR_CPUS) { hmp_next_up_delay(&p->se, new_cpu);
--Chris