[PATCH] sched: cpufreq: Rename map_util_perf to apply_dvfs_headroom

[PATCH] sched: cpufreq: Rename map_util_perf to apply_dvfs_headroom

[Qais Yousef]

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

Re: [PATCH] sched: cpufreq: Rename map_util_perf to apply_dvfs_headroom

[Viresh Kumar]

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

Re: [PATCH] sched: cpufreq: Rename map_util_perf to apply_dvfs_headroom

[Rafael J. Wysocki]

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>

Re: [PATCH] sched: cpufreq: Rename map_util_perf to apply_dvfs_headroom

[Vincent Guittot]

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
>

Re: [PATCH] sched: cpufreq: Rename map_util_perf to apply_dvfs_headroom

[Qais Yousef]

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

Re: [PATCH] sched: cpufreq: Rename map_util_perf to apply_dvfs_headroom

[Qais Yousef]

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

Re: [PATCH] sched: cpufreq: Rename map_util_perf to apply_dvfs_headroom

[Vincent Guittot]

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

Re: [PATCH] sched: cpufreq: Rename map_util_perf to apply_dvfs_headroom

[Qais Yousef]

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