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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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 <[email protected]>
--
viresh
On Mon, Feb 5, 2024 at 3:20 AM Qais Yousef <[email protected]> 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 <[email protected]>
> ---
> 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 <[email protected]>
On Mon, 5 Feb 2024 at 03:20, Qais Yousef <[email protected]> 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 <[email protected]>
> ---
> 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
>
On 02/14/24 08:32, Vincent Guittot wrote:
> On Mon, 5 Feb 2024 at 03:20, Qais Yousef <[email protected]> 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 <[email protected]>
> > ---
> > 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
> >
On 02/12/24 16:54, Rafael J. Wysocki wrote:
> On Mon, Feb 5, 2024 at 3:20 AM Qais Yousef <[email protected]> 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 <[email protected]>
> > ---
> > 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 <[email protected]>
Thanks
On Tue, 20 Feb 2024 at 14:57, Qais Yousef <[email protected]> wrote:
>
> On 02/14/24 08:32, Vincent Guittot wrote:
> > On Mon, 5 Feb 2024 at 03:20, Qais Yousef <[email protected]> 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 <[email protected]>
> > > ---
> > > 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
> > >
On 02/20/24 15:22, Vincent Guittot wrote:
> On Tue, 20 Feb 2024 at 14:57, Qais Yousef <[email protected]> wrote:
> >
> > On 02/14/24 08:32, Vincent Guittot wrote:
> > > On Mon, 5 Feb 2024 at 03:20, Qais Yousef <[email protected]> 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 <[email protected]>
> > > > ---
> > > > 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
> > > >