On 1 April 2014 12:39, Preeti U Murthy preeti@linux.vnet.ibm.com wrote:

Hi Vincent,

On 03/28/2014 06:52 PM, Vincent Guittot wrote:

...

cpu_power_orig is only changed for SMT system in order to reflect the lower capacity of CPUs. Heterogenous system also have to reflect an original capacity that is different from the default value.

There is no parameter 'cpu_power_orig' till your fourth patch right? Why is this term being used in this patch?

It looks like that i have mixed power_orig and cpu_power_orig in my commit messages

Besides, both parameters power and power_orig are changed for SMT systems to reflect the lower capacity of the CPUs.Why is there a mention of only power_orig?

Only SMT system is able to change power_orig from default value whereas all systems can already change power field with arch_scale_freq_power function. The goal of this patch is to change the function name that is used to set power_orig value to a more generic one and to extend the possibility of setting power_orig for any kind of system. The behavior of the power field is not changed with this patchset.

IMO, the subject of the patch is not clearly reflecting the main intention of the patch. There is nothing done to change the way cpu_power is used; rather you are changing the way the cpu_power is being set to be flexible, thus allowing for the right power value to be set on heterogeneous systems.

'Allow archs to set the cpu_power instead of falling to default value' or something similar would be more appropriate. What do you think?

I can change with : "Allow all archs to set the power_orig"

Thanks

Regards Preeti U Murthy

...

Create a more generic function arch_scale_cpu_power that can be also used by non SMT platform to set cpu_power_orig.

The weak behavior of arch_scale_cpu_power is the previous SMT one in order to keep backward compatibility in the use of cpu_power_orig.

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

kernel/sched/fair.c | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c index 7e9bd0b..ed42061 100644 --- a/kernel/sched/fair.c +++ b/kernel/sched/fair.c @@ -5559,6 +5559,20 @@ unsigned long __weak arch_scale_smt_power(struct sched_domain *sd, int cpu) return default_scale_smt_power(sd, cpu); }

+unsigned long __weak arch_scale_cpu_power(struct sched_domain *sd, int cpu) +{

• unsigned long weight = sd->span_weight;
  

• if ((sd->flags & SD_SHARE_CPUPOWER) && weight > 1) {
  

•         if (sched_feat(ARCH_POWER))
  

•                 return arch_scale_smt_power(sd, cpu);
  

•         else
  

•                 return default_scale_smt_power(sd, cpu);
  

• }
  

• return SCHED_POWER_SCALE;
  

+}

static unsigned long scale_rt_power(int cpu) { struct rq *rq = cpu_rq(cpu); @@ -5590,18 +5604,12 @@ static unsigned long scale_rt_power(int cpu)

static void update_cpu_power(struct sched_domain *sd, int cpu) {

• unsigned long weight = sd->span_weight;
  unsigned long power = SCHED_POWER_SCALE;
  struct sched_group *sdg = sd->groups;
  
  

• if ((sd->flags & SD_SHARE_CPUPOWER) && weight > 1) {
  

•         if (sched_feat(ARCH_POWER))
  

•                 power *= arch_scale_smt_power(sd, cpu);
  

•         else
  

•                 power *= default_scale_smt_power(sd, cpu);
  

• power *= arch_scale_cpu_power(sd, cpu);
  
  

•         power >>= SCHED_POWER_SHIFT;
  

• }
  

• power >>= SCHED_POWER_SHIFT;
  
  sdg->sgp->power_orig = power;
  
  

On Fri, Mar 28, 2014 at 02:22:28PM +0100, Vincent Guittot wrote:

The current sg_capacity solves the ghost cores issue for SMT system and cluster made of big cores which have a cpu_power above SCHED_POWER_SCALE at core level. But it still removes some real cores of a cluster made of LITTLE cores which have a cpu_power below SCHED_POWER_SCALE.

Instead of using the power_orig to detect SMT system and compute a smt factor that will be used to calculate the real number of cores, we set a core_fct field when building the sched_domain topology. We can detect SMT system thanks to SD_SHARE_CPUPOWER flag and set core_fct to know how many CPUs per core we have. The core_fct will ensure that sg_capacity will return cores capacity of a SMT system and will not remove any real core of LITTLE cluster.

This method also fixes a use case where the capacity of a SMT system was overrated. Let take the example of a system made of 8 cores HT system: At CPU level, sg_capacity is cap to a maximum capacity of 8 whereas DIV_ROUND_CLOSEST(power, SCHED_POWER_SCALE) returns 9. ((589*16) / 1024) = 9.3 Now if 2 CPUs (1 core) are fully loaded by rt tasks, sg_capacity still returns a capacity of 8 whereas it should return a capacity of 7. This happen because DIV_ROUND_CLOSEST(power, SCHED_POWER_SCALE) is still above 7.5: ((589*14) / 1024) = 8.05

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

kernel/sched/core.c | 7 +++++++ kernel/sched/fair.c | 6 ++---- kernel/sched/sched.h | 2 +- 3 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/kernel/sched/core.c b/kernel/sched/core.c index f9d9776..5b20b27 100644 --- a/kernel/sched/core.c +++ b/kernel/sched/core.c @@ -5844,6 +5844,13 @@ static void init_sched_groups_power(int cpu, struct sched_domain *sd) WARN_ON(!sg);

• if (!sd->child)

• sg->core_fct = 1;
  

• else if (sd->child->flags & SD_SHARE_CPUPOWER)

• sg->core_fct = cpumask_weight(sched_group_cpus(sg));
  

• else

• sg->core_fct = sd->child->groups->core_fct;
  

• do { sg->group_weight = cpumask_weight(sched_group_cpus(sg)); sg = sg->next;

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c index ed42061..7387c05 100644 --- a/kernel/sched/fair.c +++ b/kernel/sched/fair.c @@ -5773,12 +5773,10 @@ static inline int sg_capacity(struct lb_env *env, struct sched_group *group) power = group->sgp->power; power_orig = group->sgp->power_orig; cpus = group->group_weight;

• smt = group->core_fct;

• /* smt := ceil(cpus / power), assumes: 1 < smt_power < 2 */
• smt = DIV_ROUND_UP(SCHED_POWER_SCALE * cpus, power_orig);
• capacity = cpus / smt; /* cores */

• capacity = DIV_ROUND_CLOSEST(power * cpus, power_orig * smt);

• capacity = min_t(unsigned, capacity, DIV_ROUND_CLOSEST(power, SCHED_POWER_SCALE)); if (!capacity) capacity = fix_small_capacity(env->sd, group);

So this patch only cures a little problem; the much bigger problem is that capacity as exists is completely wrong.

We really should be using utilization here. Until a CPU is fully utilized we shouldn't be moving tasks around (unless packing, but where not there yet, and in that case you want to stop, where this starts, namely full utilization).

So while I appreciate what you're trying to 'fix' here, its really just trying to dress a monkey.