Re: [RFC PATCH 14/16] sched/schedutil: Ignore dvfs headroom when util is decaying

[Sultan Alsawaf (unemployed)]

On Tue, Aug 20, 2024 at 05:35:10PM +0100, Qais Yousef wrote:
> It means we're being idling or doing less work and are already running
> at a higher value. No need to apply any dvfs headroom in this case.
> 
> Signed-off-by: Qais Yousef <qyousef@xxxxxxxxxxx>
> ---
>  kernel/sched/cpufreq_schedutil.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
> index 318b09bc4ab1..4a1a8b353d51 100644
> --- a/kernel/sched/cpufreq_schedutil.c
> +++ b/kernel/sched/cpufreq_schedutil.c
> @@ -9,6 +9,7 @@
>  #define IOWAIT_BOOST_MIN	(SCHED_CAPACITY_SCALE / 8)
>  
>  DEFINE_PER_CPU_READ_MOSTLY(unsigned long, response_time_mult);
> +DEFINE_PER_CPU(unsigned long, last_update_util);
>  
>  struct sugov_tunables {
>  	struct gov_attr_set	attr_set;
> @@ -262,15 +263,19 @@ static unsigned int get_next_freq(struct sugov_policy *sg_policy,
>   * Also take into accounting how long tasks have been waiting in runnable but
>   * !running state. If it is high, it means we need higher DVFS headroom to
>   * reduce it.
> - *
> - * XXX: Should we provide headroom when the util is decaying?
>   */
>  static inline unsigned long sugov_apply_dvfs_headroom(unsigned long util,  int cpu)
>  {
> -	unsigned long update_headroom, waiting_headroom;
> +	unsigned long update_headroom, waiting_headroom, prev_util;
>  	struct rq *rq = cpu_rq(cpu);
>  	u64 delay;
>  
> +	prev_util = per_cpu(last_update_util, cpu);
> +	per_cpu(last_update_util, cpu) = util;
> +
> +	if (util < prev_util)
> +		return util;
> +
>  	/*
>  	 * What is the possible worst case scenario for updating util_avg, ctx
>  	 * switch or TICK?
> -- 
> 2.34.1
> 

Hmm, after the changes in "sched: cpufreq: Remove magic 1.25 headroom from
sugov_apply_dvfs_headroom()", won't sugov_apply_dvfs_headroom() already decay
the headroom gracefully in step with the decaying util? I suspect that abruptly
killing the headroom entirely could be premature depending on the workload, and
lead to util bouncing back up due to the time dilation effect you described in
the cover letter.

Cheers,
Sultan