Singleshot timer firing late

Singleshot timer firing late

[Ross Lagerwall]

[-- Attachment #1: Type: text/plain, Size: 656 bytes --]

Hi,

While investigating a separate timer issue, I noticed that, in the
absence of other timers/factors, singleshot timers in Xen always fire
50us late (the timer slop amount).

From what I can see, this behavior was introduced by 116d62ddfd11
("timers: Simplify implementation logic.").  Before that, a similar
issue was fixed by 7e3e35eed67b ("Fix one timer range issue").

I have attached an RFC patch to program the timer to the nearest
deadline and then fire any timers up to 50us after that.  This is in
contrast with the previous behavior which would always program the
timer 50us after the nearest deadline.

Cheers
Ross Lagerwall

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: fix-late-timers.patch --]
[-- Type: text/x-patch; name="fix-late-timers.patch", Size: 1459 bytes --]

diff --git a/xen/common/timer.c b/xen/common/timer.c
index 1895a78..744bc66 100644
--- a/xen/common/timer.c
+++ b/xen/common/timer.c
@@ -25,7 +25,7 @@
 #include <asm/desc.h>
 #include <asm/atomic.h>
 
-/* We program the time hardware this far behind the closest deadline. */
+/* We fire events up to this amount early. */
 static unsigned int timer_slop __read_mostly = 50000; /* 50 us */
 integer_param("timer_slop", timer_slop);
 
@@ -463,14 +463,14 @@ static void timer_softirq_action(void)
 
     /* Execute ready heap timers. */
     while ( (GET_HEAP_SIZE(heap) != 0) &&
-            ((t = heap[1])->expires < now) )
+            ((t = heap[1])->expires < (now + timer_slop) ) )
     {
         remove_from_heap(heap, t);
         execute_timer(ts, t);
     }
 
     /* Execute ready list timers. */
-    while ( ((t = ts->list) != NULL) && (t->expires < now) )
+    while ( ((t = ts->list) != NULL) && (t->expires < (now + timer_slop) ) )
     {
         ts->list = t->list_next;
         execute_timer(ts, t);
@@ -493,7 +493,7 @@ static void timer_softirq_action(void)
     if ( (ts->list != NULL) && (ts->list->expires < deadline) )
         deadline = ts->list->expires;
     this_cpu(timer_deadline) =
-        (deadline == STIME_MAX) ? 0 : deadline + timer_slop;
+        (deadline == STIME_MAX) ? 0 : deadline;
 
     if ( !reprogram_timer(this_cpu(timer_deadline)) )
         raise_softirq(TIMER_SOFTIRQ);

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: Singleshot timer firing late

[Jan Beulich]

>>> On 19.05.14 at 12:42, <ross.lagerwall@citrix.com> wrote:
> While investigating a separate timer issue, I noticed that, in the
> absence of other timers/factors, singleshot timers in Xen always fire
> 50us late (the timer slop amount).
> 
> From what I can see, this behavior was introduced by 116d62ddfd11
> ("timers: Simplify implementation logic.").  Before that, a similar
> issue was fixed by 7e3e35eed67b ("Fix one timer range issue").
> 
> I have attached an RFC patch to program the timer to the nearest
> deadline and then fire any timers up to 50us after that.  This is in
> contrast with the previous behavior which would always program the
> timer 50us after the nearest deadline.

While the patch may certainly do what you want, you don't give
enough explanation of why this is what every one wants. Plus
firing timers early is precisely what we _do not_ want in general.
Furthermore, if this 50ms slop is of concern to you, using the
corresponding command line option to shrink or eliminate it would
be the first thing to try anyway - that's why the command line
option was added after all.

Jan

Re: Singleshot timer firing late

[Ross Lagerwall]

[-- Attachment #1: Type: text/plain, Size: 2171 bytes --]

On Mon, 2014-05-19 at 12:24 +0100, Jan Beulich wrote:
> >>> On 19.05.14 at 12:42, <ross.lagerwall@citrix.com> wrote:
> > While investigating a separate timer issue, I noticed that, in the
> > absence of other timers/factors, singleshot timers in Xen always fire
> > 50us late (the timer slop amount).
> > 
> > From what I can see, this behavior was introduced by 116d62ddfd11
> > ("timers: Simplify implementation logic.").  Before that, a similar
> > issue was fixed by 7e3e35eed67b ("Fix one timer range issue").
> > 
> > I have attached an RFC patch to program the timer to the nearest
> > deadline and then fire any timers up to 50us after that.  This is in
> > contrast with the previous behavior which would always program the
> > timer 50us after the nearest deadline.
> 
> While the patch may certainly do what you want, you don't give
> enough explanation of why this is what every one wants. Plus
> firing timers early is precisely what we _do not_ want in general.
> Furthermore, if this 50ms slop is of concern to you, using the
> corresponding command line option to shrink or eliminate it would
> be the first thing to try anyway - that's why the command line
> option was added after all.
> 

Well, currently in the absence of other timers, the timer will always
fire 50us late.  If there are multiple deadlines within 50us, then some
of them may fire less than 50us late.

The last email's patch changes it so that in the absence of other
timers, the timer will fire on time.  If there are multiple deadlines
within 50us, then some of them may fire up to 50us early.  As far as I
can tell, this would give a smaller average latency (and restore the
behavior to what it was before 116d62ddfd11, which gave no reason for
the change).  Of course, if firing timers early is a problem then this
approach should not be taken.

Here's another patch which programs the timer to the deadline of the
closest timer if it is further than 50us ahead, otherwise it sets it
50us ahead. This way a single event fires on time and events don't fire
early.  I think this behaves like Linux clockevent's min_delta_ns.

Regards
Ross

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: fix-late-timers.patch --]
[-- Type: text/x-patch; name="fix-late-timers.patch", Size: 550 bytes --]

diff -r 6c0a0284d9cb xen/common/timer.c
--- a/xen/common/timer.c	Mon May 19 13:47:12 2014 +0100
+++ b/xen/common/timer.c	Mon May 19 13:49:26 2014 +0100
@@ -493,7 +493,7 @@
     if ( (ts->list != NULL) && (ts->list->expires < deadline) )
         deadline = ts->list->expires;
     this_cpu(timer_deadline) =
-        (deadline == STIME_MAX) ? 0 : deadline + timer_slop;
+        (deadline == STIME_MAX) ? 0 : MAX(deadline, now + timer_slop);
 
     if ( !reprogram_timer(this_cpu(timer_deadline)) )
         raise_softirq(TIMER_SOFTIRQ);

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: Singleshot timer firing late

[Jan Beulich]

>>> On 19.05.14 at 15:08, <ross.lagerwall@citrix.com> wrote:
> Here's another patch which programs the timer to the deadline of the
> closest timer if it is further than 50us ahead, otherwise it sets it
> 50us ahead. This way a single event fires on time and events don't fire
> early.  I think this behaves like Linux clockevent's min_delta_ns.

Yes, this one I can see us taking if submitted in proper shape. But
if at all possible I'd like you to get Keir's input here too, as he may
recall further details on what lead to 116d62ddfd11.

As a technical detail - I think I'd prefer you to not use the latched
(and stale, due to an unknown number of timers having run between
NOW() being used and the consumer here) value "now", but rather
re-latch now right before the assignment. This in particular shrinks
the number of cases where the MAX() degenerates to "deadline"
because of "now + timer_slop" already lying in the past.

Jan