[PATCH] x86/xen/time: Reduce Xen timer tick

[PATCH] x86/xen/time: Reduce Xen timer tick

[Frediano Ziglio]

Current timer tick is causing some deadline to fail.
The current high value constant was probably due to an old
bug in the Xen timer implementation causing errors if the
deadline was in the future.
This was fixed in Xen commit:
19c6cbd90965 xen/vcpu: ignore VCPU_SSHOTTMR_future

Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com>
---
 arch/x86/xen/time.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/xen/time.c b/arch/x86/xen/time.c
index 52fa5609b7f6..ce30b8d3efe7 100644
--- a/arch/x86/xen/time.c
+++ b/arch/x86/xen/time.c
@@ -30,7 +30,7 @@
 #include "xen-ops.h"
 
 /* Minimum amount of time until next clock event fires */
-#define TIMER_SLOP	100000
+#define TIMER_SLOP	1000
 
 static u64 xen_sched_clock_offset __read_mostly;
 
-- 
2.45.1

Re: [PATCH] x86/xen/time: Reduce Xen timer tick

[Jan Beulich]

On 17.06.2024 16:13, Frediano Ziglio wrote:
> Current timer tick is causing some deadline to fail.
> The current high value constant was probably due to an old
> bug in the Xen timer implementation causing errors if the
> deadline was in the future.
> This was fixed in Xen commit:
> 19c6cbd90965 xen/vcpu: ignore VCPU_SSHOTTMR_future

And then newer kernels are no longer reliably usable on Xen older than
this?

> --- a/arch/x86/xen/time.c
> +++ b/arch/x86/xen/time.c
> @@ -30,7 +30,7 @@
>  #include "xen-ops.h"
>  
>  /* Minimum amount of time until next clock event fires */
> -#define TIMER_SLOP	100000
> +#define TIMER_SLOP	1000

It may be just the lack of knowledge of mine towards noadays's Linux'es
time handling, but the change of a value with this name and thus
commented doesn't directly relate to "timer tick" rate. Could you maybe
help me see the connection?

Jan

Re: [PATCH] x86/xen/time: Reduce Xen timer tick

[Roger Pau Monné]

On Mon, Jun 17, 2024 at 04:22:21PM +0200, Jan Beulich wrote:
> On 17.06.2024 16:13, Frediano Ziglio wrote:
> > Current timer tick is causing some deadline to fail.
> > The current high value constant was probably due to an old
> > bug in the Xen timer implementation causing errors if the
> > deadline was in the future.
> > This was fixed in Xen commit:
> > 19c6cbd90965 xen/vcpu: ignore VCPU_SSHOTTMR_future
> 
> And then newer kernels are no longer reliably usable on Xen older than
> this?

I think this should reference the Linux commit that removed the usage
of VCPU_SSHOTTMR_future on Linux itself, not the change that makes Xen
ignore the flag.

> > --- a/arch/x86/xen/time.c
> > +++ b/arch/x86/xen/time.c
> > @@ -30,7 +30,7 @@
> >  #include "xen-ops.h"
> >  
> >  /* Minimum amount of time until next clock event fires */
> > -#define TIMER_SLOP	100000
> > +#define TIMER_SLOP	1000
> 
> It may be just the lack of knowledge of mine towards noadays's Linux'es
> time handling, but the change of a value with this name and thus
> commented doesn't directly relate to "timer tick" rate. Could you maybe
> help me see the connection?

The TIMER_SLOP define is used in min_delta_{ns,ticks} field, and I
think this is wrong.

The min_delta_ns for the Xen timer is 1ns.  If Linux needs some
greater min delta than what the timer interface supports it should be
handled in the generic timer code, not open coded at the definition of
possibly each timer implementation.

Thanks, Roger.

Re: [PATCH] x86/xen/time: Reduce Xen timer tick

[Frediano Ziglio]

On Mon, Jun 17, 2024 at 3:37 PM Roger Pau Monné <roger.pau@citrix.com> wrote:
>
> On Mon, Jun 17, 2024 at 04:22:21PM +0200, Jan Beulich wrote:
> > On 17.06.2024 16:13, Frediano Ziglio wrote:
> > > Current timer tick is causing some deadline to fail.
> > > The current high value constant was probably due to an old
> > > bug in the Xen timer implementation causing errors if the
> > > deadline was in the future.
> > > This was fixed in Xen commit:
> > > 19c6cbd90965 xen/vcpu: ignore VCPU_SSHOTTMR_future
> >
> > And then newer kernels are no longer reliably usable on Xen older than
> > this?
>
> I think this should reference the Linux commit that removed the usage
> of VCPU_SSHOTTMR_future on Linux itself, not the change that makes Xen
> ignore the flag.
>

Yes, Linux kernel stopped using this flag since 2016 with commit
c06b6d70feb32d28f04ba37aa3df17973fd37b6b, "xen/x86: don't lose event
interrupts", I'll add it in the commit message.

> > > --- a/arch/x86/xen/time.c
> > > +++ b/arch/x86/xen/time.c
> > > @@ -30,7 +30,7 @@
> > >  #include "xen-ops.h"
> > >
> > >  /* Minimum amount of time until next clock event fires */
> > > -#define TIMER_SLOP 100000
> > > +#define TIMER_SLOP 1000
> >
> > It may be just the lack of knowledge of mine towards noadays's Linux'es
> > time handling, but the change of a value with this name and thus
> > commented doesn't directly relate to "timer tick" rate. Could you maybe
> > help me see the connection?
>
> The TIMER_SLOP define is used in min_delta_{ns,ticks} field, and I
> think this is wrong.
>
> The min_delta_ns for the Xen timer is 1ns.  If Linux needs some
> greater min delta than what the timer interface supports it should be
> handled in the generic timer code, not open coded at the definition of
> possibly each timer implementation.
>

I think this is done to reduce potential event handling frequency, in
some other part of timer code (in kernel/time/clockevents.c) there's a
comment "Deltas less than 1usec are pointless noise".
I think it's hard for a software to get a frequency so high so I
didn't propose 1ns.
What are you suggesting? To put 1ns and see what happens? Is there any
proper test code for this?

> Thanks, Roger.

Frediano

Re: [PATCH] x86/xen/time: Reduce Xen timer tick

[Roger Pau Monné]

On Tue, Jun 18, 2024 at 09:37:08AM +0100, Frediano Ziglio wrote:
> On Mon, Jun 17, 2024 at 3:37 PM Roger Pau Monné <roger.pau@citrix.com> wrote:
> >
> > On Mon, Jun 17, 2024 at 04:22:21PM +0200, Jan Beulich wrote:
> > > On 17.06.2024 16:13, Frediano Ziglio wrote:
> > > > Current timer tick is causing some deadline to fail.
> > > > The current high value constant was probably due to an old
> > > > bug in the Xen timer implementation causing errors if the
> > > > deadline was in the future.
> > > > This was fixed in Xen commit:
> > > > 19c6cbd90965 xen/vcpu: ignore VCPU_SSHOTTMR_future
> > >
> > > And then newer kernels are no longer reliably usable on Xen older than
> > > this?
> >
> > I think this should reference the Linux commit that removed the usage
> > of VCPU_SSHOTTMR_future on Linux itself, not the change that makes Xen
> > ignore the flag.
> >
> 
> Yes, Linux kernel stopped using this flag since 2016 with commit
> c06b6d70feb32d28f04ba37aa3df17973fd37b6b, "xen/x86: don't lose event
> interrupts", I'll add it in the commit message.
> 
> > > > --- a/arch/x86/xen/time.c
> > > > +++ b/arch/x86/xen/time.c
> > > > @@ -30,7 +30,7 @@
> > > >  #include "xen-ops.h"
> > > >
> > > >  /* Minimum amount of time until next clock event fires */
> > > > -#define TIMER_SLOP 100000
> > > > +#define TIMER_SLOP 1000
> > >
> > > It may be just the lack of knowledge of mine towards noadays's Linux'es
> > > time handling, but the change of a value with this name and thus
> > > commented doesn't directly relate to "timer tick" rate. Could you maybe
> > > help me see the connection?
> >
> > The TIMER_SLOP define is used in min_delta_{ns,ticks} field, and I
> > think this is wrong.
> >
> > The min_delta_ns for the Xen timer is 1ns.  If Linux needs some
> > greater min delta than what the timer interface supports it should be
> > handled in the generic timer code, not open coded at the definition of
> > possibly each timer implementation.
> >
> 
> I think this is done to reduce potential event handling frequency, in
> some other part of timer code (in kernel/time/clockevents.c) there's a
> comment "Deltas less than 1usec are pointless noise".

Then why does the interface allow for timers having a resolution up to
1ns then?

> I think it's hard for a software to get a frequency so high so I
> didn't propose 1ns.
> What are you suggesting? To put 1ns and see what happens? Is there any
> proper test code for this?

The Xen timer interface has a resolution of 1ns, and the Linux
structures that describe timers also support a 1ns resolution.  I can
perfectly understand that deltas of 1ns make no sense, but given how
the Xen timer works those won't be a problem.  The interrupt will get
injected strictly after the hypercall to setup the timer, because by
the time Xen processes the delta it will most likely have already
expired.

Forcing every timer to setup a minimal delta of 1usec is pointless.
It either needs to be done in the generic code, or the interface to
register timers needs to be adjusted to allow for a minimum resolution
of 1usec.

Thanks, Roger.