* Re: [PATCH] sched: cpufreq: Rename map_util_perf to apply_dvfs_headroom
2024-02-05 2:20 [PATCH] sched: cpufreq: Rename map_util_perf to apply_dvfs_headroom Qais Yousef
@ 2024-02-05 7:41 ` Viresh Kumar
2024-02-12 15:54 ` Rafael J. Wysocki
2024-02-14 7:32 ` Vincent Guittot
2 siblings, 0 replies; 8+ messages in thread
From: Viresh Kumar @ 2024-02-05 7:41 UTC (permalink / raw)
To: Qais Yousef
Cc: Ingo Molnar, Peter Zijlstra, Rafael J. Wysocki, Vincent Guittot,
Dietmar Eggemann, linux-kernel, linux-pm
On 05-02-24, 02:20, Qais Yousef wrote:
> We are providing headroom for the utilization to grow until the next
> decision point to pick the next frequency. Give the function a better
> name and give it some documentation. It is not really mapping anything.
>
> Also move it to sched.h. This function relies on updating util signal
> appropriately to give a headroom to grow. This is more of a scheduler
> functionality than cpufreq. Move it to sched.h where all the other util
> handling code belongs.
>
> Signed-off-by: Qais Yousef <qyousef@layalina.io>
> ---
> include/linux/sched/cpufreq.h | 5 -----
> kernel/sched/cpufreq_schedutil.c | 2 +-
> kernel/sched/sched.h | 17 +++++++++++++++++
> 3 files changed, 18 insertions(+), 6 deletions(-)
>
> diff --git a/include/linux/sched/cpufreq.h b/include/linux/sched/cpufreq.h
> index bdd31ab93bc5..d01755d3142f 100644
> --- a/include/linux/sched/cpufreq.h
> +++ b/include/linux/sched/cpufreq.h
> @@ -28,11 +28,6 @@ static inline unsigned long map_util_freq(unsigned long util,
> {
> return freq * util / cap;
> }
> -
> -static inline unsigned long map_util_perf(unsigned long util)
> -{
> - return util + (util >> 2);
> -}
> #endif /* CONFIG_CPU_FREQ */
>
> #endif /* _LINUX_SCHED_CPUFREQ_H */
> diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
> index 95c3c097083e..abbd1ddb0359 100644
> --- a/kernel/sched/cpufreq_schedutil.c
> +++ b/kernel/sched/cpufreq_schedutil.c
> @@ -179,7 +179,7 @@ unsigned long sugov_effective_cpu_perf(int cpu, unsigned long actual,
> unsigned long max)
> {
> /* Add dvfs headroom to actual utilization */
> - actual = map_util_perf(actual);
> + actual = apply_dvfs_headroom(actual);
> /* Actually we don't need to target the max performance */
> if (actual < max)
> max = actual;
> diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
> index e58a54bda77d..0da3425200b1 100644
> --- a/kernel/sched/sched.h
> +++ b/kernel/sched/sched.h
> @@ -3002,6 +3002,23 @@ unsigned long sugov_effective_cpu_perf(int cpu, unsigned long actual,
> unsigned long min,
> unsigned long max);
>
> +/*
> + * DVFS decision are made at discrete points. If CPU stays busy, the util will
> + * continue to grow, which means it could need to run at a higher frequency
> + * before the next decision point was reached. IOW, we can't follow the util as
> + * it grows immediately, but there's a delay before we issue a request to go to
> + * higher frequency. The headroom caters for this delay so the system continues
> + * to run at adequate performance point.
> + *
> + * This function provides enough headroom to provide adequate performance
> + * assuming the CPU continues to be busy.
> + *
> + * At the moment it is a constant multiplication with 1.25.
> + */
> +static inline unsigned long apply_dvfs_headroom(unsigned long util)
> +{
> + return util + (util >> 2);
> +}
>
> /*
> * Verify the fitness of task @p to run on @cpu taking into account the
Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
--
viresh
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] sched: cpufreq: Rename map_util_perf to apply_dvfs_headroom
2024-02-05 2:20 [PATCH] sched: cpufreq: Rename map_util_perf to apply_dvfs_headroom Qais Yousef
2024-02-05 7:41 ` Viresh Kumar
@ 2024-02-12 15:54 ` Rafael J. Wysocki
2024-02-20 13:58 ` Qais Yousef
2024-02-14 7:32 ` Vincent Guittot
2 siblings, 1 reply; 8+ messages in thread
From: Rafael J. Wysocki @ 2024-02-12 15:54 UTC (permalink / raw)
To: Qais Yousef
Cc: Ingo Molnar, Peter Zijlstra, Rafael J. Wysocki, Viresh Kumar,
Vincent Guittot, Dietmar Eggemann, linux-kernel, linux-pm
On Mon, Feb 5, 2024 at 3:20 AM Qais Yousef <qyousef@layalina.io> wrote:
>
> We are providing headroom for the utilization to grow until the next
> decision point to pick the next frequency. Give the function a better
> name and give it some documentation. It is not really mapping anything.
>
> Also move it to sched.h. This function relies on updating util signal
> appropriately to give a headroom to grow. This is more of a scheduler
> functionality than cpufreq. Move it to sched.h where all the other util
> handling code belongs.
>
> Signed-off-by: Qais Yousef <qyousef@layalina.io>
> ---
> include/linux/sched/cpufreq.h | 5 -----
> kernel/sched/cpufreq_schedutil.c | 2 +-
> kernel/sched/sched.h | 17 +++++++++++++++++
> 3 files changed, 18 insertions(+), 6 deletions(-)
>
> diff --git a/include/linux/sched/cpufreq.h b/include/linux/sched/cpufreq.h
> index bdd31ab93bc5..d01755d3142f 100644
> --- a/include/linux/sched/cpufreq.h
> +++ b/include/linux/sched/cpufreq.h
> @@ -28,11 +28,6 @@ static inline unsigned long map_util_freq(unsigned long util,
> {
> return freq * util / cap;
> }
> -
> -static inline unsigned long map_util_perf(unsigned long util)
> -{
> - return util + (util >> 2);
> -}
> #endif /* CONFIG_CPU_FREQ */
>
> #endif /* _LINUX_SCHED_CPUFREQ_H */
> diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
> index 95c3c097083e..abbd1ddb0359 100644
> --- a/kernel/sched/cpufreq_schedutil.c
> +++ b/kernel/sched/cpufreq_schedutil.c
> @@ -179,7 +179,7 @@ unsigned long sugov_effective_cpu_perf(int cpu, unsigned long actual,
> unsigned long max)
> {
> /* Add dvfs headroom to actual utilization */
> - actual = map_util_perf(actual);
> + actual = apply_dvfs_headroom(actual);
> /* Actually we don't need to target the max performance */
> if (actual < max)
> max = actual;
> diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
> index e58a54bda77d..0da3425200b1 100644
> --- a/kernel/sched/sched.h
> +++ b/kernel/sched/sched.h
> @@ -3002,6 +3002,23 @@ unsigned long sugov_effective_cpu_perf(int cpu, unsigned long actual,
> unsigned long min,
> unsigned long max);
>
> +/*
> + * DVFS decision are made at discrete points. If CPU stays busy, the util will
> + * continue to grow, which means it could need to run at a higher frequency
> + * before the next decision point was reached. IOW, we can't follow the util as
> + * it grows immediately, but there's a delay before we issue a request to go to
> + * higher frequency. The headroom caters for this delay so the system continues
> + * to run at adequate performance point.
> + *
> + * This function provides enough headroom to provide adequate performance
> + * assuming the CPU continues to be busy.
> + *
> + * At the moment it is a constant multiplication with 1.25.
> + */
> +static inline unsigned long apply_dvfs_headroom(unsigned long util)
> +{
> + return util + (util >> 2);
> +}
>
> /*
> * Verify the fitness of task @p to run on @cpu taking into account the
> --
This touches sched.h, so I'd prefer it to go in via tip and
Acked-by: Rafael J. Wysocki <rafael@kernel.org>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] sched: cpufreq: Rename map_util_perf to apply_dvfs_headroom
2024-02-12 15:54 ` Rafael J. Wysocki
@ 2024-02-20 13:58 ` Qais Yousef
0 siblings, 0 replies; 8+ messages in thread
From: Qais Yousef @ 2024-02-20 13:58 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Ingo Molnar, Peter Zijlstra, Viresh Kumar, Vincent Guittot,
Dietmar Eggemann, linux-kernel, linux-pm
On 02/12/24 16:54, Rafael J. Wysocki wrote:
> On Mon, Feb 5, 2024 at 3:20 AM Qais Yousef <qyousef@layalina.io> wrote:
> >
> > We are providing headroom for the utilization to grow until the next
> > decision point to pick the next frequency. Give the function a better
> > name and give it some documentation. It is not really mapping anything.
> >
> > Also move it to sched.h. This function relies on updating util signal
> > appropriately to give a headroom to grow. This is more of a scheduler
> > functionality than cpufreq. Move it to sched.h where all the other util
> > handling code belongs.
> >
> > Signed-off-by: Qais Yousef <qyousef@layalina.io>
> > ---
> > include/linux/sched/cpufreq.h | 5 -----
> > kernel/sched/cpufreq_schedutil.c | 2 +-
> > kernel/sched/sched.h | 17 +++++++++++++++++
> > 3 files changed, 18 insertions(+), 6 deletions(-)
> >
> > diff --git a/include/linux/sched/cpufreq.h b/include/linux/sched/cpufreq.h
> > index bdd31ab93bc5..d01755d3142f 100644
> > --- a/include/linux/sched/cpufreq.h
> > +++ b/include/linux/sched/cpufreq.h
> > @@ -28,11 +28,6 @@ static inline unsigned long map_util_freq(unsigned long util,
> > {
> > return freq * util / cap;
> > }
> > -
> > -static inline unsigned long map_util_perf(unsigned long util)
> > -{
> > - return util + (util >> 2);
> > -}
> > #endif /* CONFIG_CPU_FREQ */
> >
> > #endif /* _LINUX_SCHED_CPUFREQ_H */
> > diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
> > index 95c3c097083e..abbd1ddb0359 100644
> > --- a/kernel/sched/cpufreq_schedutil.c
> > +++ b/kernel/sched/cpufreq_schedutil.c
> > @@ -179,7 +179,7 @@ unsigned long sugov_effective_cpu_perf(int cpu, unsigned long actual,
> > unsigned long max)
> > {
> > /* Add dvfs headroom to actual utilization */
> > - actual = map_util_perf(actual);
> > + actual = apply_dvfs_headroom(actual);
> > /* Actually we don't need to target the max performance */
> > if (actual < max)
> > max = actual;
> > diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
> > index e58a54bda77d..0da3425200b1 100644
> > --- a/kernel/sched/sched.h
> > +++ b/kernel/sched/sched.h
> > @@ -3002,6 +3002,23 @@ unsigned long sugov_effective_cpu_perf(int cpu, unsigned long actual,
> > unsigned long min,
> > unsigned long max);
> >
> > +/*
> > + * DVFS decision are made at discrete points. If CPU stays busy, the util will
> > + * continue to grow, which means it could need to run at a higher frequency
> > + * before the next decision point was reached. IOW, we can't follow the util as
> > + * it grows immediately, but there's a delay before we issue a request to go to
> > + * higher frequency. The headroom caters for this delay so the system continues
> > + * to run at adequate performance point.
> > + *
> > + * This function provides enough headroom to provide adequate performance
> > + * assuming the CPU continues to be busy.
> > + *
> > + * At the moment it is a constant multiplication with 1.25.
> > + */
> > +static inline unsigned long apply_dvfs_headroom(unsigned long util)
> > +{
> > + return util + (util >> 2);
> > +}
> >
> > /*
> > * Verify the fitness of task @p to run on @cpu taking into account the
> > --
>
> This touches sched.h, so I'd prefer it to go in via tip and
>
> Acked-by: Rafael J. Wysocki <rafael@kernel.org>
Thanks
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] sched: cpufreq: Rename map_util_perf to apply_dvfs_headroom
2024-02-05 2:20 [PATCH] sched: cpufreq: Rename map_util_perf to apply_dvfs_headroom Qais Yousef
2024-02-05 7:41 ` Viresh Kumar
2024-02-12 15:54 ` Rafael J. Wysocki
@ 2024-02-14 7:32 ` Vincent Guittot
2024-02-20 13:57 ` Qais Yousef
2 siblings, 1 reply; 8+ messages in thread
From: Vincent Guittot @ 2024-02-14 7:32 UTC (permalink / raw)
To: Qais Yousef
Cc: Ingo Molnar, Peter Zijlstra, Rafael J. Wysocki, Viresh Kumar,
Dietmar Eggemann, linux-kernel, linux-pm
On Mon, 5 Feb 2024 at 03:20, Qais Yousef <qyousef@layalina.io> wrote:
>
> We are providing headroom for the utilization to grow until the next
> decision point to pick the next frequency. Give the function a better
> name and give it some documentation. It is not really mapping anything.
The renaming makes sense.
>
> Also move it to sched.h. This function relies on updating util signal
I don't see the benefit of moving it the sched.h as it is only used by
cpufreq_schedutil()
> appropriately to give a headroom to grow. This is more of a scheduler
> functionality than cpufreq. Move it to sched.h where all the other util
> handling code belongs.
>
> Signed-off-by: Qais Yousef <qyousef@layalina.io>
> ---
> include/linux/sched/cpufreq.h | 5 -----
> kernel/sched/cpufreq_schedutil.c | 2 +-
> kernel/sched/sched.h | 17 +++++++++++++++++
> 3 files changed, 18 insertions(+), 6 deletions(-)
>
> diff --git a/include/linux/sched/cpufreq.h b/include/linux/sched/cpufreq.h
> index bdd31ab93bc5..d01755d3142f 100644
> --- a/include/linux/sched/cpufreq.h
> +++ b/include/linux/sched/cpufreq.h
> @@ -28,11 +28,6 @@ static inline unsigned long map_util_freq(unsigned long util,
> {
> return freq * util / cap;
> }
> -
> -static inline unsigned long map_util_perf(unsigned long util)
> -{
> - return util + (util >> 2);
> -}
> #endif /* CONFIG_CPU_FREQ */
>
> #endif /* _LINUX_SCHED_CPUFREQ_H */
> diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
> index 95c3c097083e..abbd1ddb0359 100644
> --- a/kernel/sched/cpufreq_schedutil.c
> +++ b/kernel/sched/cpufreq_schedutil.c
> @@ -179,7 +179,7 @@ unsigned long sugov_effective_cpu_perf(int cpu, unsigned long actual,
> unsigned long max)
> {
> /* Add dvfs headroom to actual utilization */
> - actual = map_util_perf(actual);
> + actual = apply_dvfs_headroom(actual);
> /* Actually we don't need to target the max performance */
> if (actual < max)
> max = actual;
> diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
> index e58a54bda77d..0da3425200b1 100644
> --- a/kernel/sched/sched.h
> +++ b/kernel/sched/sched.h
> @@ -3002,6 +3002,23 @@ unsigned long sugov_effective_cpu_perf(int cpu, unsigned long actual,
> unsigned long min,
> unsigned long max);
>
> +/*
> + * DVFS decision are made at discrete points. If CPU stays busy, the util will
> + * continue to grow, which means it could need to run at a higher frequency
> + * before the next decision point was reached. IOW, we can't follow the util as
> + * it grows immediately, but there's a delay before we issue a request to go to
> + * higher frequency. The headroom caters for this delay so the system continues
> + * to run at adequate performance point.
> + *
> + * This function provides enough headroom to provide adequate performance
> + * assuming the CPU continues to be busy.
> + *
> + * At the moment it is a constant multiplication with 1.25.
> + */
> +static inline unsigned long apply_dvfs_headroom(unsigned long util)
> +{
> + return util + (util >> 2);
> +}
>
> /*
> * Verify the fitness of task @p to run on @cpu taking into account the
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] sched: cpufreq: Rename map_util_perf to apply_dvfs_headroom
2024-02-14 7:32 ` Vincent Guittot
@ 2024-02-20 13:57 ` Qais Yousef
2024-02-20 14:22 ` Vincent Guittot
0 siblings, 1 reply; 8+ messages in thread
From: Qais Yousef @ 2024-02-20 13:57 UTC (permalink / raw)
To: Vincent Guittot
Cc: Ingo Molnar, Peter Zijlstra, Rafael J. Wysocki, Viresh Kumar,
Dietmar Eggemann, linux-kernel, linux-pm
On 02/14/24 08:32, Vincent Guittot wrote:
> On Mon, 5 Feb 2024 at 03:20, Qais Yousef <qyousef@layalina.io> wrote:
> >
> > We are providing headroom for the utilization to grow until the next
> > decision point to pick the next frequency. Give the function a better
> > name and give it some documentation. It is not really mapping anything.
>
> The renaming makes sense.
>
> >
> > Also move it to sched.h. This function relies on updating util signal
>
> I don't see the benefit of moving it the sched.h as it is only used by
> cpufreq_schedutil()
Hehe what's for me the reason to move it for you it's the reason not to :-)
(I believe you meant cpufreq_schedutil.c)
It doesn't make sense outside of schedutil, does it? I can't see it being
suitable for consumption by other governors for example as it is not generic
enough.
And the headroom definition needs to evolve. And the tight coupling to util
which is a scheduler internal metric will make it hard once it's part of
cpufreq. The headroom IMO is a property of the governor.
We can defer the moving for now if you insist. But I think it's inevitable?
>
>
> > appropriately to give a headroom to grow. This is more of a scheduler
> > functionality than cpufreq. Move it to sched.h where all the other util
> > handling code belongs.
> >
> > Signed-off-by: Qais Yousef <qyousef@layalina.io>
> > ---
> > include/linux/sched/cpufreq.h | 5 -----
> > kernel/sched/cpufreq_schedutil.c | 2 +-
> > kernel/sched/sched.h | 17 +++++++++++++++++
> > 3 files changed, 18 insertions(+), 6 deletions(-)
> >
> > diff --git a/include/linux/sched/cpufreq.h b/include/linux/sched/cpufreq.h
> > index bdd31ab93bc5..d01755d3142f 100644
> > --- a/include/linux/sched/cpufreq.h
> > +++ b/include/linux/sched/cpufreq.h
> > @@ -28,11 +28,6 @@ static inline unsigned long map_util_freq(unsigned long util,
> > {
> > return freq * util / cap;
> > }
> > -
> > -static inline unsigned long map_util_perf(unsigned long util)
> > -{
> > - return util + (util >> 2);
> > -}
> > #endif /* CONFIG_CPU_FREQ */
> >
> > #endif /* _LINUX_SCHED_CPUFREQ_H */
> > diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
> > index 95c3c097083e..abbd1ddb0359 100644
> > --- a/kernel/sched/cpufreq_schedutil.c
> > +++ b/kernel/sched/cpufreq_schedutil.c
> > @@ -179,7 +179,7 @@ unsigned long sugov_effective_cpu_perf(int cpu, unsigned long actual,
> > unsigned long max)
> > {
> > /* Add dvfs headroom to actual utilization */
> > - actual = map_util_perf(actual);
> > + actual = apply_dvfs_headroom(actual);
> > /* Actually we don't need to target the max performance */
> > if (actual < max)
> > max = actual;
> > diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
> > index e58a54bda77d..0da3425200b1 100644
> > --- a/kernel/sched/sched.h
> > +++ b/kernel/sched/sched.h
> > @@ -3002,6 +3002,23 @@ unsigned long sugov_effective_cpu_perf(int cpu, unsigned long actual,
> > unsigned long min,
> > unsigned long max);
> >
> > +/*
> > + * DVFS decision are made at discrete points. If CPU stays busy, the util will
> > + * continue to grow, which means it could need to run at a higher frequency
> > + * before the next decision point was reached. IOW, we can't follow the util as
> > + * it grows immediately, but there's a delay before we issue a request to go to
> > + * higher frequency. The headroom caters for this delay so the system continues
> > + * to run at adequate performance point.
> > + *
> > + * This function provides enough headroom to provide adequate performance
> > + * assuming the CPU continues to be busy.
> > + *
> > + * At the moment it is a constant multiplication with 1.25.
> > + */
> > +static inline unsigned long apply_dvfs_headroom(unsigned long util)
> > +{
> > + return util + (util >> 2);
> > +}
> >
> > /*
> > * Verify the fitness of task @p to run on @cpu taking into account the
> > --
> > 2.34.1
> >
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] sched: cpufreq: Rename map_util_perf to apply_dvfs_headroom
2024-02-20 13:57 ` Qais Yousef
@ 2024-02-20 14:22 ` Vincent Guittot
2024-02-20 14:28 ` Qais Yousef
0 siblings, 1 reply; 8+ messages in thread
From: Vincent Guittot @ 2024-02-20 14:22 UTC (permalink / raw)
To: Qais Yousef
Cc: Ingo Molnar, Peter Zijlstra, Rafael J. Wysocki, Viresh Kumar,
Dietmar Eggemann, linux-kernel, linux-pm
On Tue, 20 Feb 2024 at 14:57, Qais Yousef <qyousef@layalina.io> wrote:
>
> On 02/14/24 08:32, Vincent Guittot wrote:
> > On Mon, 5 Feb 2024 at 03:20, Qais Yousef <qyousef@layalina.io> wrote:
> > >
> > > We are providing headroom for the utilization to grow until the next
> > > decision point to pick the next frequency. Give the function a better
> > > name and give it some documentation. It is not really mapping anything.
> >
> > The renaming makes sense.
> >
> > >
> > > Also move it to sched.h. This function relies on updating util signal
> >
> > I don't see the benefit of moving it the sched.h as it is only used by
> > cpufreq_schedutil()
>
> Hehe what's for me the reason to move it for you it's the reason not to :-)
>
> (I believe you meant cpufreq_schedutil.c)
>
> It doesn't make sense outside of schedutil, does it? I can't see it being
> suitable for consumption by other governors for example as it is not generic
> enough.
>
> And the headroom definition needs to evolve. And the tight coupling to util
> which is a scheduler internal metric will make it hard once it's part of
> cpufreq. The headroom IMO is a property of the governor.
In this case make it part of cpufreq_schedutil.c if this is the
governor that can use it. I don't like sched.h because It gives the
impression that scheduler can play with it whereas it's a property of
the cpufreq governor
>
> We can defer the moving for now if you insist. But I think it's inevitable?
>
> >
> >
> > > appropriately to give a headroom to grow. This is more of a scheduler
> > > functionality than cpufreq. Move it to sched.h where all the other util
> > > handling code belongs.
> > >
> > > Signed-off-by: Qais Yousef <qyousef@layalina.io>
> > > ---
> > > include/linux/sched/cpufreq.h | 5 -----
> > > kernel/sched/cpufreq_schedutil.c | 2 +-
> > > kernel/sched/sched.h | 17 +++++++++++++++++
> > > 3 files changed, 18 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/include/linux/sched/cpufreq.h b/include/linux/sched/cpufreq.h
> > > index bdd31ab93bc5..d01755d3142f 100644
> > > --- a/include/linux/sched/cpufreq.h
> > > +++ b/include/linux/sched/cpufreq.h
> > > @@ -28,11 +28,6 @@ static inline unsigned long map_util_freq(unsigned long util,
> > > {
> > > return freq * util / cap;
> > > }
> > > -
> > > -static inline unsigned long map_util_perf(unsigned long util)
> > > -{
> > > - return util + (util >> 2);
> > > -}
> > > #endif /* CONFIG_CPU_FREQ */
> > >
> > > #endif /* _LINUX_SCHED_CPUFREQ_H */
> > > diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
> > > index 95c3c097083e..abbd1ddb0359 100644
> > > --- a/kernel/sched/cpufreq_schedutil.c
> > > +++ b/kernel/sched/cpufreq_schedutil.c
> > > @@ -179,7 +179,7 @@ unsigned long sugov_effective_cpu_perf(int cpu, unsigned long actual,
> > > unsigned long max)
> > > {
> > > /* Add dvfs headroom to actual utilization */
> > > - actual = map_util_perf(actual);
> > > + actual = apply_dvfs_headroom(actual);
> > > /* Actually we don't need to target the max performance */
> > > if (actual < max)
> > > max = actual;
> > > diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
> > > index e58a54bda77d..0da3425200b1 100644
> > > --- a/kernel/sched/sched.h
> > > +++ b/kernel/sched/sched.h
> > > @@ -3002,6 +3002,23 @@ unsigned long sugov_effective_cpu_perf(int cpu, unsigned long actual,
> > > unsigned long min,
> > > unsigned long max);
> > >
> > > +/*
> > > + * DVFS decision are made at discrete points. If CPU stays busy, the util will
> > > + * continue to grow, which means it could need to run at a higher frequency
> > > + * before the next decision point was reached. IOW, we can't follow the util as
> > > + * it grows immediately, but there's a delay before we issue a request to go to
> > > + * higher frequency. The headroom caters for this delay so the system continues
> > > + * to run at adequate performance point.
> > > + *
> > > + * This function provides enough headroom to provide adequate performance
> > > + * assuming the CPU continues to be busy.
> > > + *
> > > + * At the moment it is a constant multiplication with 1.25.
> > > + */
> > > +static inline unsigned long apply_dvfs_headroom(unsigned long util)
> > > +{
> > > + return util + (util >> 2);
> > > +}
> > >
> > > /*
> > > * Verify the fitness of task @p to run on @cpu taking into account the
> > > --
> > > 2.34.1
> > >
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] sched: cpufreq: Rename map_util_perf to apply_dvfs_headroom
2024-02-20 14:22 ` Vincent Guittot
@ 2024-02-20 14:28 ` Qais Yousef
0 siblings, 0 replies; 8+ messages in thread
From: Qais Yousef @ 2024-02-20 14:28 UTC (permalink / raw)
To: Vincent Guittot
Cc: Ingo Molnar, Peter Zijlstra, Rafael J. Wysocki, Viresh Kumar,
Dietmar Eggemann, linux-kernel, linux-pm
On 02/20/24 15:22, Vincent Guittot wrote:
> On Tue, 20 Feb 2024 at 14:57, Qais Yousef <qyousef@layalina.io> wrote:
> >
> > On 02/14/24 08:32, Vincent Guittot wrote:
> > > On Mon, 5 Feb 2024 at 03:20, Qais Yousef <qyousef@layalina.io> wrote:
> > > >
> > > > We are providing headroom for the utilization to grow until the next
> > > > decision point to pick the next frequency. Give the function a better
> > > > name and give it some documentation. It is not really mapping anything.
> > >
> > > The renaming makes sense.
> > >
> > > >
> > > > Also move it to sched.h. This function relies on updating util signal
> > >
> > > I don't see the benefit of moving it the sched.h as it is only used by
> > > cpufreq_schedutil()
> >
> > Hehe what's for me the reason to move it for you it's the reason not to :-)
> >
> > (I believe you meant cpufreq_schedutil.c)
> >
> > It doesn't make sense outside of schedutil, does it? I can't see it being
> > suitable for consumption by other governors for example as it is not generic
> > enough.
> >
> > And the headroom definition needs to evolve. And the tight coupling to util
> > which is a scheduler internal metric will make it hard once it's part of
> > cpufreq. The headroom IMO is a property of the governor.
>
> In this case make it part of cpufreq_schedutil.c if this is the
> governor that can use it. I don't like sched.h because It gives the
> impression that scheduler can play with it whereas it's a property of
> the cpufreq governor
Okay will do
Thanks!
>
> >
> > We can defer the moving for now if you insist. But I think it's inevitable?
> >
> > >
> > >
> > > > appropriately to give a headroom to grow. This is more of a scheduler
> > > > functionality than cpufreq. Move it to sched.h where all the other util
> > > > handling code belongs.
> > > >
> > > > Signed-off-by: Qais Yousef <qyousef@layalina.io>
> > > > ---
> > > > include/linux/sched/cpufreq.h | 5 -----
> > > > kernel/sched/cpufreq_schedutil.c | 2 +-
> > > > kernel/sched/sched.h | 17 +++++++++++++++++
> > > > 3 files changed, 18 insertions(+), 6 deletions(-)
> > > >
> > > > diff --git a/include/linux/sched/cpufreq.h b/include/linux/sched/cpufreq.h
> > > > index bdd31ab93bc5..d01755d3142f 100644
> > > > --- a/include/linux/sched/cpufreq.h
> > > > +++ b/include/linux/sched/cpufreq.h
> > > > @@ -28,11 +28,6 @@ static inline unsigned long map_util_freq(unsigned long util,
> > > > {
> > > > return freq * util / cap;
> > > > }
> > > > -
> > > > -static inline unsigned long map_util_perf(unsigned long util)
> > > > -{
> > > > - return util + (util >> 2);
> > > > -}
> > > > #endif /* CONFIG_CPU_FREQ */
> > > >
> > > > #endif /* _LINUX_SCHED_CPUFREQ_H */
> > > > diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
> > > > index 95c3c097083e..abbd1ddb0359 100644
> > > > --- a/kernel/sched/cpufreq_schedutil.c
> > > > +++ b/kernel/sched/cpufreq_schedutil.c
> > > > @@ -179,7 +179,7 @@ unsigned long sugov_effective_cpu_perf(int cpu, unsigned long actual,
> > > > unsigned long max)
> > > > {
> > > > /* Add dvfs headroom to actual utilization */
> > > > - actual = map_util_perf(actual);
> > > > + actual = apply_dvfs_headroom(actual);
> > > > /* Actually we don't need to target the max performance */
> > > > if (actual < max)
> > > > max = actual;
> > > > diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
> > > > index e58a54bda77d..0da3425200b1 100644
> > > > --- a/kernel/sched/sched.h
> > > > +++ b/kernel/sched/sched.h
> > > > @@ -3002,6 +3002,23 @@ unsigned long sugov_effective_cpu_perf(int cpu, unsigned long actual,
> > > > unsigned long min,
> > > > unsigned long max);
> > > >
> > > > +/*
> > > > + * DVFS decision are made at discrete points. If CPU stays busy, the util will
> > > > + * continue to grow, which means it could need to run at a higher frequency
> > > > + * before the next decision point was reached. IOW, we can't follow the util as
> > > > + * it grows immediately, but there's a delay before we issue a request to go to
> > > > + * higher frequency. The headroom caters for this delay so the system continues
> > > > + * to run at adequate performance point.
> > > > + *
> > > > + * This function provides enough headroom to provide adequate performance
> > > > + * assuming the CPU continues to be busy.
> > > > + *
> > > > + * At the moment it is a constant multiplication with 1.25.
> > > > + */
> > > > +static inline unsigned long apply_dvfs_headroom(unsigned long util)
> > > > +{
> > > > + return util + (util >> 2);
> > > > +}
> > > >
> > > > /*
> > > > * Verify the fitness of task @p to run on @cpu taking into account the
> > > > --
> > > > 2.34.1
> > > >
^ permalink raw reply [flat|nested] 8+ messages in thread