[PATCH] sched/fair: Introduce fits_capacity()

[PATCH] sched/fair: Introduce fits_capacity()

[Viresh Kumar]

The same formula to check utilization against capacity (after
considering capacity_margin) is already used at 5 different locations.

This patch creates a new macro, fits_capacity(), which can be used from
all these locations without exposing the details of it and hence
simplify code.

All the 5 code locations are updated as well to use it..

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 kernel/sched/fair.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 7f8d477f90fe..db3a218b7928 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -102,6 +102,8 @@ int __weak arch_asym_cpu_priority(int cpu)
  * (default: ~20%)
  */
 static unsigned int capacity_margin			= 1280;
+
+#define fits_capacity(cap, max)	((cap) * capacity_margin < (max) * 1024)
 #endif
 
 #ifdef CONFIG_CFS_BANDWIDTH
@@ -3727,7 +3729,7 @@ util_est_dequeue(struct cfs_rq *cfs_rq, struct task_struct *p, bool task_sleep)
 
 static inline int task_fits_capacity(struct task_struct *p, long capacity)
 {
-	return capacity * 1024 > task_util_est(p) * capacity_margin;
+	return fits_capacity(task_util_est(p), capacity);
 }
 
 static inline void update_misfit_status(struct task_struct *p, struct rq *rq)
@@ -5143,7 +5145,7 @@ static inline unsigned long cpu_util(int cpu);
 
 static inline bool cpu_overutilized(int cpu)
 {
-	return (capacity_of(cpu) * 1024) < (cpu_util(cpu) * capacity_margin);
+	return !fits_capacity(cpu_util(cpu), capacity_of(cpu));
 }
 
 static inline void update_overutilized_status(struct rq *rq)
@@ -6304,7 +6306,7 @@ static int find_energy_efficient_cpu(struct task_struct *p, int prev_cpu)
 			/* Skip CPUs that will be overutilized. */
 			util = cpu_util_next(cpu, p, cpu);
 			cpu_cap = capacity_of(cpu);
-			if (cpu_cap * 1024 < util * capacity_margin)
+			if (!fits_capacity(util, cpu_cap))
 				continue;
 
 			/* Always use prev_cpu as a candidate. */
@@ -7853,8 +7855,7 @@ group_is_overloaded(struct lb_env *env, struct sg_lb_stats *sgs)
 static inline bool
 group_smaller_min_cpu_capacity(struct sched_group *sg, struct sched_group *ref)
 {
-	return sg->sgc->min_capacity * capacity_margin <
-						ref->sgc->min_capacity * 1024;
+	return fits_capacity(sg->sgc->min_capacity, ref->sgc->min_capacity);
 }
 
 /*
@@ -7864,8 +7865,7 @@ group_smaller_min_cpu_capacity(struct sched_group *sg, struct sched_group *ref)
 static inline bool
 group_smaller_max_cpu_capacity(struct sched_group *sg, struct sched_group *ref)
 {
-	return sg->sgc->max_capacity * capacity_margin <
-						ref->sgc->max_capacity * 1024;
+	return fits_capacity(sg->sgc->max_capacity, ref->sgc->max_capacity);
 }
 
 static inline enum
-- 
2.21.0.rc0.269.g1a574e7a288b

Re: [PATCH] sched/fair: Introduce fits_capacity()

[Peter Oskolkov]

On Tue, Jun 4, 2019 at 12:02 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> The same formula to check utilization against capacity (after
> considering capacity_margin) is already used at 5 different locations.
>
> This patch creates a new macro, fits_capacity(), which can be used from
> all these locations without exposing the details of it and hence
> simplify code.
>
> All the 5 code locations are updated as well to use it..
>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  kernel/sched/fair.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 7f8d477f90fe..db3a218b7928 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -102,6 +102,8 @@ int __weak arch_asym_cpu_priority(int cpu)
>   * (default: ~20%)
>   */
>  static unsigned int capacity_margin                    = 1280;
> +
> +#define fits_capacity(cap, max)        ((cap) * capacity_margin < (max) * 1024)

Any reason to have this as a macro and not as an inline function?

>  #endif
>
>  #ifdef CONFIG_CFS_BANDWIDTH
> @@ -3727,7 +3729,7 @@ util_est_dequeue(struct cfs_rq *cfs_rq, struct task_struct *p, bool task_sleep)
>
>  static inline int task_fits_capacity(struct task_struct *p, long capacity)
>  {
> -       return capacity * 1024 > task_util_est(p) * capacity_margin;
> +       return fits_capacity(task_util_est(p), capacity);
>  }
>
>  static inline void update_misfit_status(struct task_struct *p, struct rq *rq)
> @@ -5143,7 +5145,7 @@ static inline unsigned long cpu_util(int cpu);
>
>  static inline bool cpu_overutilized(int cpu)
>  {
> -       return (capacity_of(cpu) * 1024) < (cpu_util(cpu) * capacity_margin);
> +       return !fits_capacity(cpu_util(cpu), capacity_of(cpu));
>  }
>
>  static inline void update_overutilized_status(struct rq *rq)
> @@ -6304,7 +6306,7 @@ static int find_energy_efficient_cpu(struct task_struct *p, int prev_cpu)
>                         /* Skip CPUs that will be overutilized. */
>                         util = cpu_util_next(cpu, p, cpu);
>                         cpu_cap = capacity_of(cpu);
> -                       if (cpu_cap * 1024 < util * capacity_margin)
> +                       if (!fits_capacity(util, cpu_cap))
>                                 continue;
>
>                         /* Always use prev_cpu as a candidate. */
> @@ -7853,8 +7855,7 @@ group_is_overloaded(struct lb_env *env, struct sg_lb_stats *sgs)
>  static inline bool
>  group_smaller_min_cpu_capacity(struct sched_group *sg, struct sched_group *ref)
>  {
> -       return sg->sgc->min_capacity * capacity_margin <
> -                                               ref->sgc->min_capacity * 1024;
> +       return fits_capacity(sg->sgc->min_capacity, ref->sgc->min_capacity);
>  }
>
>  /*
> @@ -7864,8 +7865,7 @@ group_smaller_min_cpu_capacity(struct sched_group *sg, struct sched_group *ref)
>  static inline bool
>  group_smaller_max_cpu_capacity(struct sched_group *sg, struct sched_group *ref)
>  {
> -       return sg->sgc->max_capacity * capacity_margin <
> -                                               ref->sgc->max_capacity * 1024;
> +       return fits_capacity(sg->sgc->max_capacity, ref->sgc->max_capacity);
>  }
>
>  static inline enum
> --
> 2.21.0.rc0.269.g1a574e7a288b
>

Re: [PATCH] sched/fair: Introduce fits_capacity()

[Quentin Perret]

Hi Viresh,

On Tuesday 04 Jun 2019 at 12:31:52 (+0530), Viresh Kumar wrote:
> The same formula to check utilization against capacity (after
> considering capacity_margin) is already used at 5 different locations.
> 
> This patch creates a new macro, fits_capacity(), which can be used from
> all these locations without exposing the details of it and hence
> simplify code.
> 
> All the 5 code locations are updated as well to use it..
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  kernel/sched/fair.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 7f8d477f90fe..db3a218b7928 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -102,6 +102,8 @@ int __weak arch_asym_cpu_priority(int cpu)
>   * (default: ~20%)
>   */
>  static unsigned int capacity_margin			= 1280;
> +
> +#define fits_capacity(cap, max)	((cap) * capacity_margin < (max) * 1024)
>  #endif
>  
>  #ifdef CONFIG_CFS_BANDWIDTH
> @@ -3727,7 +3729,7 @@ util_est_dequeue(struct cfs_rq *cfs_rq, struct task_struct *p, bool task_sleep)
>  
>  static inline int task_fits_capacity(struct task_struct *p, long capacity)
>  {
> -	return capacity * 1024 > task_util_est(p) * capacity_margin;
> +	return fits_capacity(task_util_est(p), capacity);
>  }
>  
>  static inline void update_misfit_status(struct task_struct *p, struct rq *rq)
> @@ -5143,7 +5145,7 @@ static inline unsigned long cpu_util(int cpu);
>  
>  static inline bool cpu_overutilized(int cpu)
>  {
> -	return (capacity_of(cpu) * 1024) < (cpu_util(cpu) * capacity_margin);
> +	return !fits_capacity(cpu_util(cpu), capacity_of(cpu));

This ...

>  }
>  
>  static inline void update_overutilized_status(struct rq *rq)
> @@ -6304,7 +6306,7 @@ static int find_energy_efficient_cpu(struct task_struct *p, int prev_cpu)
>  			/* Skip CPUs that will be overutilized. */
>  			util = cpu_util_next(cpu, p, cpu);
>  			cpu_cap = capacity_of(cpu);
> -			if (cpu_cap * 1024 < util * capacity_margin)
> +			if (!fits_capacity(util, cpu_cap))

... and this isn't _strictly_ equivalent to the existing code but I
guess we can live with the difference :-)

>  				continue;
>  
>  			/* Always use prev_cpu as a candidate. */
> @@ -7853,8 +7855,7 @@ group_is_overloaded(struct lb_env *env, struct sg_lb_stats *sgs)
>  static inline bool
>  group_smaller_min_cpu_capacity(struct sched_group *sg, struct sched_group *ref)
>  {
> -	return sg->sgc->min_capacity * capacity_margin <
> -						ref->sgc->min_capacity * 1024;
> +	return fits_capacity(sg->sgc->min_capacity, ref->sgc->min_capacity);
>  }
>  
>  /*
> @@ -7864,8 +7865,7 @@ group_smaller_min_cpu_capacity(struct sched_group *sg, struct sched_group *ref)
>  static inline bool
>  group_smaller_max_cpu_capacity(struct sched_group *sg, struct sched_group *ref)
>  {
> -	return sg->sgc->max_capacity * capacity_margin <
> -						ref->sgc->max_capacity * 1024;
> +	return fits_capacity(sg->sgc->max_capacity, ref->sgc->max_capacity);
>  }
>  
>  static inline enum
> -- 
> 2.21.0.rc0.269.g1a574e7a288b
> 

Also, since we're talking about making the capacity_margin code more
consistent, one small thing I had in mind: we have a capacity margin
in sugov too, which happens to be 1.25 has well (see map_util_freq()).
Conceptually, capacity_margin in fair.c and the sugov margin are both
about answering: "do I have enough CPU capacity to serve X of util, or
do I need more ?"

So perhaps we should factorize the capacity_margin code some more to use
it in both places in a consistent way ? This could be done in a separate
patch, though.

Thanks,
Quentin

Re: [PATCH] sched/fair: Introduce fits_capacity()

[Viresh Kumar]

On 05-06-19, 10:16, Quentin Perret wrote:
> Hi Viresh,
> 
> On Tuesday 04 Jun 2019 at 12:31:52 (+0530), Viresh Kumar wrote:
> > The same formula to check utilization against capacity (after
> > considering capacity_margin) is already used at 5 different locations.
> > 
> > This patch creates a new macro, fits_capacity(), which can be used from
> > all these locations without exposing the details of it and hence
> > simplify code.
> > 
> > All the 5 code locations are updated as well to use it..
> > 
> > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> > ---
> >  kernel/sched/fair.c | 14 +++++++-------
> >  1 file changed, 7 insertions(+), 7 deletions(-)
> > 
> > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> > index 7f8d477f90fe..db3a218b7928 100644
> > --- a/kernel/sched/fair.c
> > +++ b/kernel/sched/fair.c
> > @@ -102,6 +102,8 @@ int __weak arch_asym_cpu_priority(int cpu)
> >   * (default: ~20%)
> >   */
> >  static unsigned int capacity_margin			= 1280;
> > +
> > +#define fits_capacity(cap, max)	((cap) * capacity_margin < (max) * 1024)
> >  #endif
> >  
> >  #ifdef CONFIG_CFS_BANDWIDTH
> > @@ -3727,7 +3729,7 @@ util_est_dequeue(struct cfs_rq *cfs_rq, struct task_struct *p, bool task_sleep)
> >  
> >  static inline int task_fits_capacity(struct task_struct *p, long capacity)
> >  {
> > -	return capacity * 1024 > task_util_est(p) * capacity_margin;
> > +	return fits_capacity(task_util_est(p), capacity);
> >  }
> >  
> >  static inline void update_misfit_status(struct task_struct *p, struct rq *rq)
> > @@ -5143,7 +5145,7 @@ static inline unsigned long cpu_util(int cpu);
> >  
> >  static inline bool cpu_overutilized(int cpu)
> >  {
> > -	return (capacity_of(cpu) * 1024) < (cpu_util(cpu) * capacity_margin);
> > +	return !fits_capacity(cpu_util(cpu), capacity_of(cpu));
> 
> This ...
> 
> >  }
> >  
> >  static inline void update_overutilized_status(struct rq *rq)
> > @@ -6304,7 +6306,7 @@ static int find_energy_efficient_cpu(struct task_struct *p, int prev_cpu)
> >  			/* Skip CPUs that will be overutilized. */
> >  			util = cpu_util_next(cpu, p, cpu);
> >  			cpu_cap = capacity_of(cpu);
> > -			if (cpu_cap * 1024 < util * capacity_margin)
> > +			if (!fits_capacity(util, cpu_cap))
> 
> ... and this isn't _strictly_ equivalent to the existing code but I
> guess we can live with the difference :-)

Yes, I missed the == part it seems. Good catch. Though as you said,
maybe we don't need to take that into account and can live with the
new macro :)

> 
> >  				continue;
> >  
> >  			/* Always use prev_cpu as a candidate. */
> > @@ -7853,8 +7855,7 @@ group_is_overloaded(struct lb_env *env, struct sg_lb_stats *sgs)
> >  static inline bool
> >  group_smaller_min_cpu_capacity(struct sched_group *sg, struct sched_group *ref)
> >  {
> > -	return sg->sgc->min_capacity * capacity_margin <
> > -						ref->sgc->min_capacity * 1024;
> > +	return fits_capacity(sg->sgc->min_capacity, ref->sgc->min_capacity);
> >  }
> >  
> >  /*
> > @@ -7864,8 +7865,7 @@ group_smaller_min_cpu_capacity(struct sched_group *sg, struct sched_group *ref)
> >  static inline bool
> >  group_smaller_max_cpu_capacity(struct sched_group *sg, struct sched_group *ref)
> >  {
> > -	return sg->sgc->max_capacity * capacity_margin <
> > -						ref->sgc->max_capacity * 1024;
> > +	return fits_capacity(sg->sgc->max_capacity, ref->sgc->max_capacity);
> >  }
> >  
> >  static inline enum
> > -- 
> > 2.21.0.rc0.269.g1a574e7a288b
> > 
> 
> Also, since we're talking about making the capacity_margin code more
> consistent, one small thing I had in mind: we have a capacity margin
> in sugov too, which happens to be 1.25 has well (see map_util_freq()).
> Conceptually, capacity_margin in fair.c and the sugov margin are both
> about answering: "do I have enough CPU capacity to serve X of util, or
> do I need more ?"
> 
> So perhaps we should factorize the capacity_margin code some more to use
> it in both places in a consistent way ? This could be done in a separate
> patch, though.

Hmm, even if the values are same currently I am not sure if we want
the same for ever. I will write a patch for it though, if Peter/Rafael
feel the same as you.

Thanks Quentin.

-- 
viresh

Re: [PATCH] sched/fair: Introduce fits_capacity()

[Viresh Kumar]

On 04-06-19, 08:59, Peter Oskolkov wrote:
> On Tue, Jun 4, 2019 at 12:02 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> >
> > The same formula to check utilization against capacity (after
> > considering capacity_margin) is already used at 5 different locations.
> >
> > This patch creates a new macro, fits_capacity(), which can be used from
> > all these locations without exposing the details of it and hence
> > simplify code.
> >
> > All the 5 code locations are updated as well to use it..
> >
> > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> > ---
> >  kernel/sched/fair.c | 14 +++++++-------
> >  1 file changed, 7 insertions(+), 7 deletions(-)
> >
> > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> > index 7f8d477f90fe..db3a218b7928 100644
> > --- a/kernel/sched/fair.c
> > +++ b/kernel/sched/fair.c
> > @@ -102,6 +102,8 @@ int __weak arch_asym_cpu_priority(int cpu)
> >   * (default: ~20%)
> >   */
> >  static unsigned int capacity_margin                    = 1280;
> > +
> > +#define fits_capacity(cap, max)        ((cap) * capacity_margin < (max) * 1024)
> 
> Any reason to have this as a macro and not as an inline function?

I don't have any strong preference here, I used a macro as I didn't
feel that type-checking is really required on the parameters and
eventually this will get open coded anyway.

Though I would be fine to make it a routine if maintainers want it
that way.

Thanks Peter.

-- 
viresh

Re: [PATCH] sched/fair: Introduce fits_capacity()

[Quentin Perret]

On Thursday 06 Jun 2019 at 08:22:04 (+0530), Viresh Kumar wrote:
> On 05-06-19, 10:16, Quentin Perret wrote:
> > Hi Viresh,
> > 
> > On Tuesday 04 Jun 2019 at 12:31:52 (+0530), Viresh Kumar wrote:
> > > The same formula to check utilization against capacity (after
> > > considering capacity_margin) is already used at 5 different locations.
> > > 
> > > This patch creates a new macro, fits_capacity(), which can be used from
> > > all these locations without exposing the details of it and hence
> > > simplify code.
> > > 
> > > All the 5 code locations are updated as well to use it..
> > > 
> > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> > > ---
> > >  kernel/sched/fair.c | 14 +++++++-------
> > >  1 file changed, 7 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> > > index 7f8d477f90fe..db3a218b7928 100644
> > > --- a/kernel/sched/fair.c
> > > +++ b/kernel/sched/fair.c
> > > @@ -102,6 +102,8 @@ int __weak arch_asym_cpu_priority(int cpu)
> > >   * (default: ~20%)
> > >   */
> > >  static unsigned int capacity_margin			= 1280;
> > > +
> > > +#define fits_capacity(cap, max)	((cap) * capacity_margin < (max) * 1024)
> > >  #endif
> > >  
> > >  #ifdef CONFIG_CFS_BANDWIDTH
> > > @@ -3727,7 +3729,7 @@ util_est_dequeue(struct cfs_rq *cfs_rq, struct task_struct *p, bool task_sleep)
> > >  
> > >  static inline int task_fits_capacity(struct task_struct *p, long capacity)
> > >  {
> > > -	return capacity * 1024 > task_util_est(p) * capacity_margin;
> > > +	return fits_capacity(task_util_est(p), capacity);
> > >  }
> > >  
> > >  static inline void update_misfit_status(struct task_struct *p, struct rq *rq)
> > > @@ -5143,7 +5145,7 @@ static inline unsigned long cpu_util(int cpu);
> > >  
> > >  static inline bool cpu_overutilized(int cpu)
> > >  {
> > > -	return (capacity_of(cpu) * 1024) < (cpu_util(cpu) * capacity_margin);
> > > +	return !fits_capacity(cpu_util(cpu), capacity_of(cpu));
> > 
> > This ...
> > 
> > >  }
> > >  
> > >  static inline void update_overutilized_status(struct rq *rq)
> > > @@ -6304,7 +6306,7 @@ static int find_energy_efficient_cpu(struct task_struct *p, int prev_cpu)
> > >  			/* Skip CPUs that will be overutilized. */
> > >  			util = cpu_util_next(cpu, p, cpu);
> > >  			cpu_cap = capacity_of(cpu);
> > > -			if (cpu_cap * 1024 < util * capacity_margin)
> > > +			if (!fits_capacity(util, cpu_cap))
> > 
> > ... and this isn't _strictly_ equivalent to the existing code but I
> > guess we can live with the difference :-)
> 
> Yes, I missed the == part it seems. Good catch. Though as you said,
> maybe we don't need to take that into account and can live with the
> new macro :)
> 
> > 
> > >  				continue;
> > >  
> > >  			/* Always use prev_cpu as a candidate. */
> > > @@ -7853,8 +7855,7 @@ group_is_overloaded(struct lb_env *env, struct sg_lb_stats *sgs)
> > >  static inline bool
> > >  group_smaller_min_cpu_capacity(struct sched_group *sg, struct sched_group *ref)
> > >  {
> > > -	return sg->sgc->min_capacity * capacity_margin <
> > > -						ref->sgc->min_capacity * 1024;
> > > +	return fits_capacity(sg->sgc->min_capacity, ref->sgc->min_capacity);
> > >  }
> > >  
> > >  /*
> > > @@ -7864,8 +7865,7 @@ group_smaller_min_cpu_capacity(struct sched_group *sg, struct sched_group *ref)
> > >  static inline bool
> > >  group_smaller_max_cpu_capacity(struct sched_group *sg, struct sched_group *ref)
> > >  {
> > > -	return sg->sgc->max_capacity * capacity_margin <
> > > -						ref->sgc->max_capacity * 1024;
> > > +	return fits_capacity(sg->sgc->max_capacity, ref->sgc->max_capacity);
> > >  }
> > >  
> > >  static inline enum
> > > -- 
> > > 2.21.0.rc0.269.g1a574e7a288b
> > > 
> > 
> > Also, since we're talking about making the capacity_margin code more
> > consistent, one small thing I had in mind: we have a capacity margin
> > in sugov too, which happens to be 1.25 has well (see map_util_freq()).
> > Conceptually, capacity_margin in fair.c and the sugov margin are both
> > about answering: "do I have enough CPU capacity to serve X of util, or
> > do I need more ?"
> > 
> > So perhaps we should factorize the capacity_margin code some more to use
> > it in both places in a consistent way ? This could be done in a separate
> > patch, though.
> 
> Hmm, even if the values are same currently I am not sure if we want
> the same for ever.

Right, that's a good point. It is cheaper to raise the freq on a CPU than
to migrate a task, so perhaps there could be a case for different
thresholds ...

> I will write a patch for it though, if Peter/Rafael
> feel the same as you.

Sounds good, thanks !
Quentin

Re: [PATCH] sched/fair: Introduce fits_capacity()

[Peter Zijlstra]

On Thu, Jun 06, 2019 at 08:22:04AM +0530, Viresh Kumar wrote:
> Hmm, even if the values are same currently I am not sure if we want
> the same for ever. I will write a patch for it though, if Peter/Rafael
> feel the same as you.

Is it really the same variable or just two numbers that happen to be the
same?

Re: [PATCH] sched/fair: Introduce fits_capacity()

[Viresh Kumar]

+Rafael

On 17-06-19, 17:02, Peter Zijlstra wrote:
> On Thu, Jun 06, 2019 at 08:22:04AM +0530, Viresh Kumar wrote:
> > Hmm, even if the values are same currently I am not sure if we want
> > the same for ever. I will write a patch for it though, if Peter/Rafael
> > feel the same as you.
> 
> Is it really the same variable or just two numbers that happen to be the
> same?

In both cases we are trying to keep the load under 80% of what can be supported.
But I am not sure of the answer to your question.

Maybe Rafael knows :)

-- 
viresh

Re: [PATCH] sched/fair: Introduce fits_capacity()

[Rafael J. Wysocki]

On Tue, Jun 18, 2019 at 5:12 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> +Rafael
>
> On 17-06-19, 17:02, Peter Zijlstra wrote:
> > On Thu, Jun 06, 2019 at 08:22:04AM +0530, Viresh Kumar wrote:
> > > Hmm, even if the values are same currently I am not sure if we want
> > > the same for ever. I will write a patch for it though, if Peter/Rafael
> > > feel the same as you.
> >
> > Is it really the same variable or just two numbers that happen to be the
> > same?
>
> In both cases we are trying to keep the load under 80% of what can be supported.
> But I am not sure of the answer to your question.
>
> Maybe Rafael knows :)

Which variable?

Re: [PATCH] sched/fair: Introduce fits_capacity()

[Viresh Kumar]

On 18-06-19, 09:26, Rafael J. Wysocki wrote:
> On Tue, Jun 18, 2019 at 5:12 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> >
> > +Rafael
> >
> > On 17-06-19, 17:02, Peter Zijlstra wrote:
> > > On Thu, Jun 06, 2019 at 08:22:04AM +0530, Viresh Kumar wrote:
> > > > Hmm, even if the values are same currently I am not sure if we want
> > > > the same for ever. I will write a patch for it though, if Peter/Rafael
> > > > feel the same as you.
> > >
> > > Is it really the same variable or just two numbers that happen to be the
> > > same?
> >
> > In both cases we are trying to keep the load under 80% of what can be supported.
> > But I am not sure of the answer to your question.
> >
> > Maybe Rafael knows :)
> 
> Which variable?

Schedutil multiplies the target frequency by 1.25 (20% more capacity eventually)
to get enough room for more load and similar thing is done in fair.c at several
places to see if the new task can fit in a runqueue without overloading it.

Quentin suggested to use common code for this calculation and that is what is
getting discussed here.

-- 
viresh

Re: [PATCH] sched/fair: Introduce fits_capacity()

[Rafael J. Wysocki]

On Tue, Jun 18, 2019 at 9:47 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 18-06-19, 09:26, Rafael J. Wysocki wrote:
> > On Tue, Jun 18, 2019 at 5:12 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > >
> > > +Rafael
> > >
> > > On 17-06-19, 17:02, Peter Zijlstra wrote:
> > > > On Thu, Jun 06, 2019 at 08:22:04AM +0530, Viresh Kumar wrote:
> > > > > Hmm, even if the values are same currently I am not sure if we want
> > > > > the same for ever. I will write a patch for it though, if Peter/Rafael
> > > > > feel the same as you.
> > > >
> > > > Is it really the same variable or just two numbers that happen to be the
> > > > same?
> > >
> > > In both cases we are trying to keep the load under 80% of what can be supported.
> > > But I am not sure of the answer to your question.
> > >
> > > Maybe Rafael knows :)
> >
> > Which variable?
>
> Schedutil multiplies the target frequency by 1.25 (20% more capacity eventually)
> to get enough room for more load and similar thing is done in fair.c at several
> places to see if the new task can fit in a runqueue without overloading it.

For the schedutil part, see the changelog of the commit that introduced it:

9bdcb44e391d cpufreq: schedutil: New governor based on scheduler
utilization data

As for the other places, I don't know about the exact reasoning.

> Quentin suggested to use common code for this calculation and that is what is
> getting discussed here.

I guess if the rationale for the formula is the same in all cases, it
would be good to consolidate that code and document the rationale
while at it.

Otherwise, I'm not sure.

Re: [PATCH] sched/fair: Introduce fits_capacity()

[Quentin Perret]

On Tuesday 18 Jun 2019 at 13:17:28 (+0530), Viresh Kumar wrote:
> On 18-06-19, 09:26, Rafael J. Wysocki wrote:
> > On Tue, Jun 18, 2019 at 5:12 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > >
> > > +Rafael
> > >
> > > On 17-06-19, 17:02, Peter Zijlstra wrote:
> > > > On Thu, Jun 06, 2019 at 08:22:04AM +0530, Viresh Kumar wrote:
> > > > > Hmm, even if the values are same currently I am not sure if we want
> > > > > the same for ever. I will write a patch for it though, if Peter/Rafael
> > > > > feel the same as you.
> > > >
> > > > Is it really the same variable or just two numbers that happen to be the
> > > > same?
> > >
> > > In both cases we are trying to keep the load under 80% of what can be supported.
> > > But I am not sure of the answer to your question.
> > >
> > > Maybe Rafael knows :)
> > 
> > Which variable?
> 
> Schedutil multiplies the target frequency by 1.25 (20% more capacity eventually)
> to get enough room for more load and similar thing is done in fair.c at several
> places to see if the new task can fit in a runqueue without overloading it.
> 
> Quentin suggested to use common code for this calculation and that is what is
> getting discussed here.

Right, sugov and load balance happen to use the same margin (1.25)
to check if a given util fits in a given capacity, though the thresholds
are hardcoded in different places (see map_util_freq() and
capacity_margin). So my suggestion was to unify the capacity_margin code
for frequency selection and CPU selection, for clarity and consistency.

But again, this is a small thing and FWIW Viresh's patch LGTM as-is so
no objection from my end if you guys would like to merge it.

Thanks,
Quentin

Re: [PATCH] sched/fair: Introduce fits_capacity()

[Quentin Perret]

On Tuesday 18 Jun 2019 at 10:10:48 (+0200), Rafael J. Wysocki wrote:
> On Tue, Jun 18, 2019 at 9:47 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> >
> > On 18-06-19, 09:26, Rafael J. Wysocki wrote:
> > > On Tue, Jun 18, 2019 at 5:12 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > > >
> > > > +Rafael
> > > >
> > > > On 17-06-19, 17:02, Peter Zijlstra wrote:
> > > > > On Thu, Jun 06, 2019 at 08:22:04AM +0530, Viresh Kumar wrote:
> > > > > > Hmm, even if the values are same currently I am not sure if we want
> > > > > > the same for ever. I will write a patch for it though, if Peter/Rafael
> > > > > > feel the same as you.
> > > > >
> > > > > Is it really the same variable or just two numbers that happen to be the
> > > > > same?
> > > >
> > > > In both cases we are trying to keep the load under 80% of what can be supported.
> > > > But I am not sure of the answer to your question.
> > > >
> > > > Maybe Rafael knows :)
> > >
> > > Which variable?
> >
> > Schedutil multiplies the target frequency by 1.25 (20% more capacity eventually)
> > to get enough room for more load and similar thing is done in fair.c at several
> > places to see if the new task can fit in a runqueue without overloading it.
> 
> For the schedutil part, see the changelog of the commit that introduced it:
> 
> 9bdcb44e391d cpufreq: schedutil: New governor based on scheduler
> utilization data
> 
> As for the other places, I don't know about the exact reasoning.
> 
> > Quentin suggested to use common code for this calculation and that is what is
> > getting discussed here.
> 
> I guess if the rationale for the formula is the same in all cases, it
> would be good to consolidate that code and document the rationale
> while at it.

I _think_ it is, but I guess others could correct me if this is
incorrect.

When choosing a CPU or a frequency using a util value, we look for a
capacity that will provide us with 20% of idle time. And in both case we
use the same threshold, just hardcoded in different places. Hence the
suggestion to unify things.

I hope that makes sense :-)

Thanks,
Quentin

Re: [PATCH] sched/fair: Introduce fits_capacity()

[Rafael J. Wysocki]

On Tue, Jun 18, 2019 at 10:25 AM Quentin Perret <quentin.perret@arm.com> wrote:
>
> On Tuesday 18 Jun 2019 at 10:10:48 (+0200), Rafael J. Wysocki wrote:
> > On Tue, Jun 18, 2019 at 9:47 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > >
> > > On 18-06-19, 09:26, Rafael J. Wysocki wrote:
> > > > On Tue, Jun 18, 2019 at 5:12 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > > > >
> > > > > +Rafael
> > > > >
> > > > > On 17-06-19, 17:02, Peter Zijlstra wrote:
> > > > > > On Thu, Jun 06, 2019 at 08:22:04AM +0530, Viresh Kumar wrote:
> > > > > > > Hmm, even if the values are same currently I am not sure if we want
> > > > > > > the same for ever. I will write a patch for it though, if Peter/Rafael
> > > > > > > feel the same as you.
> > > > > >
> > > > > > Is it really the same variable or just two numbers that happen to be the
> > > > > > same?
> > > > >
> > > > > In both cases we are trying to keep the load under 80% of what can be supported.
> > > > > But I am not sure of the answer to your question.
> > > > >
> > > > > Maybe Rafael knows :)
> > > >
> > > > Which variable?
> > >
> > > Schedutil multiplies the target frequency by 1.25 (20% more capacity eventually)
> > > to get enough room for more load and similar thing is done in fair.c at several
> > > places to see if the new task can fit in a runqueue without overloading it.
> >
> > For the schedutil part, see the changelog of the commit that introduced it:
> >
> > 9bdcb44e391d cpufreq: schedutil: New governor based on scheduler
> > utilization data
> >
> > As for the other places, I don't know about the exact reasoning.
> >
> > > Quentin suggested to use common code for this calculation and that is what is
> > > getting discussed here.
> >
> > I guess if the rationale for the formula is the same in all cases, it
> > would be good to consolidate that code and document the rationale
> > while at it.
>
> I _think_ it is, but I guess others could correct me if this is
> incorrect.
>
> When choosing a CPU or a frequency using a util value, we look for a
> capacity that will provide us with 20% of idle time. And in both case we
> use the same threshold, just hardcoded in different places. Hence the
> suggestion to unify things.
>
> I hope that makes sense :-)

Well, for schedutil, the 1.25 value comes from the case when
utilization is not frequency-invariant  the next-frequency formula is
recursive (the next frequency is proportional to the current one).  It
is chosen to get the new frequency equal to the old one if (util /
max) is .8.  That translates to the "capacity that will provide 20%
more of idle time" in the frequency-invariant utilization case, but
the original rationale was different.

Re: [PATCH] sched/fair: Introduce fits_capacity()

[Quentin Perret]

On Tuesday 18 Jun 2019 at 10:36:56 (+0200), Rafael J. Wysocki wrote:
> On Tue, Jun 18, 2019 at 10:25 AM Quentin Perret <quentin.perret@arm.com> wrote:
> >
> > On Tuesday 18 Jun 2019 at 10:10:48 (+0200), Rafael J. Wysocki wrote:
> > > On Tue, Jun 18, 2019 at 9:47 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > > >
> > > > On 18-06-19, 09:26, Rafael J. Wysocki wrote:
> > > > > On Tue, Jun 18, 2019 at 5:12 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > > > > >
> > > > > > +Rafael
> > > > > >
> > > > > > On 17-06-19, 17:02, Peter Zijlstra wrote:
> > > > > > > On Thu, Jun 06, 2019 at 08:22:04AM +0530, Viresh Kumar wrote:
> > > > > > > > Hmm, even if the values are same currently I am not sure if we want
> > > > > > > > the same for ever. I will write a patch for it though, if Peter/Rafael
> > > > > > > > feel the same as you.
> > > > > > >
> > > > > > > Is it really the same variable or just two numbers that happen to be the
> > > > > > > same?
> > > > > >
> > > > > > In both cases we are trying to keep the load under 80% of what can be supported.
> > > > > > But I am not sure of the answer to your question.
> > > > > >
> > > > > > Maybe Rafael knows :)
> > > > >
> > > > > Which variable?
> > > >
> > > > Schedutil multiplies the target frequency by 1.25 (20% more capacity eventually)
> > > > to get enough room for more load and similar thing is done in fair.c at several
> > > > places to see if the new task can fit in a runqueue without overloading it.
> > >
> > > For the schedutil part, see the changelog of the commit that introduced it:
> > >
> > > 9bdcb44e391d cpufreq: schedutil: New governor based on scheduler
> > > utilization data
> > >
> > > As for the other places, I don't know about the exact reasoning.
> > >
> > > > Quentin suggested to use common code for this calculation and that is what is
> > > > getting discussed here.
> > >
> > > I guess if the rationale for the formula is the same in all cases, it
> > > would be good to consolidate that code and document the rationale
> > > while at it.
> >
> > I _think_ it is, but I guess others could correct me if this is
> > incorrect.
> >
> > When choosing a CPU or a frequency using a util value, we look for a
> > capacity that will provide us with 20% of idle time. And in both case we
> > use the same threshold, just hardcoded in different places. Hence the
> > suggestion to unify things.
> >
> > I hope that makes sense :-)
> 
> Well, for schedutil, the 1.25 value comes from the case when
> utilization is not frequency-invariant  the next-frequency formula is
> recursive (the next frequency is proportional to the current one).  It
> is chosen to get the new frequency equal to the old one if (util /
> max) is .8.  That translates to the "capacity that will provide 20%
> more of idle time" in the frequency-invariant utilization case, but
> the original rationale was different.

OK, thanks, I wasn't aware of this. I understood it the other way
around, but re-reading the commit message you shared earlier this makes
sense.

I guess it is also worth mentioning that, to the best of my knowledge,
the vast majority of real-world sugov users are in fact frequency
invariant. So perhaps there is still a case for the code factorization
suggested earlier. But in the end it is really just a cleanup to help
maintainability, so if you guys don't buy in there is no point pushing
further :-)

Thanks,
Quentin

Re: [PATCH] sched/fair: Introduce fits_capacity()

[Viresh Kumar]

On 04-06-19, 12:31, Viresh Kumar wrote:
> The same formula to check utilization against capacity (after
> considering capacity_margin) is already used at 5 different locations.
> 
> This patch creates a new macro, fits_capacity(), which can be used from
> all these locations without exposing the details of it and hence
> simplify code.
> 
> All the 5 code locations are updated as well to use it..
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  kernel/sched/fair.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)

@Peter: Do you suggest any modifications to this patch? Do I need to
resend it ?

-- 
viresh

Re: [PATCH] sched/fair: Introduce fits_capacity()

[Peter Zijlstra]

On Wed, Jul 17, 2019 at 04:03:56PM +0530, Viresh Kumar wrote:
> On 04-06-19, 12:31, Viresh Kumar wrote:
> > The same formula to check utilization against capacity (after
> > considering capacity_margin) is already used at 5 different locations.
> > 
> > This patch creates a new macro, fits_capacity(), which can be used from
> > all these locations without exposing the details of it and hence
> > simplify code.
> > 
> > All the 5 code locations are updated as well to use it..
> > 
> > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> > ---
> >  kernel/sched/fair.c | 14 +++++++-------
> >  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> @Peter: Do you suggest any modifications to this patch? Do I need to
> resend it ?

Ah, I've picked it up with a few (small) edits.

Thanks!

---
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -96,12 +96,12 @@ int __weak arch_asym_cpu_priority(int cp
 }
 
 /*
- * The margin used when comparing utilization with CPU capacity:
- * util * margin < capacity * 1024
+ * The margin used when comparing utilization with CPU capacity.
  *
  * (default: ~20%)
  */
-static unsigned int capacity_margin			= 1280;
+#define fits_capacity(cap, max)	((cap) * 1280 < (max) * 1024)
+
 #endif
 
 #ifdef CONFIG_CFS_BANDWIDTH
@@ -3754,7 +3754,7 @@ util_est_dequeue(struct cfs_rq *cfs_rq,
 
 static inline int task_fits_capacity(struct task_struct *p, long capacity)
 {
-	return capacity * 1024 > task_util_est(p) * capacity_margin;
+	return fits_capacity(task_util_est(p), capacity);
 }
 
 static inline void update_misfit_status(struct task_struct *p, struct rq *rq)
@@ -5181,7 +5181,7 @@ static inline unsigned long cpu_util(int
 
 static inline bool cpu_overutilized(int cpu)
 {
-	return (capacity_of(cpu) * 1024) < (cpu_util(cpu) * capacity_margin);
+	return !fits_capacity(cpu_util(cpu), capacity_of(cpu));
 }
 
 static inline void update_overutilized_status(struct rq *rq)
@@ -6402,7 +6402,7 @@ static int find_energy_efficient_cpu(str
 			/* Skip CPUs that will be overutilized. */
 			util = cpu_util_next(cpu, p, cpu);
 			cpu_cap = capacity_of(cpu);
-			if (cpu_cap * 1024 < util * capacity_margin)
+			if (!fits_capacity(util, cpu_cap))
 				continue;
 
 			/* Always use prev_cpu as a candidate. */
@@ -7957,8 +7957,7 @@ group_is_overloaded(struct lb_env *env,
 static inline bool
 group_smaller_min_cpu_capacity(struct sched_group *sg, struct sched_group *ref)
 {
-	return sg->sgc->min_capacity * capacity_margin <
-						ref->sgc->min_capacity * 1024;
+	return fits_capacity(sg->sgc->min_capacity, ref->sgc->min_capacity);
 }
 
 /*
@@ -7968,8 +7967,7 @@ group_smaller_min_cpu_capacity(struct sc
 static inline bool
 group_smaller_max_cpu_capacity(struct sched_group *sg, struct sched_group *ref)
 {
-	return sg->sgc->max_capacity * capacity_margin <
-						ref->sgc->max_capacity * 1024;
+	return fits_capacity(sg->sgc->max_capacity, ref->sgc->max_capacity);
 }
 
 static inline enum

[tip:sched/core] sched/fair: Introduce fits_capacity()

[tip-bot for Viresh Kumar]

Commit-ID:  60e17f5cef838e9ca7946ced208ceddcec6c315d
Gitweb:     https://git.kernel.org/tip/60e17f5cef838e9ca7946ced208ceddcec6c315d
Author:     Viresh Kumar <viresh.kumar@linaro.org>
AuthorDate: Tue, 4 Jun 2019 12:31:52 +0530
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Thu, 25 Jul 2019 15:51:56 +0200

sched/fair: Introduce fits_capacity()

The same formula to check utilization against capacity (after
considering capacity_margin) is already used at 5 different locations.

This patch creates a new macro, fits_capacity(), which can be used from
all these locations without exposing the details of it and hence
simplify code.

All the 5 code locations are updated as well to use it..

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Vincent Guittot <vincent.guittot@linaro.org>
Link: https://lkml.kernel.org/r/b477ac75a2b163048bdaeb37f57b4c3f04f75a31.1559631700.git.viresh.kumar@linaro.org
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 kernel/sched/fair.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 52564e050062..fb75c0bea80f 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -96,12 +96,12 @@ int __weak arch_asym_cpu_priority(int cpu)
 }
 
 /*
- * The margin used when comparing utilization with CPU capacity:
- * util * margin < capacity * 1024
+ * The margin used when comparing utilization with CPU capacity.
  *
  * (default: ~20%)
  */
-static unsigned int capacity_margin			= 1280;
+#define fits_capacity(cap, max)	((cap) * 1280 < (max) * 1024)
+
 #endif
 
 #ifdef CONFIG_CFS_BANDWIDTH
@@ -3808,7 +3808,7 @@ util_est_dequeue(struct cfs_rq *cfs_rq, struct task_struct *p, bool task_sleep)
 
 static inline int task_fits_capacity(struct task_struct *p, long capacity)
 {
-	return capacity * 1024 > task_util_est(p) * capacity_margin;
+	return fits_capacity(task_util_est(p), capacity);
 }
 
 static inline void update_misfit_status(struct task_struct *p, struct rq *rq)
@@ -5235,7 +5235,7 @@ static inline unsigned long cpu_util(int cpu);
 
 static inline bool cpu_overutilized(int cpu)
 {
-	return (capacity_of(cpu) * 1024) < (cpu_util(cpu) * capacity_margin);
+	return !fits_capacity(cpu_util(cpu), capacity_of(cpu));
 }
 
 static inline void update_overutilized_status(struct rq *rq)
@@ -6456,7 +6456,7 @@ static int find_energy_efficient_cpu(struct task_struct *p, int prev_cpu)
 			/* Skip CPUs that will be overutilized. */
 			util = cpu_util_next(cpu, p, cpu);
 			cpu_cap = capacity_of(cpu);
-			if (cpu_cap * 1024 < util * capacity_margin)
+			if (!fits_capacity(util, cpu_cap))
 				continue;
 
 			/* Always use prev_cpu as a candidate. */
@@ -8011,8 +8011,7 @@ group_is_overloaded(struct lb_env *env, struct sg_lb_stats *sgs)
 static inline bool
 group_smaller_min_cpu_capacity(struct sched_group *sg, struct sched_group *ref)
 {
-	return sg->sgc->min_capacity * capacity_margin <
-						ref->sgc->min_capacity * 1024;
+	return fits_capacity(sg->sgc->min_capacity, ref->sgc->min_capacity);
 }
 
 /*
@@ -8022,8 +8021,7 @@ group_smaller_min_cpu_capacity(struct sched_group *sg, struct sched_group *ref)
 static inline bool
 group_smaller_max_cpu_capacity(struct sched_group *sg, struct sched_group *ref)
 {
-	return sg->sgc->max_capacity * capacity_margin <
-						ref->sgc->max_capacity * 1024;
+	return fits_capacity(sg->sgc->max_capacity, ref->sgc->max_capacity);
 }
 
 static inline enum