Re: [PATCH 2/2] xen/cpufreq: fix usages of align_timer() in the on-demand governor

[Jason Andryuk <jason.andryuk@xxxxxxx>]

On 2026-02-27 02:32, Roger Pau Monne wrote:
> The first parameter passed to align_timer() is the timer expiration, not
> the current time.  Adjust the calls to align_timer() in the on-demand
> governor to pass the expected timer expiration as the first parameter.
Internally, we have a report of a benchmark regressing ~6% with this 
change on 4.20.
s_time_t align_timer(s_time_t firsttick, uint64_t period)
{
    if ( !period )
        return firsttick;

    return firsttick + (period - 1) - ((firsttick - 1) % period);
}

The code rounds firsttick up to the next period:

align_timer(0, period)          -> 0
align_timer(1, period)          -> period
align_timer(period - 1, period) -> period
align_timer(period, period)     -> period
align_timer(period + 1, period) -> 2 * period

With the change of this patch adding the period before calling 
align_timer(), the timer is set for two periods in the future.  The only 
exception is when firsttick % period == 0.  I think that is unlikely to 
happen since NOW() will always be a little after the period.  Even if it 
did happen, the timer would fire immediately, but the next timer would 
be set for 1 period later.
So I think we want to revert?

Regards,
Jason

> Fixes: af74e3a15a83 ("cpufreq: align dbs timer for better package C state 
> residency")
> Fixes: 382b95f627a9 ("Fix cpufreq HW-ALL coordination handle")
> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> ---
>   xen/drivers/cpufreq/cpufreq_ondemand.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/xen/drivers/cpufreq/cpufreq_ondemand.c 
> b/xen/drivers/cpufreq/cpufreq_ondemand.c
> index 537695eaab19..0d94c0e464a6 100644
> --- a/xen/drivers/cpufreq/cpufreq_ondemand.c
> +++ b/xen/drivers/cpufreq/cpufreq_ondemand.c
> @@ -185,7 +185,8 @@ static void cf_check do_dbs_timer(void *dbs)
>       dbs_check_cpu(dbs_info);
>   
>       set_timer(&per_cpu(dbs_timer, dbs_info->cpu),
> -            align_timer(NOW() , dbs_tuners_ins.sampling_rate));
> +              align_timer(NOW() + dbs_tuners_ins.sampling_rate,
> +                          dbs_tuners_ins.sampling_rate));
>   }
>   
>   static void dbs_timer_init(struct cpu_dbs_info_s *dbs_info)
> @@ -400,6 +401,6 @@ void cpufreq_dbs_timer_resume(void)
>               (void)cmpxchg(stoppable, -1, 1);
>           }
>           else
> -            set_timer(t, align_timer(now, dbs_tuners_ins.sampling_rate));
> +            set_timer(t, align_timer(t->expires, 
> dbs_tuners_ins.sampling_rate));
>       }
>   }