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

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

[Jason Andryuk]

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@amd.com>
---
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")
---
 xen/drivers/cpufreq/cpufreq_ondemand.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/xen/drivers/cpufreq/cpufreq_ondemand.c b/xen/drivers/cpufreq/cpufreq_ondemand.c
index 0d94c0e464..537695eaab 100644
--- 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));
 }
 
 static void dbs_timer_init(struct cpu_dbs_info_s *dbs_info)
@@ -401,6 +400,6 @@ void cpufreq_dbs_timer_resume(void)
             (void)cmpxchg(stoppable, -1, 1);
         }
         else
-            set_timer(t, align_timer(t->expires, dbs_tuners_ins.sampling_rate));
+            set_timer(t, align_timer(now, dbs_tuners_ins.sampling_rate));
     }
 }
-- 
2.54.0

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

[Jan Beulich]

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@amd.com>
> ---
> 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?

> --- 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.

Jan

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

[Roger Pau Monné]

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@amd.com>

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

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.

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

[Oleksii Kurochko]

On 6/16/26 8:47 AM, Roger Pau Monné wrote:
> 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@amd.com>
> 
> Acked-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> Adding Oleksii for a release-ack.
> 

Release-Acked-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>

Thanks.

~ Oleksii

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

[Jan Beulich]

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@amd.com>

Oh, also Cc: Oleksii for a release-ack.

Jan