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

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
        Re: [PATCH 2/6] sched/fair: Convert arch_scale_cpu_capacity() from  weak function to #define Dietmar Eggemann <dietmar.eggemann@arm.com> - 2015-09-07 15:30 +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-11 15:30 +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-11 16:50 +0200

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

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

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

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

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] | [next] | [standalone]

#1220176

On 04/09/15 08:26, Vincent Guittot wrote:
> On 3 September 2015 at 21:58, Dietmar Eggemann <dietmar.eggemann@arm.com> wrote:

[...]

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

The only problem with this solution is that now we got a call to
arch_scale_cpu_capacity() in the hotpath whereas before it is only
called in update_cpu_capacity(). An implementation of
scale_cpu_capacity() in arch/arm/kernel/topology.c leads to a function
call in __update_load_avg. I'm in the middle of doing some performance
tests on TC2 w/ and w/o the cpu invariant implementation.

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

Yeah, the cpu invariant functionality should not base on EAS. We just
use the short-cut in EAS RFCv5 to get it working on ARM64.

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

Agreed.

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

#1222847

On 04/09/15 08:26, Vincent Guittot wrote:
> On 3 September 2015 at 21:58, Dietmar Eggemann <dietmar.eggemann@arm.com> wrote:

[...]

> 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

My tests on ARM TC2 (only the 2 A15's) show that the influence of the
non-default arch_scale_cpu_capacity function in
arch/arm/kernel/topology.c and with it the extra function call to it
from __update_load_avg() has no measurable influence on performance:

perf stat --null --repeat 10 -- perf bench sched messaging --g 50 -l 200

- default arch_scale_cpu_capacity function [kernel/sched/sched.h]

16.006976251 seconds time elapsed ( +- 0.29% )
16.063814914 seconds time elapsed ( +- 0.37% )
16.088199252 seconds time elapsed ( +- 0.39% )

- arch_scale_cpu_capacity function [arch/arm/kernel/topology.c]

15.945975308 seconds time elapsed ( +- 0.10% )
16.131203074 seconds time elapsed ( +- 0.21% )
16.108302562 seconds time elapsed ( +- 0.41% )

If I force the function to be inline, the result is slightly worse:

- arch_scale_cpu_capacity function [arch/arm/include/asm/topology.h]

16.122545216 seconds time elapsed ( +- 0.09% )
16.285819258 seconds time elapsed ( +- 0.35% )
16.157454024 seconds time elapsed ( +- 0.15% )

So I think we can connect the arch_scale_cpu_capacity function
[arch/arm/kernel/topology.c] to the CFS scheduler for ARCH=arm so that
people get cpu scale different to 1024 on arm big.little machines w/
A15/A7 in case the specify clock-frequency properties in their dtb file.

Can we still have your 'Acked-by' for this patch and 3/6 even though we
now scale weight (by frequency) and scale_freq (by cpu) instead of the
time related values (delta_w, contrib, delta)?

Thanks,

-- Dietmar

> 
> 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] | [next] | [standalone]

#1222893

On 11 September 2015 at 15:21, Dietmar Eggemann
<dietmar.eggemann@arm.com> wrote:
> On 04/09/15 08:26, Vincent Guittot wrote:
>> On 3 September 2015 at 21:58, Dietmar Eggemann <dietmar.eggemann@arm.com> wrote:
>
> [...]
>
>> 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
>
> My tests on ARM TC2 (only the 2 A15's) show that the influence of the
> non-default arch_scale_cpu_capacity function in
> arch/arm/kernel/topology.c and with it the extra function call to it
> from __update_load_avg() has no measurable influence on performance:
>
> perf stat --null --repeat 10 -- perf bench sched messaging --g 50 -l 200
>
> - default arch_scale_cpu_capacity function [kernel/sched/sched.h]
>
> 16.006976251 seconds time elapsed ( +- 0.29% )
> 16.063814914 seconds time elapsed ( +- 0.37% )
> 16.088199252 seconds time elapsed ( +- 0.39% )
>
> - arch_scale_cpu_capacity function [arch/arm/kernel/topology.c]
>
> 15.945975308 seconds time elapsed ( +- 0.10% )
> 16.131203074 seconds time elapsed ( +- 0.21% )
> 16.108302562 seconds time elapsed ( +- 0.41% )
>
> If I force the function to be inline, the result is slightly worse:
>
> - arch_scale_cpu_capacity function [arch/arm/include/asm/topology.h]
>
> 16.122545216 seconds time elapsed ( +- 0.09% )
> 16.285819258 seconds time elapsed ( +- 0.35% )
> 16.157454024 seconds time elapsed ( +- 0.15% )
>
> So I think we can connect the arch_scale_cpu_capacity function
> [arch/arm/kernel/topology.c] to the CFS scheduler for ARCH=arm so that
> people get cpu scale different to 1024 on arm big.little machines w/
> A15/A7 in case the specify clock-frequency properties in their dtb file.
>
> Can we still have your 'Acked-by' for this patch and 3/6 even though we
> now scale weight (by frequency) and scale_freq (by cpu) instead of the
> time related values (delta_w, contrib, delta)?

Yes, Please add my Acked-by

Vincent

>
> Thanks,
>
> -- Dietmar
>
>>
>> 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