| Message ID | 1396012949-6227-2-git-send-email-vincent.guittot@linaro.org |
|---|---|
| State | New |
| Headers | show |
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? 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? 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? 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; > -- 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 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; >> > -- 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 04/01/2014 04:50 PM, Vincent Guittot wrote: > 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 I am unable to understand this. arch_scale_freq_power() is not doing the same job as arch_scale_smt_power() right? While the former allows the arch to set a power value per cpu different from the default value, the latter allows arch to adjust the power for hyper threads. For example in big.LITTLE, the big cores would be required to have a higher cpu power than what is currently being used as default. This is done by arch_scale_freq_power(). Whereas for an SMT system, the power of a core could perhaps be the same as default value, but needs to be divided among the hyper threads. This is done by arch_scale_smt_power(). They have different purposes right? So I do not understand why you mention "all systems can already change power field with arch_scale_freq_power()." Actually I was assuming this patch would introduce arch_scale_smt_power() and arch_scale_freq_power(), two functions that would *enable all archs and not just ARCH_POWER* as is found today, to adjust their smt power and non-smt power respectively. For the same reasons, I am also unclear as to why power_orig is not initialized *after* default/arch_scale_freq_power(). For big cores for instance you would want the power_orig to be set not to a default value but to what the arch says as the frequency of a core scaled over the default value. > 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" Ok, but for the reasons mentioned above I was thinking the power_orig should be avoided since archs should be able to scale both power and power_orig per thread and not just ARCH_POWER. Regards Preeti U Murthy > > Thanks > >> >> Regards >> Preeti U Murthy -- 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/
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;
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. 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(-)