Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1217444 > unrolled thread

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

Started byVincent Guittot <vincent.guittot@linaro.org>
First post2015-09-02 11:40 +0200
Last post2015-09-04 09:30 +0200
Articles 4 — 2 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 2/6] sched/fair: Convert arch_scale_cpu_capacity() from  weak function to #define Vincent Guittot <vincent.guittot@linaro.org> - 2015-09-02 11:40 +0200
    Re: [PATCH 2/6] sched/fair: Convert arch_scale_cpu_capacity() from  weak function to #define Vincent Guittot <vincent.guittot@linaro.org> - 2015-09-02 14:50 +0200
    Re: [PATCH 2/6] sched/fair: Convert arch_scale_cpu_capacity() from  weak function to #define Dietmar Eggemann <dietmar.eggemann@arm.com> - 2015-09-03 22:00 +0200
      Re: [PATCH 2/6] sched/fair: Convert arch_scale_cpu_capacity() from  weak function to #define Vincent Guittot <vincent.guittot@linaro.org> - 2015-09-04 09:30 +0200

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

FromVincent Guittot <vincent.guittot@linaro.org>
Date2015-09-02 11:40 +0200
SubjectRe: [PATCH 2/6] sched/fair: Convert arch_scale_cpu_capacity() from weak function to #define
Message-ID<q4cGm-17b-3@gated-at.bofh.it>
Hi Morten,

On 14 August 2015 at 18:23, Morten Rasmussen <morten.rasmussen@arm.com> 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@redhat.com>
> cc: Peter Zijlstra <peterz@infradead.org>
>
> Signed-off-by: Morten Rasmussen <morten.rasmussen@arm.com>
> ---
>  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.

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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1217577

FromVincent Guittot <vincent.guittot@linaro.org>
Date2015-09-02 14:50 +0200
Message-ID<q4fEe-5kp-7@gated-at.bofh.it>
In reply to#1217444
On 2 September 2015 at 11:31, Vincent Guittot
<vincent.guittot@linaro.org> wrote:
> Hi Morten,
>
> On 14 August 2015 at 18:23, Morten Rasmussen <morten.rasmussen@arm.com> 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@redhat.com>
>> cc: Peter Zijlstra <peterz@infradead.org>
>>
>> Signed-off-by: Morten Rasmussen <morten.rasmussen@arm.com>
>> ---
>>  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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1218551

FromDietmar Eggemann <dietmar.eggemann@arm.com>
Date2015-09-03 22:00 +0200
Message-ID<q4IPU-503-29@gated-at.bofh.it>
In reply to#1217444
Hi Vincent,

On 02/09/15 10:31, Vincent Guittot wrote:
> Hi Morten,
> 
> On 14 August 2015 at 18:23, Morten Rasmussen <morten.rasmussen@arm.com> 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.

[...]

> 
> 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.

We were reluctant to do this because this functionality makes only sense
for ARCH=arm big.Little systems w/ cortex-a{15|7} cores and only if the
clock-frequency property is set in the dts file. 

Are you planning to push for a 'struct cpu_efficiency/clock-frequency
property' solution for ARCH=arm64 as well?

I'm asking because for ARCH=arm64 systems today (JUNO, Hi6220) we use the
capacity value of the last entry of the capacity_state vector for the cores
(e.g. cortex-a{57|53). 
    
To connect the cpu invariant engine (scale_cpu_capacity()
[arch/arm/kernel/topology.c]) with the scheduler, something like this is
missing:

diff --git a/arch/arm/include/asm/topology.h b/arch/arm/include/asm/topology.h
index 370f7a732900..17c6b3243196 100644
--- a/arch/arm/include/asm/topology.h
+++ b/arch/arm/include/asm/topology.h
@@ -24,6 +24,10 @@ void init_cpu_topology(void);
 void store_cpu_topology(unsigned int cpuid);
 const struct cpumask *cpu_coregroup_mask(int cpu);
 
+#define arch_scale_cpu_capacity scale_cpu_capacity
+struct sched_domain;
+extern unsigned long scale_cpu_capacity(struct sched_domain *sd, int cpu);
+
 #else
 
 static inline void init_cpu_topology(void) { }
diff --git a/arch/arm/kernel/topology.c b/arch/arm/kernel/topology.c
index 08b7847bf912..907e0d2d9b82 100644
--- a/arch/arm/kernel/topology.c
+++ b/arch/arm/kernel/topology.c
@@ -42,7 +42,7 @@
  */
 static DEFINE_PER_CPU(unsigned long, cpu_scale);
 
-unsigned long arch_scale_cpu_capacity(struct sched_domain *sd, int cpu)
+unsigned long scale_cpu_capacity(struct sched_domain *sd, int cpu)
 {
        return per_cpu(cpu_scale, cpu);
 }
@@ -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, scale_cpu_capacity(NULL, cpu));
 }

-- Dietmar

[...]

--
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/

[toc] | [prev] | [next] | [standalone]

#1218702

FromVincent Guittot <vincent.guittot@linaro.org>
Date2015-09-04 09:30 +0200
Message-ID<q4TBE-3Gn-5@gated-at.bofh.it>
In reply to#1218551
On 3 September 2015 at 21:58, Dietmar Eggemann <dietmar.eggemann@arm.com> wrote:
> Hi Vincent,
>
> On 02/09/15 10:31, Vincent Guittot wrote:
>> Hi Morten,
>>
>> On 14 August 2015 at 18:23, Morten Rasmussen <morten.rasmussen@arm.com> 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.
>
> [...]
>
>>
>> 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.
>
> We were reluctant to do this because this functionality makes only sense
> for ARCH=arm big.Little systems w/ cortex-a{15|7} cores and only if the
> clock-frequency property is set in the dts file.

IMO, we should maintain the compatibility of current implementation
instead of breaking the link and creating a dead code.
Your proposal below fits the requirement

>
> Are you planning to push for a 'struct cpu_efficiency/clock-frequency
> property' solution for ARCH=arm64 as well?

I know that there has been some discussions aorund that but i didn't
follow the thread in details

>
> I'm asking because for ARCH=arm64 systems today (JUNO, Hi6220) we use the
> capacity value of the last entry of the capacity_state vector for the cores
> (e.g. cortex-a{57|53).

This is a struct of the eas feature ? Not sure that we should link the
definition of  the cpu capacity to an internal struct of a feature; DT
seems a better way to define it.
So if you want to revisit the way, we set the capacity of CPU for arm
and/or arm64, I'm fully open to the discussion but this should happen
in another thread than this one which has for only purpose the
alignment of the arch_scale_cpu_capacity interface declaration with
arch_scale_freq_capacity one.

So, with the patch below that updates the arm definition of
arch_scale_cpu_capacity, you can add my Acked-by: Vincent Guittot
<vincent.guittot@linaro.org> on this patch and the additional one
below

Regards,
Vincent

>
> To connect the cpu invariant engine (scale_cpu_capacity()
> [arch/arm/kernel/topology.c]) with the scheduler, something like this is
> missing:
>
> diff --git a/arch/arm/include/asm/topology.h b/arch/arm/include/asm/topology.h
> index 370f7a732900..17c6b3243196 100644
> --- a/arch/arm/include/asm/topology.h
> +++ b/arch/arm/include/asm/topology.h
> @@ -24,6 +24,10 @@ void init_cpu_topology(void);
>  void store_cpu_topology(unsigned int cpuid);
>  const struct cpumask *cpu_coregroup_mask(int cpu);
>
> +#define arch_scale_cpu_capacity scale_cpu_capacity
> +struct sched_domain;
> +extern unsigned long scale_cpu_capacity(struct sched_domain *sd, int cpu);
> +
>  #else
>
>  static inline void init_cpu_topology(void) { }
> diff --git a/arch/arm/kernel/topology.c b/arch/arm/kernel/topology.c
> index 08b7847bf912..907e0d2d9b82 100644
> --- a/arch/arm/kernel/topology.c
> +++ b/arch/arm/kernel/topology.c
> @@ -42,7 +42,7 @@
>   */
>  static DEFINE_PER_CPU(unsigned long, cpu_scale);
>
> -unsigned long arch_scale_cpu_capacity(struct sched_domain *sd, int cpu)
> +unsigned long scale_cpu_capacity(struct sched_domain *sd, int cpu)
>  {
>         return per_cpu(cpu_scale, cpu);
>  }
> @@ -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, scale_cpu_capacity(NULL, cpu));
>  }
>
> -- Dietmar
>
> [...]
>
--
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/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web