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

[Jason Andryuk <jason.andryuk@xxxxxxx>]

On 2026-06-15 14:33, Roger Pau Monné wrote:
> 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?
I'll send out a straight revert soon.  Additional changes can go on top 
(though probably not for 4.22?).  Thanks for the feedback.
Regards,
Jason