| Message ID | 20240617141303.53857-1-frediano.ziglio@cloud.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86/xen/time: Reduce Xen timer tick | expand |
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
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.
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
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.
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;
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(-)