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

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

On Tue, Jun 16, 2026 at 08:30:25AM +0200, Jan Beulich wrote:
> On 15.06.2026 21:39, Jason Andryuk wrote:
> > The original commit showed a ~6% regression in a benchmark.  The call to
> > align_timer(firsttick, period) rounds firsttick up to the next mutiple
> > of the period, if firsttick % period != 0:
> > 
> > align_timer(0, period)          -> 0
> > align_timer(1, period)          -> period
> > align_timer(period, period)     -> period
> > align_timer(period + 1, period) -> 2 * period
> > 
> > So adding the period (sampling_rate) before calling align_timer() will
> > in most cases incease the expiration to 2 * period (sampling_rate) (the
> > exception being firsttick % period == 0).  This longer timer slows the
> > reaction time of the algorithm.
> > 
> > This reverts commit a0ed5bcfbeee81c91c574ad484faa057054eaf09.
> > 
> > Signed-off-by: Jason Andryuk <jason.andryuk@xxxxxxx>

Acked-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

Adding Oleksii for a release-ack.

> > ---
> > This is backported in stable trees and should be reverted there as well
> > (found in 4.20.3).
> > 
> > A Fixes seems superfluous and not normally used with a revert, but if
> > needed:
> > Fixes: a0ed5bcfbeee ("xen/cpufreq: fix usages of align_timer() in the 
> > on-demand governor")
> 
> If "git grep" is intended to (also) find such straight reverts by merely
> matching "Fixes: ", I think the tag should still be put there.
> 
> Talking of Fixes: tags - the original change had two of them, so there
> must have been a problem? Or, Roger, was this purely an observation from
> looking the the code?

It was purely by observation that the current timer setup will very
likely fire before the set period, making the sampling intervals
possibly shorter.  However shorter periods are fine, we simply
over-sample.  Longer periods as shown by Jason have an adverse
impact in the response time of the governor.

> > --- a/xen/drivers/cpufreq/cpufreq_ondemand.c
> > +++ b/xen/drivers/cpufreq/cpufreq_ondemand.c
> > @@ -185,8 +185,7 @@ 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,
> > -                          dbs_tuners_ins.sampling_rate));
> > +            align_timer(NOW() , dbs_tuners_ins.sampling_rate));
> >  }
> 
> As much as I understand you wanting to have things simple by doing a
> straight revert, imo the formatting flaws better wouldn't be introduced
> again. If I ended up committing this, I'd very likely take the liberty
> of doing so. Yet before ack-ing the question above needs answering.

I wouldn't mind adjusting this at commit.

Thanks, Roger.