Re: [PATCH 2/6] sched/fair: Convert arch_scale_cpu_capacity() from weak function to #define

[Vincent Guittot]

On 2 September 2015 at 11:31, Vincent Guittot
<vincent.guittot@xxxxxxxxxx> wrote:
> Hi Morten,
>
> On 14 August 2015 at 18:23, Morten Rasmussen <morten.rasmussen@xxxxxxx> wrote:
>> Bring arch_scale_cpu_capacity() in line with the recent change of its
>> arch_scale_freq_capacity() sibling in commit dfbca41f3479 ("sched:
>> Optimize freq invariant accounting") from weak function to #define to
>> allow inlining of the function.
>>
>> While at it, remove the ARCH_CAPACITY sched_feature as well. With the
>> change to #define there isn't a straightforward way to allow runtime
>> switch between an arch implementation and the default implementation of
>> arch_scale_cpu_capacity() using sched_feature. The default was to use
>> the arch-specific implementation, but only the arm architecture provides
>> one and that is essentially equivalent to the default implementation.
>>
>> cc: Ingo Molnar <mingo@xxxxxxxxxx>
>> cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
>>
>> Signed-off-by: Morten Rasmussen <morten.rasmussen@xxxxxxx>
>> ---
>>  kernel/sched/fair.c     | 22 +---------------------
>>  kernel/sched/features.h |  5 -----
>>  kernel/sched/sched.h    | 11 +++++++++++
>>  3 files changed, 12 insertions(+), 26 deletions(-)
>>
>> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
>> index 1626410..c72223a 100644
>> --- a/kernel/sched/fair.c
>> +++ b/kernel/sched/fair.c
>> @@ -6016,19 +6016,6 @@ static inline int get_sd_load_idx(struct sched_domain *sd,
>>         return load_idx;
>>  }
>>
>> -static unsigned long default_scale_cpu_capacity(struct sched_domain *sd, int cpu)
>> -{
>> -       if ((sd->flags & SD_SHARE_CPUCAPACITY) && (sd->span_weight > 1))
>> -               return sd->smt_gain / sd->span_weight;
>> -
>> -       return SCHED_CAPACITY_SCALE;
>> -}
>> -
>> -unsigned long __weak arch_scale_cpu_capacity(struct sched_domain *sd, int cpu)
>> -{
>> -       return default_scale_cpu_capacity(sd, cpu);
>> -}
>> -
>>  static unsigned long scale_rt_capacity(int cpu)
>>  {
>>         struct rq *rq = cpu_rq(cpu);
>> @@ -6058,16 +6045,9 @@ static unsigned long scale_rt_capacity(int cpu)
>>
>>  static void update_cpu_capacity(struct sched_domain *sd, int cpu)
>>  {
>> -       unsigned long capacity = SCHED_CAPACITY_SCALE;
>> +       unsigned long capacity = arch_scale_cpu_capacity(sd, cpu);
>>         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;
>> -
>>         cpu_rq(cpu)->cpu_capacity_orig = capacity;
>>
>>         capacity *= scale_rt_capacity(cpu);
>> diff --git a/kernel/sched/features.h b/kernel/sched/features.h
>> index 83a50e7..6565eac 100644
>> --- a/kernel/sched/features.h
>> +++ b/kernel/sched/features.h
>> @@ -36,11 +36,6 @@ SCHED_FEAT(CACHE_HOT_BUDDY, true)
>>   */
>>  SCHED_FEAT(WAKEUP_PREEMPTION, true)
>>
>> -/*
>> - * Use arch dependent cpu capacity functions
>> - */
>> -SCHED_FEAT(ARCH_CAPACITY, true)
>> -
>>  SCHED_FEAT(HRTICK, false)
>>  SCHED_FEAT(DOUBLE_TICK, false)
>>  SCHED_FEAT(LB_BIAS, true)
>> diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
>> index 22ccc55..7e6f250 100644
>> --- a/kernel/sched/sched.h
>> +++ b/kernel/sched/sched.h
>> @@ -1402,6 +1402,17 @@ unsigned long arch_scale_freq_capacity(struct sched_domain *sd, int cpu)
>>  }
>>  #endif
>>
>> +#ifndef arch_scale_cpu_capacity
>> +static __always_inline
>> +unsigned long arch_scale_cpu_capacity(struct sched_domain *sd, int cpu)
>> +{
>> +       if ((sd->flags & SD_SHARE_CPUCAPACITY) && (sd->span_weight > 1))
>> +               return sd->smt_gain / sd->span_weight;
>> +
>> +       return SCHED_CAPACITY_SCALE;
>> +}
>> +#endif
>> +
>
> So you change the way to declare arch_scale_cpu_capacity but i don't
> see the update of the arm arch which declare a
> arch_scale_cpu_capacity to reflect this change in your series.

You mentioned in the cover letter that the arch part will be posted
separately later but i'm not sure that it's a good idea to separate
the change of an interface and the update of the archs that implement
it. IMHO, it's better to keep all modification together in one
patchset

Regards,
Vincent
>
> Regards,
> Vincent
>
>>  static inline void sched_rt_avg_update(struct rq *rq, u64 rt_delta)
>>  {
>>         rq->rt_avg += rt_delta * arch_scale_freq_capacity(NULL, cpu_of(rq));
>> --
>> 1.9.1
>>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/