On Fri, Feb 27, 2015 at 04:54:07PM +0100, Vincent Guittot wrote:

• unsigned long scale_freq = arch_scale_freq_capacity(NULL, cpu);

• 	sa->running_avg_sum += delta_w * scale_freq
  

• 		>> SCHED_CAPACITY_SHIFT;
  

so the only thing that could be improved is somehow making this multiplication go away when the arch doesn't implement the function.

But I'm not sure how to do that without #ifdef.

Maybe a little something like so then... that should make the compiler get rid of those multiplications unless the arch needs them.

--- a/kernel/sched/fair.c +++ b/kernel/sched/fair.c @@ -2484,8 +2484,6 @@ static u32 __compute_runnable_contrib(u6 return contrib + runnable_avg_yN_sum[n]; }

-unsigned long __weak arch_scale_freq_capacity(struct sched_domain *sd, int cpu); - /* * We can represent the historical contribution to runnable average as the * coefficients of a geometric series. To do this we sub-divide our runnable @@ -6015,11 +6013,6 @@ static unsigned long default_scale_capac return SCHED_CAPACITY_SCALE; }

-unsigned long __weak arch_scale_freq_capacity(struct sched_domain *sd, int cpu) -{ - return default_scale_capacity(sd, cpu); -} - static unsigned long default_scale_cpu_capacity(struct sched_domain *sd, int cpu) { if ((sd->flags & SD_SHARE_CPUCAPACITY) && (sd->span_weight > 1)) --- a/kernel/sched/sched.h +++ b/kernel/sched/sched.h @@ -1387,7 +1387,14 @@ static inline int hrtick_enabled(struct

#ifdef CONFIG_SMP extern void sched_avg_update(struct rq *rq); -extern unsigned long arch_scale_freq_capacity(struct sched_domain *sd, int cpu); + +#ifndef arch_scale_freq_capacity +static __always_inline +unsigned long arch_scale_freq_capacity(struct sched_domain *sd, int cpu) +{ + return SCHED_CAPACITY_SCALE; +} +#endif

static inline void sched_rt_avg_update(struct rq *rq, u64 rt_delta) {

The arch_scale_{freq, cpu}_capacity() functions currently return a scaling factor that need to be multiplied and shifted by the caller. The default weak functions don't result in any scaling by the the multiplication and shift is still done. By moving the multiplication and shift into the arch_scale*() functions instead, the weak implementation can just return the input value and avoid the unnecessary multiplication and shift.

While we are at it, we can remove the sched_domain parameter by moving the SD_SHARE_CPUCAPACITY outside the weak arch_scale_cpu_capacity() function.

cc: Ingo Molnar mingo@redhat.com cc: Peter Zijlstra peterz@infradead.org

Signed-off-by: Morten Rasmussen morten.rasmussen@arm.com --- arch/arm/kernel/topology.c | 6 +++--- kernel/sched/fair.c | 34 ++++++++++++++-------------------- 2 files changed, 17 insertions(+), 23 deletions(-)

diff --git a/arch/arm/kernel/topology.c b/arch/arm/kernel/topology.c index 08b7847..5328f79 100644 --- a/arch/arm/kernel/topology.c +++ b/arch/arm/kernel/topology.c @@ -42,9 +42,9 @@ */ static DEFINE_PER_CPU(unsigned long, cpu_scale);

-unsigned long arch_scale_cpu_capacity(struct sched_domain *sd, int cpu) +unsigned long arch_scale_cpu_capacity(int cpu, unsigned long factor) { - return per_cpu(cpu_scale, cpu); + return (factor * per_cpu(cpu_scale, cpu)) >> SCHED_CAPACITY_SHIFT; }

static void set_capacity_scale(unsigned int cpu, unsigned long capacity) @@ -166,7 +166,7 @@ static void update_cpu_capacity(unsigned int cpu) set_capacity_scale(cpu, cpu_capacity(cpu) / middle_capacity);

pr_info("CPU%u: update cpu_capacity %lu\n", - cpu, arch_scale_cpu_capacity(NULL, cpu)); + cpu, arch_scale_cpu_capacity(cpu, SCHED_CAPACITY_SCALE)); }

#else diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c index 5080c0d..60c3172 100644 --- a/kernel/sched/fair.c +++ b/kernel/sched/fair.c @@ -5958,27 +5958,19 @@ static inline int get_sd_load_idx(struct sched_domain *sd, return load_idx; }

-static unsigned long default_scale_capacity(struct sched_domain *sd, int cpu) +static unsigned long default_scale_capacity(int cpu, unsigned long factor) { - return SCHED_CAPACITY_SCALE; -} - -unsigned long __weak arch_scale_freq_capacity(struct sched_domain *sd, int cpu) -{ - return default_scale_capacity(sd, cpu); + return factor; }

-static unsigned long default_scale_cpu_capacity(struct sched_domain *sd, int cpu) +unsigned long __weak arch_scale_freq_capacity(int cpu, unsigned long factor) { - if ((sd->flags & SD_SHARE_CPUCAPACITY) && (sd->span_weight > 1)) - return sd->smt_gain / sd->span_weight; - - return SCHED_CAPACITY_SCALE; + return default_scale_capacity(cpu, factor); }

-unsigned long __weak arch_scale_cpu_capacity(struct sched_domain *sd, int cpu) +unsigned long __weak arch_scale_cpu_capacity(int cpu, unsigned long factor) { - return default_scale_cpu_capacity(sd, cpu); + return default_scale_capacity(cpu, factor); }

static unsigned long scale_rt_capacity(int cpu) @@ -6020,12 +6012,14 @@ static void update_cpu_capacity(struct sched_domain *sd, int cpu) unsigned long capacity = SCHED_CAPACITY_SCALE; struct sched_group *sdg = sd->groups;

- if (sched_feat(ARCH_CAPACITY)) - capacity *= arch_scale_cpu_capacity(sd, cpu); - else - capacity *= default_scale_cpu_capacity(sd, cpu); - - capacity >>= SCHED_CAPACITY_SHIFT; + if (sched_feat(ARCH_CAPACITY)) { + capacity = arch_scale_cpu_capacity(cpu, capacity); + } else { + if ((sd->flags & SD_SHARE_CPUCAPACITY) && (sd->span_weight > 1)) + capacity = sd->smt_gain / sd->span_weight; + else + capacity = default_scale_capacity(cpu, capacity); + }

sdg->sgc->capacity_orig = capacity;

On Wed, Mar 25, 2015 at 05:33:09PM +0000, Peter Zijlstra wrote:

On Tue, Mar 24, 2015 at 11:00:57AM +0100, Vincent Guittot wrote:

...

On 23 March 2015 at 14:19, Peter Zijlstra peterz@infradead.org wrote:

...

On Fri, Feb 27, 2015 at 04:54:07PM +0100, Vincent Guittot wrote:

...

• unsigned long scale_freq = arch_scale_freq_capacity(NULL, cpu);
  

...

•                 sa->running_avg_sum += delta_w * scale_freq
  

•                         >> SCHED_CAPACITY_SHIFT;
  

so the only thing that could be improved is somehow making this multiplication go away when the arch doesn't implement the function.

But I'm not sure how to do that without #ifdef.

Maybe a little something like so then... that should make the compiler get rid of those multiplications unless the arch needs them.

yes, it removes useless multiplication when not used by an arch. It also adds a constraint on the arch side which have to define arch_scale_freq_capacity like below:

#define arch_scale_freq_capacity xxx_arch_scale_freq_capacity with xxx_arch_scale_freq_capacity an architecture specific function

Yeah, but it not being weak should make that a compile time warn/fail, which should be pretty easy to deal with.

Could you enlighten me a bit about how to define the arch specific implementation without getting into trouble? I'm failing miserably :(

I thought the arm arch-specific topology.h file was a good place to put the define as it get included in sched.h, so I did a:

#define arch_scale_freq_capacity arm_arch_scale_freq_capacity

However, I have to put a function prototype in the same (or some other included) header file to avoid doing an implicit function definition. arch_scale_freq_capacity() takes a struct sched_domain pointer, so I have to include linux/sched.h which leads to circular dependency between linux/sched.h and topology.h.

The only way out I can think of to create (or find) a new arch-specific header file that can include linux/sched.h and be included in kernel/sched/sched.h and have the define and prototype in there.

I must be missing something?

We can drop the sched_domain pointer as we don't use it, but I'm going to do the same trick for arch_scale_cpu_capacity() as well which does require the sd pointer.

Finally, is introducing an ARCH_HAS_SCALE_FREQ_CAPACITY or similar a complete no go?

diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h index 91c6736..5707fb7 100644 --- a/kernel/sched/sched.h +++ b/kernel/sched/sched.h @@ -1388,12 +1388,14 @@ static inline int hrtick_enabled(struct rq *rq) #ifdef CONFIG_SMP extern void sched_avg_update(struct rq *rq);

-#ifndef arch_scale_freq_capacity +#ifndef ARCH_HAS_SCALE_FREQ_CAPACITY static __always_inline unsigned long arch_scale_freq_capacity(struct sched_domain *sd, int cpu) { return SCHED_CAPACITY_SCALE; } +#else +extern unsigned long arch_scale_freq_capacity(struct sched_domain *sd, int cpu); #endif

static inline void sched_rt_avg_update(struct rq *rq, u64 rt_delta)

On 27/02/15 15:54, Vincent Guittot wrote:

Monitor the usage level of each group of each sched_domain level. The usage is the portion of cpu_capacity_orig that is currently used on a CPU or group of CPUs. We use the utilization_load_avg to evaluate the usage level of each group.

The utilization_load_avg only takes into account the running time of the CFS tasks on a CPU with a maximum value of SCHED_LOAD_SCALE when the CPU is fully utilized. Nevertheless, we must cap utilization_load_avg which can be temporaly

s/temporaly/temporally

greater than SCHED_LOAD_SCALE after the migration of a task on this CPU and until the metrics are stabilized.

The utilization_load_avg is in the range [0..SCHED_LOAD_SCALE] to reflect the running load on the CPU whereas the available capacity for the CFS task is in the range [0..cpu_capacity_orig]. In order to test if a CPU is fully utilized by CFS tasks, we have to scale the utilization in the cpu_capacity_orig range of the CPU to get the usage of the latter. The usage can then be compared with the available capacity (ie cpu_capacity) to deduct the usage level of a CPU.

The frequency scaling invariance of the usage is not taken into account in this patch, it will be solved in another patch which will deal with frequency scaling invariance on the running_load_avg.

The use of underscores in running_load_avg implies to me that this is a data member of struct sched_avg or something similar. But there is no running_load_avg in the current code. However, I can see that sched_avg::*running_avg_sum* (and therefore cfs_rq::*utilization_load_avg*) are frequency scale invariant.

Signed-off-by: Vincent Guittot vincent.guittot@linaro.org Acked-by: Morten Rasmussen morten.rasmussen@arm.com

---

kernel/sched/fair.c | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c index 10f84c3..faf61a2 100644 --- a/kernel/sched/fair.c +++ b/kernel/sched/fair.c @@ -4781,6 +4781,33 @@ static int select_idle_sibling(struct task_struct *p, int target) done: return target; } +/*

• • get_cpu_usage returns the amount of capacity of a CPU that is used by CFS
• • tasks. The unit of the return value must capacity so we can compare the

s/must capacity/must be the one of capacity

• • usage with the capacity of the CPU that is available for CFS task (ie
• • cpu_capacity).
• • cfs.utilization_load_avg is the sum of running time of runnable tasks on a
• • CPU. It represents the amount of utilization of a CPU in the range
• • [0..SCHED_LOAD_SCALE]. The usage of a CPU can't be higher than the full
• • capacity of the CPU because it's about the running time on this CPU.
• • Nevertheless, cfs.utilization_load_avg can be higher than SCHED_LOAD_SCALE
• • because of unfortunate rounding in avg_period and running_load_avg or just
• • after migrating tasks until the average stabilizes with the new running
• • time. So we need to check that the usage stays into the range
• • [0..cpu_capacity_orig] and cap if necessary.
• • Without capping the usage, a group could be seen as overloaded (CPU0 usage
• • at 121% + CPU1 usage at 80%) whereas CPU1 has 20% of available capacity/

s/capacity//capacity.

[...]

-- Dietmar

Monitor the usage level of each group of each sched_domain level. The usage is the portion of cpu_capacity_orig that is currently used on a CPU or group of CPUs. We use the utilization_load_avg to evaluate the usage level of each group.

The utilization_load_avg only takes into account the running time of the CFS tasks on a CPU with a maximum value of SCHED_LOAD_SCALE when the CPU is fully utilized. Nevertheless, we must cap utilization_load_avg which can be temporally greater than SCHED_LOAD_SCALE after the migration of a task on this CPU and until the metrics are stabilized.

The utilization_load_avg is in the range [0..SCHED_LOAD_SCALE] to reflect the running load on the CPU whereas the available capacity for the CFS task is in the range [0..cpu_capacity_orig]. In order to test if a CPU is fully utilized by CFS tasks, we have to scale the utilization in the cpu_capacity_orig range of the CPU to get the usage of the latter. The usage can then be compared with the available capacity (ie cpu_capacity) to deduct the usage level of a CPU.

The frequency scaling invariance of the usage is not taken into account in this patch, it will be solved in another patch which will deal with frequency scaling invariance on the utilization_load_avg.

Signed-off-by: Vincent Guittot vincent.guittot@linaro.org Acked-by: Morten Rasmussen morten.rasmussen@arm.com --- kernel/sched/fair.c | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c index 10f84c3..faf61a2 100644 --- a/kernel/sched/fair.c +++ b/kernel/sched/fair.c @@ -4781,6 +4781,33 @@ static int select_idle_sibling(struct task_struct *p, int target) done: return target; } +/* + * get_cpu_usage returns the amount of capacity of a CPU that is used by CFS + * tasks. The unit of the return value must be the one of capacity so we can + * compare the usage with the capacity of the CPU that is available for CFS + * task (ie cpu_capacity). + * cfs.utilization_load_avg is the sum of running time of runnable tasks on a + * CPU. It represents the amount of utilization of a CPU in the range + * [0..SCHED_LOAD_SCALE]. The usage of a CPU can't be higher than the full + * capacity of the CPU because it's about the running time on this CPU. + * Nevertheless, cfs.utilization_load_avg can be higher than SCHED_LOAD_SCALE + * because of unfortunate rounding in avg_period and running_load_avg or just + * after migrating tasks until the average stabilizes with the new running + * time. So we need to check that the usage stays into the range + * [0..cpu_capacity_orig] and cap if necessary. + * Without capping the usage, a group could be seen as overloaded (CPU0 usage + * at 121% + CPU1 usage at 80%) whereas CPU1 has 20% of available capacity + */ +static int get_cpu_usage(int cpu) +{ + unsigned long usage = cpu_rq(cpu)->cfs.utilization_load_avg; + unsigned long capacity = capacity_orig_of(cpu); + + if (usage >= SCHED_LOAD_SCALE) + return capacity; + + return (usage * capacity) >> SCHED_LOAD_SHIFT; +}

/* * select_task_rq_fair: Select target runqueue for the waking task in domains @@ -5907,6 +5934,7 @@ struct sg_lb_stats { unsigned long sum_weighted_load; /* Weighted load of group's tasks */ unsigned long load_per_task; unsigned long group_capacity; + unsigned long group_usage; /* Total usage of the group */ unsigned int sum_nr_running; /* Nr tasks running in the group */ unsigned int group_capacity_factor; unsigned int idle_cpus; @@ -6255,6 +6283,7 @@ static inline void update_sg_lb_stats(struct lb_env *env, load = source_load(i, load_idx);

sgs->group_load += load; + sgs->group_usage += get_cpu_usage(i); sgs->sum_nr_running += rq->cfs.h_nr_running;

if (rq->nr_running > 1)

On 27 March 2015 at 15:52, Xunlei Pang pang.xunlei@linaro.org wrote:

Hi Vincent,

On 27 February 2015 at 23:54, Vincent Guittot vincent.guittot@linaro.org wrote:

...

/** @@ -6432,18 +6435,19 @@ static inline void update_sd_lb_stats(struct lb_env *env, struct sd_lb_stats *sd

            /*
             * In case the child domain prefers tasks go to siblings

•            * first, lower the sg capacity factor to one so that we'll try
  

•            * first, lower the sg capacity so that we'll try
             * and move all the excess tasks away. We lower the capacity
             * of a group only if the local group has the capacity to fit
  

•            * these excess tasks, i.e. nr_running < group_capacity_factor. The
  

•            * extra check prevents the case where you always pull from the
  

•            * heaviest group when it is already under-utilized (possible
  

•            * with a large weight task outweighs the tasks on the system).
  

•            * these excess tasks. The extra check prevents the case where
  

•            * you always pull from the heaviest group when it is already
  

•            * under-utilized (possible with a large weight task outweighs
  

•            * the tasks on the system).
             */
            if (prefer_sibling && sds->local &&
  

•               sds->local_stat.group_has_free_capacity) {
  

•                   sgs->group_capacity_factor = min(sgs->group_capacity_factor, 1U);
  

•                   sgs->group_type = group_classify(sg, sgs);
  

•               group_has_capacity(env, &sds->local_stat) &&
  

•               (sgs->sum_nr_running > 1)) {
  

•                   sgs->group_no_capacity = 1;
  

•                   sgs->group_type = group_overloaded;
            }
  
  

For SD_PREFER_SIBLING, if local has 1 task and group_has_capacity() returns true(but not overloaded) for it, and assume sgs group has 2 tasks, should we still mark this group overloaded?

yes, the load balance will then choose if it's worth pulling it or not depending of the load of each groups

-Xunlei

On 1 April 2015 at 05:37, Xunlei Pang pang.xunlei@linaro.org wrote:

Hi Vincent,

On 27 March 2015 at 23:59, Vincent Guittot vincent.guittot@linaro.org wrote:

...

On 27 March 2015 at 15:52, Xunlei Pang pang.xunlei@linaro.org wrote:

...

Hi Vincent,

On 27 February 2015 at 23:54, Vincent Guittot vincent.guittot@linaro.org wrote:

...

/** @@ -6432,18 +6435,19 @@ static inline void update_sd_lb_stats(struct lb_env *env, struct sd_lb_stats *sd

            /*
             * In case the child domain prefers tasks go to siblings

•            * first, lower the sg capacity factor to one so that we'll try
  

•            * first, lower the sg capacity so that we'll try
             * and move all the excess tasks away. We lower the capacity
             * of a group only if the local group has the capacity to fit
  

•            * these excess tasks, i.e. nr_running < group_capacity_factor. The
  

•            * extra check prevents the case where you always pull from the
  

•            * heaviest group when it is already under-utilized (possible
  

•            * with a large weight task outweighs the tasks on the system).
  

•            * these excess tasks. The extra check prevents the case where
  

•            * you always pull from the heaviest group when it is already
  

•            * under-utilized (possible with a large weight task outweighs
  

•            * the tasks on the system).
             */
            if (prefer_sibling && sds->local &&
  

•               sds->local_stat.group_has_free_capacity) {
  

•                   sgs->group_capacity_factor = min(sgs->group_capacity_factor, 1U);
  

•                   sgs->group_type = group_classify(sg, sgs);
  

•               group_has_capacity(env, &sds->local_stat) &&
  

•               (sgs->sum_nr_running > 1)) {
  

•                   sgs->group_no_capacity = 1;
  

•                   sgs->group_type = group_overloaded;
            }
  
  

For SD_PREFER_SIBLING, if local has 1 task and group_has_capacity() returns true(but not overloaded) for it, and assume sgs group has 2 tasks, should we still mark this group overloaded?

yes, the load balance will then choose if it's worth pulling it or not depending of the load of each groups

Maybe I didn't make it clearly. For example, CPU0~1 are SMT siblings, CPU2~CPU3 are another pair. CPU0 is idle, others each has 1 task. Then according to this patch, CPU2~CPU3(as one group) will be viewed as overloaded(CPU0~CPU1 as local group, and group_has_capacity() returns true here), so the balancer may initiate an active task moving. This is different from the current code as SD_PREFER_SIBLING logic does. Is this problematic?

IMHO, it's not problematic, It's worth triggering a load balance if there is an imbalance between the 2 groups (as an example CPU0~1 has one low nice prio task but CPU1~2 have 2 high nice prio tasks) so the decision will be done when calculating the imbalance

Vincent

...

...

-Xunlei

On 1 April 2015 at 16:54, Xunlei Pang pang.xunlei@linaro.org wrote:

Hi Vincent,

On 1 April 2015 at 17:06, Vincent Guittot vincent.guittot@linaro.org wrote:

...

On 1 April 2015 at 05:37, Xunlei Pang pang.xunlei@linaro.org wrote:

...

Hi Vincent,

On 27 March 2015 at 23:59, Vincent Guittot vincent.guittot@linaro.org wrote:

...

On 27 March 2015 at 15:52, Xunlei Pang pang.xunlei@linaro.org wrote:

...

Hi Vincent,

On 27 February 2015 at 23:54, Vincent Guittot vincent.guittot@linaro.org wrote:

...

/** @@ -6432,18 +6435,19 @@ static inline void update_sd_lb_stats(struct lb_env *env, struct sd_lb_stats *sd

            /*
             * In case the child domain prefers tasks go to siblings

•            * first, lower the sg capacity factor to one so that we'll try
  

•            * first, lower the sg capacity so that we'll try
             * and move all the excess tasks away. We lower the capacity
             * of a group only if the local group has the capacity to fit
  

•            * these excess tasks, i.e. nr_running < group_capacity_factor. The
  

•            * extra check prevents the case where you always pull from the
  

•            * heaviest group when it is already under-utilized (possible
  

•            * with a large weight task outweighs the tasks on the system).
  

•            * these excess tasks. The extra check prevents the case where
  

•            * you always pull from the heaviest group when it is already
  

•            * under-utilized (possible with a large weight task outweighs
  

•            * the tasks on the system).
             */
            if (prefer_sibling && sds->local &&
  

•               sds->local_stat.group_has_free_capacity) {
  

•                   sgs->group_capacity_factor = min(sgs->group_capacity_factor, 1U);
  

•                   sgs->group_type = group_classify(sg, sgs);
  

•               group_has_capacity(env, &sds->local_stat) &&
  

•               (sgs->sum_nr_running > 1)) {
  

•                   sgs->group_no_capacity = 1;
  

•                   sgs->group_type = group_overloaded;
            }
  
  

For SD_PREFER_SIBLING, if local has 1 task and group_has_capacity() returns true(but not overloaded) for it, and assume sgs group has 2 tasks, should we still mark this group overloaded?

yes, the load balance will then choose if it's worth pulling it or not depending of the load of each groups

Maybe I didn't make it clearly. For example, CPU0~1 are SMT siblings, CPU2~CPU3 are another pair. CPU0 is idle, others each has 1 task. Then according to this patch, CPU2~CPU3(as one group) will be viewed as overloaded(CPU0~CPU1 as local group, and group_has_capacity() returns true here), so the balancer may initiate an active task moving. This is different from the current code as SD_PREFER_SIBLING logic does. Is this problematic?

IMHO, it's not problematic, It's worth triggering a load balance if there is an imbalance between the 2 groups (as an example CPU0~1 has one low nice prio task but CPU1~2 have 2 high nice prio tasks) so the decision will be done when calculating the imbalance

Yes, but assuming the balancer calculated some imbalance, after moving like CPU0~CPU1 have 1 low prio task and 1 high prio task, CPU2~CPU3 have 1 high piro task, seems it does no good because there's only 1 task per CPU after all.

In this condition i agree that it doesn't worth to move a task and the scheduler should not because the imbalance will be too small to found a busiest queue. So the decision is more linked to the weighted load of the tasks than to the number of tasks

So, is code below better( we may have more than 2 SMT siblings, like Broadcom XLP processor having 4 SMT per core)? if (prefer_sibling && sds->local && group_has_capacity(env, &sds->local_stat) && (sgs->sum_nr_running > sds->local_stat.sum_nr_running + 1)) { sgs->group_no_capacity = 1; sgs->group_type = group_overloaded; }

I would say no because it mainly depends of the weighted load of the tasks so calculate_imbalance is the right place

Vincent

Thanks, -Xunlei

capacity_orig field is no more used in the scheduler so we can remove it from struct sched_group_capacity

Signed-off-by: Vincent Guittot vincent.guittot@linaro.org --- kernel/sched/core.c | 12 ------------ kernel/sched/fair.c | 13 +++---------- kernel/sched/sched.h | 2 +- 3 files changed, 4 insertions(+), 23 deletions(-)

diff --git a/kernel/sched/core.c b/kernel/sched/core.c index 28e3ec2..29f7037 100644 --- a/kernel/sched/core.c +++ b/kernel/sched/core.c @@ -5446,17 +5446,6 @@ static int sched_domain_debug_one(struct sched_domain *sd, int cpu, int level, break; }

- /* - * Even though we initialize ->capacity to something semi-sane, - * we leave capacity_orig unset. This allows us to detect if - * domain iteration is still funny without causing /0 traps. - */ - if (!group->sgc->capacity_orig) { - printk(KERN_CONT "\n"); - printk(KERN_ERR "ERROR: domain->cpu_capacity not set\n"); - break; - } - if (!cpumask_weight(sched_group_cpus(group))) { printk(KERN_CONT "\n"); printk(KERN_ERR "ERROR: empty group\n"); @@ -5941,7 +5930,6 @@ build_overlap_sched_groups(struct sched_domain *sd, int cpu) * die on a /0 trap. */ sg->sgc->capacity = SCHED_CAPACITY_SCALE * cpumask_weight(sg_span); - sg->sgc->capacity_orig = sg->sgc->capacity;

/* * Make sure the first group of this domain contains the diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c index 9d7431f..7420d21 100644 --- a/kernel/sched/fair.c +++ b/kernel/sched/fair.c @@ -6073,7 +6073,6 @@ static void update_cpu_capacity(struct sched_domain *sd, int cpu) capacity >>= SCHED_CAPACITY_SHIFT;

cpu_rq(cpu)->cpu_capacity_orig = capacity; - sdg->sgc->capacity_orig = capacity;

capacity *= scale_rt_capacity(cpu); capacity >>= SCHED_CAPACITY_SHIFT; @@ -6089,7 +6088,7 @@ void update_group_capacity(struct sched_domain *sd, int cpu) { struct sched_domain *child = sd->child; struct sched_group *group, *sdg = sd->groups; - unsigned long capacity, capacity_orig; + unsigned long capacity; unsigned long interval;

interval = msecs_to_jiffies(sd->balance_interval); @@ -6101,7 +6100,7 @@ void update_group_capacity(struct sched_domain *sd, int cpu) return; }

- capacity_orig = capacity = 0; + capacity = 0;

if (child->flags & SD_OVERLAP) { /* @@ -6121,19 +6120,15 @@ void update_group_capacity(struct sched_domain *sd, int cpu) * Use capacity_of(), which is set irrespective of domains * in update_cpu_capacity(). * - * This avoids capacity/capacity_orig from being 0 and + * This avoids capacity from being 0 and * causing divide-by-zero issues on boot. - * - * Runtime updates will correct capacity_orig. */ if (unlikely(!rq->sd)) { - capacity_orig += capacity_orig_of(cpu); capacity += capacity_of(cpu); continue; }

sgc = rq->sd->groups->sgc; - capacity_orig += sgc->capacity_orig; capacity += sgc->capacity; } } else { @@ -6144,13 +6139,11 @@ void update_group_capacity(struct sched_domain *sd, int cpu)

group = child->groups; do { - capacity_orig += group->sgc->capacity_orig; capacity += group->sgc->capacity; group = group->next; } while (group != child->groups); }

- sdg->sgc->capacity_orig = capacity_orig; sdg->sgc->capacity = capacity; }

diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h index 9f06d24..24c4aaf 100644 --- a/kernel/sched/sched.h +++ b/kernel/sched/sched.h @@ -814,7 +814,7 @@ struct sched_group_capacity { * CPU capacity of this group, SCHED_LOAD_SCALE being max capacity * for a single CPU. */ - unsigned int capacity, capacity_orig; + unsigned int capacity; unsigned long next_update; int imbalance; /* XXX unrelated to capacity but shared group state */ /*

* Vincent Guittot vincent.guittot@linaro.org [2015-02-27 16:54:13]:

Add the SD_PREFER_SIBLING flag for SMT level in order to ensure that the scheduler will put at least 1 task per core.

Signed-off-by: Vincent Guittot vincent.guittot@linaro.org Reviewed-by: Preeti U. Murthy preeti@linux.vnet.ibm.com

---

kernel/sched/core.c | 1 + 1 file changed, 1 insertion(+)

diff --git a/kernel/sched/core.c b/kernel/sched/core.c index 29f7037..753f0a2 100644 --- a/kernel/sched/core.c +++ b/kernel/sched/core.c @@ -6240,6 +6240,7 @@ sd_init(struct sched_domain_topology_level *tl, int cpu) */ if (sd->flags & SD_SHARE_CPUCAPACITY) {

• sd->flags |= SD_PREFER_SIBLING;
  

  sd->imbalance_pct = 110; sd->smt_gain = 1178; /* ~15% */

Prefer siblings logic dates back to https://lkml.org/lkml/2009/8/27/210 and only used in update_sd_lb_stats() where we have

if (child && child->flags & SD_PREFER_SIBLING) prefer_sibling = 1;

However what confuses me is why should we even look at a child domain's flag to balance tasks across the current sched domain? Why cant we just set and use a sd flag at current level than to look at child domain flag?

-- 1.9.1

-- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/

On 26 March 2015 at 15:19, Dietmar Eggemann dietmar.eggemann@arm.com wrote:

On 27/02/15 15:54, Vincent Guittot wrote:

...

When a CPU is used to handle a lot of IRQs or some RT tasks, the remaining capacity for CFS tasks can be significantly reduced. Once we detect such situation by comparing cpu_capacity_orig and cpu_capacity, we trig an idle load balance to check if it's worth moving its tasks on an idle CPU. It's worth trying to move the task before the CPU is fully utilized to minimize the preemption by irq or RT tasks.

Once the idle load_balance has selected the busiest CPU, it will look for an active load balance for only two cases :

• there is only 1 task on the busiest CPU.
• we haven't been able to move a task of the busiest rq.

A CPU with a reduced capacity is included in the 1st case, and it's worth to actively migrate its task if the idle CPU has got more available capacity for CFS tasks. This test has been added in need_active_balance.

As a sidenote, this will not generate more spurious ilb because we already trig an ilb if there is more than 1 busy cpu. If this cpu is the only one that has a task, we will trig the ilb once for migrating the task.

The nohz_kick_needed function has been cleaned up a bit while adding the new test

env.src_cpu and env.src_rq must be set unconditionnally because they are used in need_active_balance which is called even if busiest->nr_running equals 1

Signed-off-by: Vincent Guittot vincent.guittot@linaro.org

kernel/sched/fair.c | 69 ++++++++++++++++++++++++++++++++++++----------------- 1 file changed, 47 insertions(+), 22 deletions(-)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c index 7420d21..e70c315 100644 --- a/kernel/sched/fair.c +++ b/kernel/sched/fair.c @@ -6855,6 +6855,19 @@ static int need_active_balance(struct lb_env *env) return 1; }

•   /*
  

•    * The dst_cpu is idle and the src_cpu CPU has only 1 CFS task.
  

•    * It's worth migrating the task if the src_cpu's capacity is
  

reduced

•    * because of other sched_class or IRQs if more capacity stays
  

•    * available on dst_cpu.
  

•    */
  

•   if ((env->idle != CPU_NOT_IDLE) &&
  

•       (env->src_rq->cfs.h_nr_running == 1)) {
  

•           if ((check_cpu_capacity(env->src_rq, sd)) &&
  

•               (capacity_of(env->src_cpu)*sd->imbalance_pct <
  

capacity_of(env->dst_cpu)*100))

•                   return 1;
  

•   }
  

•   return unlikely(sd->nr_balance_failed > sd->cache_nice_tries+2);
  

  }

@@ -6954,6 +6967,9 @@ static int load_balance(int this_cpu, struct rq *this_rq,

    schedstat_add(sd, lb_imbalance[idle], env.imbalance);

•   env.src_cpu = busiest->cpu;
  

Isn't this 'env.src_cpu = busiest->cpu;' or 'env.src_cpu = cpu_of(busiest);' already needed due to the existing ASYM_PACKING check in need_active_balance() 'if ( ... && env->src_cpu > env->dst_cpu)' for CPU_NEWLY_IDLE? Otherwise like you said, in these 'busiest->nr_running equals 1' instances, env->src_cpu is un-initialized.

yes, i sent a fix for that purpose some times ago : https://lkml.org/lkml/2013/2/12/158 but it has not gone further than the mailing list.

AFAICT, SD_ASYM_PACKING can't trig an active load balance on the cpu with the lowest id without this fix as src_cpu is initialized to 0 which implies that 'env->src_cpu > env->dst_cpu' is always false

Vincent

...

•   env.src_rq = busiest;
  

•   ld_moved = 0;
    if (busiest->nr_running > 1) {
            /*
  

@@ -6963,8 +6979,6 @@ static int load_balance(int this_cpu, struct rq *this_rq, * correctly treated as an imbalance. */ env.flags |= LBF_ALL_PINNED;

•           env.src_cpu   = busiest->cpu;
  

•           env.src_rq    = busiest;
            env.loop_max  = min(sysctl_sched_nr_migrate,
  

busiest->nr_running);

more_balance:

[...]