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.

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@citrix.com>
---
 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));
     }
 }
-- 
2.51.0

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@citrix.com>
> ---
>   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));
>       }
>   }

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.
> 
> So I think we want to revert?

Forgot to mention in the first reply, as I went straight into the
technical side: thanks for finding and reporting this, we can
hopefully get it sorted before the release.

Roger.

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

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

On 27.02.2026 08: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.
> 
> 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@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>