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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Mon, Jun 15, 2026 at 01:44:54PM -0400, Jason Andryuk wrote:
> 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.

Hm, I see.  So this is explicitly done to never exceed one period
between sampling, even if that implies using a smaller period and
over-sampling.  That's kind of different from how the other caller
uses align_timer(), where it's expected the timer to fire after the
period has expired, not before.  I think create_periodic_time() is
fine, because it's only the first tick that might be delayed,
afterwards the next tick should be aligned to the period already.

> So I think we want to revert?

I think we want to revert the first chunk...

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

... but possibly keep this as-is?  Thinking about it, t->expires
should already be aligned, and hence we could drop the align_timer()
call here?  It should be equivalent to aligning NOW() to the next
period boundary, so yes, we could revert this chunk also and
timer expiry should be the same.

And maybe we want to add an extra align_timer() in dbs_timer_init() to
align the first call also in a separate patch.

Do you want to send the revert, or should I?

Regards, Roger.