[PATCH 0/3] xen: RCU: Improve the idle timer handling

[PATCH 0/3] xen: RCU: Improve the idle timer handling

[Dario Faggioli]

Hello,

this series is the followup of my previous RCU series. It contains:
- the patch that makes the period of the RCU idle timer configurable, via a Xen
  boot time parameter, as suggested by Jan, during review of the original series
  (patch 2);
- the patch that makse the period of the RCU idle timer adaptive, but using the
  algorithm suggested by George, during review of the original series (patch 3).

Patch 1 is a fix for the fact that if we stop a timer that has already expired,
right now, we miss invoking the timer handler! This is a general bug, but is
particularly relevant for this series, as, without it, the RCU idle timer
handler is never run (and hence, George's algorithm can't work :-/).

Regards,
Dario
---
Dario Faggioli (3):
      xen: timers: don't miss a timer event because of stop_timer()
      xen: RCU: make the period of the idle timer configurable.
      xen: RCU: make the period of the idle timer adaptive.

 docs/misc/xen-command-line.markdown |    9 ++++++
 xen/common/rcupdate.c               |   57 ++++++++++++++++++++++++++++++++---
 xen/common/timer.c                  |   14 +++++++--
 3 files changed, 73 insertions(+), 7 deletions(-)
--
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)

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

[PATCH 1/3] xen: timers: don't miss a timer event because of stop_timer()

[Dario Faggioli]

If stop_timer() is called between when a timer interrupt
arrives (and TIMER_SOFTIRQ is raised) and when softirqs are
checked and handled, the timer that has fire is (right in
stop_timer()) deactivated, and the handler for that
occurrence of the interrupt never executed.

This happens, e.g. to timers stopped during the wakeup
from idle (e.g., C-states, on x86) path.

To fix that, don't deactivate a timer, while stopping it,
if it has expired. On the contrary, when that happens,
(re-)raise the timer softirq, to make sure the handler is
invoked.

Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
---
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: George Dunlap <George.Dunlap@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Julien Grall <julien.grall@arm.com>
Cc: Tim Deegan <tim@xen.org>
---
 xen/common/timer.c |   14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/xen/common/timer.c b/xen/common/timer.c
index d9ff669..b1d1511 100644
--- a/xen/common/timer.c
+++ b/xen/common/timer.c
@@ -217,7 +217,7 @@ static inline void activate_timer(struct timer *timer)
     timer->status = TIMER_STATUS_invalid;
     list_del(&timer->inactive);
 
-    if ( add_entry(timer) )
+    if ( add_entry(timer) || timer->expires <= NOW() )
         cpu_raise_softirq(timer->cpu, TIMER_SOFTIRQ);
 }
 
@@ -326,7 +326,17 @@ void stop_timer(struct timer *timer)
         return;
 
     if ( active_timer(timer) )
-        deactivate_timer(timer);
+    {
+        /*
+         * If the timer is expired already, 'call' the softirq handler to
+         * execute it (it will leave it inactive after that). If not, just
+         * deactivate it.
+         */
+        if ( timer->expires <= NOW() )
+            cpu_raise_softirq(timer->cpu, TIMER_SOFTIRQ);
+        else
+            deactivate_timer(timer);
+    }
 
     timer_unlock_irqrestore(timer, flags);
 }


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

[PATCH 2/3] xen: RCU: make the period of the idle timer configurable.

[Dario Faggioli]

Make it possible for the user to specify, with the boot
time parameter rcu_idle_timer_period_ms, how frequently
a CPU that went idle with pending RCU callbacks should be
woken up to check if the grace period ended.

Typical values (i.e., some of the values used by Linux as
the tick frequency) are 10, 4 or 1 ms. Default valus (used
when this parameter is not specified) is 10ms. Maximum is
100ms.

Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
---
Cc: Jan Beulich <JBeulich@suse.com>
Cc: Julien Grall <julien.grall@arm.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: George Dunlap <george.dunlap@citrix.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>,
Cc: Tim Deegan <tim@xen.org>
---
 docs/misc/xen-command-line.markdown |    9 +++++++++
 xen/common/rcupdate.c               |   33 +++++++++++++++++++++++++++++----
 2 files changed, 38 insertions(+), 4 deletions(-)

diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
index 9797c8d..acacc93 100644
--- a/docs/misc/xen-command-line.markdown
+++ b/docs/misc/xen-command-line.markdown
@@ -1422,6 +1422,15 @@ The following resources are available:
     sum of CBMs is fixed, that means actual `cos_max` in use will automatically
     reduce to half when CDP is enabled.
 
+### rcu\_idle\_timer\_period\_ms
+> `= <integer>`
+
+> Default: `10`
+
+How frequently a CPU which has gone idle, but with pending RCU callbacks,
+should be woken up to check if the grace period has completed, and the
+callbacks are safe to be executed. Expressed in milliseconds; maximum is 100ms.
+
 ### reboot
 > `= t[riple] | k[bd] | a[cpi] | p[ci] | P[ower] | e[fi] | n[o] [, [w]arm | [c]old]`
 
diff --git a/xen/common/rcupdate.c b/xen/common/rcupdate.c
index 871936f..dfd0daf 100644
--- a/xen/common/rcupdate.c
+++ b/xen/common/rcupdate.c
@@ -110,10 +110,35 @@ struct rcu_data {
  * About how far in the future the timer should be programmed each time,
  * it's hard to tell (guess!!). Since this mimics Linux's periodic timer
  * tick, take values used there as an indication. In Linux 2.6.21, tick
- * period can be 10ms, 4ms, 3.33ms or 1ms. Let's use 10ms, to enable
- * at least some power saving on the CPU that is going idle.
+ * period can be 10ms, 4ms, 3.33ms or 1ms.
+ *
+ * By default, we use 10ms, to enable at least some power saving on the
+ * CPU that is going idle. The user can change this, via a boot time
+ * parameter, but only up to 100ms.
  */
-#define RCU_IDLE_TIMER_PERIOD MILLISECS(10)
+#define IDLE_TIMER_PERIOD_MAX     MILLISECS(100)
+#define IDLE_TIMER_PERIOD_DEFAULT MILLISECS(10)
+static s_time_t __read_mostly idle_timer_period = IDLE_TIMER_PERIOD_DEFAULT;
+
+static int parse_idle_timer_period(const char *s)
+{
+    long unsigned int period = simple_strtoul(s, NULL, 10);
+    int ret = 0;
+
+    if ( MILLISECS(period) > IDLE_TIMER_PERIOD_MAX )
+    {
+        printk("WARNING: rcu_idle_timer_period_ms must be < %"PRI_stime"\n",
+               IDLE_TIMER_PERIOD_MAX / MILLISECS(1));
+        ret = -EINVAL;
+    }
+    else
+        idle_timer_period = MILLISECS(period);
+
+    printk("RCU idle timer period: %lums\n", period);
+
+    return ret;
+}
+custom_param("rcu_idle_timer_period_ms", parse_idle_timer_period);
 
 static DEFINE_PER_CPU(struct rcu_data, rcu_data);
 
@@ -453,7 +478,7 @@ void rcu_idle_timer_start()
     if (likely(!rdp->curlist))
         return;
 
-    set_timer(&rdp->idle_timer, NOW() + RCU_IDLE_TIMER_PERIOD);
+    set_timer(&rdp->idle_timer, NOW() + idle_timer_period);
     rdp->idle_timer_active = true;
 }
 


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

[PATCH 3/3] xen: RCU: make the period of the idle timer adaptive.

[Dario Faggioli]

Basically, if the RCU idle timer, when (if!) it fires,
finds that the grace period isn't over, we increase the
timer's period (i.e., it will fire later, next time).
If, OTOH, it finds the grace period is already finished,
we decrease the timer's period (i.e., it will fire a bit
earlier next time).

The goal is to let the period timer sefl-adjust to a
number of 'misses', of the order of 1%.

Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
Suggested-by: George Dunlap <george.dunlap@citrix.com>
---
Cc: Jan Beulich <JBeulich@suse.com>
Cc: Julien Grall <julien.grall@arm.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: George Dunlap <george.dunlap@citrix.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>,
Cc: Tim Deegan <tim@xen.org>
---
 xen/common/rcupdate.c |   24 +++++++++++++++++++++++-
 1 file changed, 23 insertions(+), 1 deletion(-)

diff --git a/xen/common/rcupdate.c b/xen/common/rcupdate.c
index dfd0daf..9a708ef 100644
--- a/xen/common/rcupdate.c
+++ b/xen/common/rcupdate.c
@@ -118,6 +118,7 @@ struct rcu_data {
  */
 #define IDLE_TIMER_PERIOD_MAX     MILLISECS(100)
 #define IDLE_TIMER_PERIOD_DEFAULT MILLISECS(10)
+#define IDLE_TIMER_PERIOD_MIN     MICROSECS(100)
 static s_time_t __read_mostly idle_timer_period = IDLE_TIMER_PERIOD_DEFAULT;
 
 static int parse_idle_timer_period(const char *s)
@@ -140,6 +141,17 @@ static int parse_idle_timer_period(const char *s)
 }
 custom_param("rcu_idle_timer_period_ms", parse_idle_timer_period);
 
+/*
+ * Increment and decrement values for the idle timer handler. The algorithm
+ * works as follows:
+ * - if the timer actually fires, and it finds out that the grace period isn't
+ *   over yet, we add IDLE_TIMER_PERIOD_INCR to the timer's period;
+ * - if the timer actually fires and it finds the grace period over, we
+ *   subtract IDLE_TIMER_PERIOD_DECR from the timer's period.
+ */
+#define IDLE_TIMER_PERIOD_INCR    MILLISECS(10)
+#define IDLE_TIMER_PERIOD_DECR    MICROSECS(100)
+
 static DEFINE_PER_CPU(struct rcu_data, rcu_data);
 
 static int blimit = 10;
@@ -495,8 +507,18 @@ void rcu_idle_timer_stop()
 
 static void rcu_idle_timer_handler(void* data)
 {
-    /* Nothing, really... Just count the number of times we fire */
     perfc_incr(rcu_idle_timer);
+
+    if ( !cpumask_empty(&rcu_ctrlblk.cpumask) )
+    {
+        idle_timer_period = min_t(s_time_t, IDLE_TIMER_PERIOD_MAX,
+                                  idle_timer_period + IDLE_TIMER_PERIOD_INCR);
+    }
+    else
+    {
+        idle_timer_period = max_t(s_time_t, IDLE_TIMER_PERIOD_MIN,
+                                  idle_timer_period - IDLE_TIMER_PERIOD_DECR);
+    }
 }
 
 void rcu_check_callbacks(int cpu)


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

Re: [PATCH 1/3] xen: timers: don't miss a timer event because of stop_timer()

[Jan Beulich]

>>> On 15.09.17 at 20:01, <dario.faggioli@citrix.com> wrote:
> --- a/xen/common/timer.c
> +++ b/xen/common/timer.c
> @@ -217,7 +217,7 @@ static inline void activate_timer(struct timer *timer)
>      timer->status = TIMER_STATUS_invalid;
>      list_del(&timer->inactive);
>  
> -    if ( add_entry(timer) )
> +    if ( add_entry(timer) || timer->expires <= NOW() )
>          cpu_raise_softirq(timer->cpu, TIMER_SOFTIRQ);
>  }

I'm not convinced of this change - it's unrelated to what title and
description say, and I'm not sure a timer being activated in a way
possibly clashing with its expiry is actually intended to fire.

> @@ -326,7 +326,17 @@ void stop_timer(struct timer *timer)
>          return;
>  
>      if ( active_timer(timer) )
> -        deactivate_timer(timer);
> +    {
> +        /*
> +         * If the timer is expired already, 'call' the softirq handler to
> +         * execute it (it will leave it inactive after that). If not, just
> +         * deactivate it.
> +         */
> +        if ( timer->expires <= NOW() )
> +            cpu_raise_softirq(timer->cpu, TIMER_SOFTIRQ);
> +        else
> +            deactivate_timer(timer);
> +    }

Isn't it reasonable to expect current behavior, i.e. the timer not
raising any events other than those already in progress?

Additionally I'm not convinced the changed actually does what you
want: What if NOW() becomes equal to timer->expires immediately
after you've managed to obtain its value, before execution makes
it into deactivate_timer()? IOW you're shrinking a time window which
(I think) you really mean to eliminate.

Furthermore, taking both changes together: What if the same
you try to address in stop_timer() happens in set_timer()? Wouldn't
it then be only consistent to also require a timer even to fire for the
old expiry value, before the new one is being set?

Jan


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

Re: [PATCH 2/3] xen: RCU: make the period of the idle timer configurable.

[Jan Beulich]

>>> On 15.09.17 at 20:01, <dario.faggioli@citrix.com> wrote:
> --- a/xen/common/rcupdate.c
> +++ b/xen/common/rcupdate.c
> @@ -110,10 +110,35 @@ struct rcu_data {
>   * About how far in the future the timer should be programmed each time,
>   * it's hard to tell (guess!!). Since this mimics Linux's periodic timer
>   * tick, take values used there as an indication. In Linux 2.6.21, tick
> - * period can be 10ms, 4ms, 3.33ms or 1ms. Let's use 10ms, to enable
> - * at least some power saving on the CPU that is going idle.
> + * period can be 10ms, 4ms, 3.33ms or 1ms.
> + *
> + * By default, we use 10ms, to enable at least some power saving on the
> + * CPU that is going idle. The user can change this, via a boot time
> + * parameter, but only up to 100ms.
>   */
> -#define RCU_IDLE_TIMER_PERIOD MILLISECS(10)
> +#define IDLE_TIMER_PERIOD_MAX     MILLISECS(100)
> +#define IDLE_TIMER_PERIOD_DEFAULT MILLISECS(10)
> +static s_time_t __read_mostly idle_timer_period = IDLE_TIMER_PERIOD_DEFAULT;
> +
> +static int parse_idle_timer_period(const char *s)

__init

> +{
> +    long unsigned int period = simple_strtoul(s, NULL, 10);

unsigned long

> +    int ret = 0;
> +
> +    if ( MILLISECS(period) > IDLE_TIMER_PERIOD_MAX )
> +    {
> +        printk("WARNING: rcu_idle_timer_period_ms must be < %"PRI_stime"\n",
> +               IDLE_TIMER_PERIOD_MAX / MILLISECS(1));
> +        ret = -EINVAL;
> +    }
> +    else
> +        idle_timer_period = MILLISECS(period);
> +
> +    printk("RCU idle timer period: %lums\n", period);
> +
> +    return ret;
> +}
> +custom_param("rcu_idle_timer_period_ms", parse_idle_timer_period);

Does this really need handling as custom_param(). I.e. wouldn't
integer_param() plus sanitizing in rcu_init() suffice? That would
also make the log message be printed uniformly - merely echoing
the value from the command line doesn't look very useful to me.
Or are you concerned about the value being specified in e.g.
hex rather than dec?

Additionally - what about a lower bound? Clearly zero would be
a rather bad choice?

Also can you please switch to - as the separator in the command
line argument name?

Jan


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

Re: [PATCH 3/3] xen: RCU: make the period of the idle timer adaptive.

[Jan Beulich]

>>> On 15.09.17 at 20:01, <dario.faggioli@citrix.com> wrote:
> @@ -495,8 +507,18 @@ void rcu_idle_timer_stop()
>  
>  static void rcu_idle_timer_handler(void* data)
>  {
> -    /* Nothing, really... Just count the number of times we fire */
>      perfc_incr(rcu_idle_timer);
> +
> +    if ( !cpumask_empty(&rcu_ctrlblk.cpumask) )
> +    {
> +        idle_timer_period = min_t(s_time_t, IDLE_TIMER_PERIOD_MAX,
> +                                  idle_timer_period + IDLE_TIMER_PERIOD_INCR);
> +    }
> +    else
> +    {
> +        idle_timer_period = max_t(s_time_t, IDLE_TIMER_PERIOD_MIN,
> +                                  idle_timer_period - IDLE_TIMER_PERIOD_DECR);
> +    }

Pointless braces. And do you really need min_t()/max_t() here,
rather than just min()/max()?

Jan


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

Re: [PATCH 2/3] xen: RCU: make the period of the idle timer configurable.

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 1926 bytes --]

On Tue, 2017-09-26 at 09:14 -0600, Jan Beulich wrote:
> > > > On 15.09.17 at 20:01, <dario.faggioli@citrix.com> wrote:
> > --- a/xen/common/rcupdate.c
> > +++ b/xen/common/rcupdate.c
> > +    int ret = 0;
> > +
> > +    if ( MILLISECS(period) > IDLE_TIMER_PERIOD_MAX )
> > +    {
> > +        printk("WARNING: rcu_idle_timer_period_ms must be <
> > %"PRI_stime"\n",
> > +               IDLE_TIMER_PERIOD_MAX / MILLISECS(1));
> > +        ret = -EINVAL;
> > +    }
> > +    else
> > +        idle_timer_period = MILLISECS(period);
> > +
> > +    printk("RCU idle timer period: %lums\n", period);
> > +
> > +    return ret;
> > +}
> > +custom_param("rcu_idle_timer_period_ms", parse_idle_timer_period);
> 
> Does this really need handling as custom_param(). I.e. wouldn't
> integer_param() plus sanitizing in rcu_init() suffice? 
>
I did it like that in the first place. But then I did not like the
overall look of the patch, so I changed the approach.

I can put it back together the integer_param() variant, and you'll tell
me which one you like better.

> That would
> also make the log message be printed uniformly - merely echoing
> the value from the command line doesn't look very useful to me.
>
Mmm.. Sorry, but I don't get this part.

> Additionally - what about a lower bound? Clearly zero would be
> a rather bad choice?
> 
Indeed I should enforce a meaningful min too (I'll take it from patch 3
and put it here).

> Also can you please switch to - as the separator in the command
> line argument name?
> 
Ah, ok.

Thanks and Regards,
Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

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

Re: [PATCH 3/3] xen: RCU: make the period of the idle timer adaptive.

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 1473 bytes --]

On Tue, 2017-09-26 at 09:24 -0600, Jan Beulich wrote:
> > > > On 15.09.17 at 20:01, <dario.faggioli@citrix.com> wrote:
> > @@ -495,8 +507,18 @@ void rcu_idle_timer_stop()
> >  
> >  static void rcu_idle_timer_handler(void* data)
> >  {
> > -    /* Nothing, really... Just count the number of times we fire
> > */
> >      perfc_incr(rcu_idle_timer);
> > +
> > +    if ( !cpumask_empty(&rcu_ctrlblk.cpumask) )
> > +    {
> > +        idle_timer_period = min_t(s_time_t, IDLE_TIMER_PERIOD_MAX,
> > +                                  idle_timer_period +
> > IDLE_TIMER_PERIOD_INCR);
> > +    }
> > +    else
> > +    {
> > +        idle_timer_period = max_t(s_time_t, IDLE_TIMER_PERIOD_MIN,
> > +                                  idle_timer_period -
> > IDLE_TIMER_PERIOD_DECR);
> > +    }
> 
> Pointless braces. And do you really need min_t()/max_t() here,
> rather than just min()/max()?
> 
I probably can. This must be a leftover from a version when the time of
idle_timer_period, and of the IDLE_TIMER_PERIOD_{MIN,MAX} constants
wasn't matching.

Sorry!

Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

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

Re: [PATCH 1/3] xen: timers: don't miss a timer event because of stop_timer()

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 4592 bytes --]

On Tue, 2017-09-26 at 08:59 -0600, Jan Beulich wrote:
> > > > On 15.09.17 at 20:01, <dario.faggioli@citrix.com> wrote:
> > --- a/xen/common/timer.c
> > +++ b/xen/common/timer.c
> > @@ -217,7 +217,7 @@ static inline void activate_timer(struct timer
> > *timer)
> >      timer->status = TIMER_STATUS_invalid;
> >      list_del(&timer->inactive);
> >  
> > -    if ( add_entry(timer) )
> > +    if ( add_entry(timer) || timer->expires <= NOW() )
> >          cpu_raise_softirq(timer->cpu, TIMER_SOFTIRQ);
> >  }
> 
> I'm not convinced of this change - it's unrelated to what title and
> description say, 
>
You're right. This should either be mentioned, or dropped (or live in
another patch).

> > @@ -326,7 +326,17 @@ void stop_timer(struct timer *timer)
> >          return;
> >  
> >      if ( active_timer(timer) )
> > -        deactivate_timer(timer);
> > +    {
> > +        /*
> > +         * If the timer is expired already, 'call' the softirq
> > handler to
> > +         * execute it (it will leave it inactive after that). If
> > not, just
> > +         * deactivate it.
> > +         */
> > +        if ( timer->expires <= NOW() )
> > +            cpu_raise_softirq(timer->cpu, TIMER_SOFTIRQ);
> > +        else
> > +            deactivate_timer(timer);
> > +    }
> 
> Isn't it reasonable to expect current behavior, i.e. the timer not
> raising any events other than those already in progress?
> 
Well, if we managed to get to here, with the timer that is both:
- active,
- with its expiry time in the past,

it means there is an event that *is* in progress right now (i.e., we're
stopping the timer on the path that goes from the interrupt that raised
TIMER_SOFTIRQ, and the timer softirq handler).

So, basically, I'm trying to achieve exactly what you call reasonable:
servicing an event which is in progress. :-)

The alternative is that the event happened, but we somehow managed to
miss running the timer handler for it, and we realize this only now,
potentially a long time after the miss. This scenario, as far as I can
tell, can't happen, but if it could/does, I'd still say running the
handler late is better than not running it at all.

> Additionally I'm not convinced the changed actually does what you
> want: What if NOW() becomes equal to timer->expires immediately
> after you've managed to obtain its value, before execution makes
> it into deactivate_timer()? IOW you're shrinking a time window which
> (I think) you really mean to eliminate.
> 
Well, I certainly don't like the window to be there, and closing it
would be ideal, IMO.

However, in this patch, I'm addressing the specific situation of when
we are stopping a timer for which an interrupt has already fired, the
interrupt's ISR has already raised TIMER_SOFTIRQ, and yet we don't run
the handler.

Yes, if an interrupt is about to be raised, and/or it arrives _while_
we are inside this function (after the check), or already in
deactivate_timer(), we also end up not running the handler.

I guess these can be seen as two aspects of the same problem, or as
conceptually different issues, but whatever you consider them, getting
rid of the former is something I consider an improvement.

I certainly can try to state the problem better, and describe the
situation more clearly in the changelog.

> Furthermore, taking both changes together: What if the same
> you try to address in stop_timer() happens in set_timer()? Wouldn't
> it then be only consistent to also require a timer even to fire for
> the
> old expiry value, before the new one is being set?
> 
Yes, personally, I think that, whenever it is that we figure out that
we missed handling a timer interrupt, we should run it then.

Unfortunately, for achieving this, e.g., in the set_timer() case, I
don't see much alternatives to call execute_timer() right in there. But
that would violate the current invariant that execute_timer() only run
from the TIMER_SOFTIRQ handler... which is probably doable, but is at
the same time unrelated to the problem I'm tackling here, and I would
rather do it in a dedicated series.

Regards,
Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

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

Re: [PATCH 1/3] xen: timers: don't miss a timer event because of stop_timer()

[Jan Beulich]

>>> On 26.09.17 at 20:11, <dario.faggioli@citrix.com> wrote:
> On Tue, 2017-09-26 at 08:59 -0600, Jan Beulich wrote:
>> > > > On 15.09.17 at 20:01, <dario.faggioli@citrix.com> wrote:
>> > @@ -326,7 +326,17 @@ void stop_timer(struct timer *timer)
>> >          return;
>> >  
>> >      if ( active_timer(timer) )
>> > -        deactivate_timer(timer);
>> > +    {
>> > +        /*
>> > +         * If the timer is expired already, 'call' the softirq
>> > handler to
>> > +         * execute it (it will leave it inactive after that). If
>> > not, just
>> > +         * deactivate it.
>> > +         */
>> > +        if ( timer->expires <= NOW() )
>> > +            cpu_raise_softirq(timer->cpu, TIMER_SOFTIRQ);
>> > +        else
>> > +            deactivate_timer(timer);
>> > +    }
>> 
>> Isn't it reasonable to expect current behavior, i.e. the timer not
>> raising any events other than those already in progress?
>> 
> Well, if we managed to get to here, with the timer that is both:
> - active,
> - with its expiry time in the past,
> 
> it means there is an event that *is* in progress right now (i.e., we're
> stopping the timer on the path that goes from the interrupt that raised
> TIMER_SOFTIRQ, and the timer softirq handler).
> 
> So, basically, I'm trying to achieve exactly what you call reasonable:
> servicing an event which is in progress. :-)

I'm afraid I don't understand - if the processing of the timer is
already in progress, why would you need to raise another
softirq? Wouldn't it suffice to simply skip deactivate_timer() then?
This raising of the softirq is what made me imply that, perhaps
due to some minimal skew e.g. resulting from rounding, you
assume the interrupt did not fire yet, which I'd then call the
timer event not being in progress (yet).

> The alternative is that the event happened, but we somehow managed to
> miss running the timer handler for it, and we realize this only now,
> potentially a long time after the miss. This scenario, as far as I can
> tell, can't happen, but if it could/does, I'd still say running the
> handler late is better than not running it at all.

Well, as said - what's better may very well depend on the particular
use case. As long as it's not clearly written down what the intended
behavior is, both behaviors are acceptable imo.

In the end what I think I'm missing is a clear description of an actual
case where the current behavior causes breakage (plus of course
an explanation why the new behavior is unlikely to cause issues with
existing users).

>> Additionally I'm not convinced the changed actually does what you
>> want: What if NOW() becomes equal to timer->expires immediately
>> after you've managed to obtain its value, before execution makes
>> it into deactivate_timer()? IOW you're shrinking a time window which
>> (I think) you really mean to eliminate.
>> 
> Well, I certainly don't like the window to be there, and closing it
> would be ideal, IMO.
> 
> However, in this patch, I'm addressing the specific situation of when
> we are stopping a timer for which an interrupt has already fired, the
> interrupt's ISR has already raised TIMER_SOFTIRQ, and yet we don't run
> the handler.
> 
> Yes, if an interrupt is about to be raised, and/or it arrives _while_
> we are inside this function (after the check), or already in
> deactivate_timer(), we also end up not running the handler.
> 
> I guess these can be seen as two aspects of the same problem, or as
> conceptually different issues, but whatever you consider them, getting
> rid of the former is something I consider an improvement.

It may be improvement, yes, but if there is
- no actual case breaking with the current code, I don't see
  the need for the change,
- an actual case breaking with the current code, it'll still break
  with the change as the window was merely shrunk.

>> Furthermore, taking both changes together: What if the same
>> you try to address in stop_timer() happens in set_timer()? Wouldn't
>> it then be only consistent to also require a timer even to fire for
>> the
>> old expiry value, before the new one is being set?
>> 
> Yes, personally, I think that, whenever it is that we figure out that
> we missed handling a timer interrupt, we should run it then.
> 
> Unfortunately, for achieving this, e.g., in the set_timer() case, I
> don't see much alternatives to call execute_timer() right in there. But
> that would violate the current invariant that execute_timer() only run
> from the TIMER_SOFTIRQ handler... which is probably doable, but is at
> the same time unrelated to the problem I'm tackling here, and I would
> rather do it in a dedicated series.

Indeed forcing the handler to run in the set_timer() case is more
difficult, yet as said - if there's a window to be concerned about,
the concern should be regarding all such windows, or none of
them imo.

Jan

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

Re: [PATCH 2/3] xen: RCU: make the period of the idle timer configurable.

[Jan Beulich]

>>> On 26.09.17 at 19:48, <dario.faggioli@citrix.com> wrote:
> On Tue, 2017-09-26 at 09:14 -0600, Jan Beulich wrote:
>> > > > On 15.09.17 at 20:01, <dario.faggioli@citrix.com> wrote:
>> > --- a/xen/common/rcupdate.c
>> > +++ b/xen/common/rcupdate.c
>> > +    int ret = 0;
>> > +
>> > +    if ( MILLISECS(period) > IDLE_TIMER_PERIOD_MAX )
>> > +    {
>> > +        printk("WARNING: rcu_idle_timer_period_ms must be <
>> > %"PRI_stime"\n",
>> > +               IDLE_TIMER_PERIOD_MAX / MILLISECS(1));
>> > +        ret = -EINVAL;
>> > +    }
>> > +    else
>> > +        idle_timer_period = MILLISECS(period);
>> > +
>> > +    printk("RCU idle timer period: %lums\n", period);
>> > +
>> > +    return ret;
>> > +}
>> > +custom_param("rcu_idle_timer_period_ms", parse_idle_timer_period);
>> 
>> Does this really need handling as custom_param(). I.e. wouldn't
>> integer_param() plus sanitizing in rcu_init() suffice? 
>>
> I did it like that in the first place. But then I did not like the
> overall look of the patch, so I changed the approach.
> 
> I can put it back together the integer_param() variant, and you'll tell
> me which one you like better.
> 
>> That would
>> also make the log message be printed uniformly - merely echoing
>> the value from the command line doesn't look very useful to me.
>>
> Mmm.. Sorry, but I don't get this part.

In this version of the patch you log the message if and only if
the command line option was specified. This means that
- without command line option the log won't tell us anything
- with a valid command line option the log will tell us the same
  thing twice (once via the logged command line, and another
  time in the message you add)
- with an invalid command line option you'll be issuing both a
  warning and the message reporting the used value
I can live with the new message duplicating the command line
information (i.e. there's no point in suppressing the new log
message if the command line option was used), but (a) there
shouldn't be two messages (or really three, as returning
-EINVAL will trigger another message in the caller) and (b) if
knowing the period of the timer is something you consider
worth logging, it should be logged even when there's no
command line option.

Jan


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

Re: [PATCH 1/3] xen: timers: don't miss a timer event because of stop_timer()

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 4151 bytes --]

On Wed, 2017-09-27 at 02:20 -0600, Jan Beulich wrote:
> > > > On 26.09.17 at 20:11, <dario.faggioli@citrix.com> wrote:
> > it means there is an event that *is* in progress right now (i.e.,
> > we're
> > stopping the timer on the path that goes from the interrupt that
> > raised
> > TIMER_SOFTIRQ, and the timer softirq handler).
> > 
> > So, basically, I'm trying to achieve exactly what you call
> > reasonable:
> > servicing an event which is in progress. :-)
> 
> I'm afraid I don't understand - if the processing of the timer is
> already in progress, why would you need to raise another
> softirq? Wouldn't it suffice to simply skip deactivate_timer() then?
>
Ah, yes! In the use case I'm trying to fix, that's indeed not
necessary, as it has already been risen.

Basically, as it's harmless to re-raise it, I thought to do so, with
the aim of making it easier to understand what the code was trying to
achieve. But now I actually agree with you that the effect is quite the
opposite. :-(

I will get rid of the re-raising, and explain the situation better in
the both the comment and the changelog.

> This raising of the softirq is what made me imply that, perhaps
> due to some minimal skew e.g. resulting from rounding, you
> assume the interrupt did not fire yet, which I'd then call the
> timer event not being in progress (yet).
> 
Sure, I totally see it now, and I also totally agree.

> In the end what I think I'm missing is a clear description of an
> actual
> case where the current behavior causes breakage (plus of course
> an explanation why the new behavior is unlikely to cause issues with
> existing users).
> 
So, the problem is that the handler of the RCU idle_timer, introduced
by 2b936ea7b716dc1a13c ("xen: RCU: avoid busy waiting until the end of
grace period."), never runs.

And that is because the following happens:
- the CPU wants to go idle
- sched_tick_suspend()
    rcu_idle_timer_start()
      set_timer(RCU_idle_timer)
- the CPU goes idle
  ... ... ...
- RCU_idle_timer's IRQ arrives
- the CPU wakes up
- raise_softirq(TIMER_SOFTIRQ)
- sched_tick_resume()
    rcu_idle_timer_stop()
      stop_timer(RCU_idle_timer)
        deactivate_timer(RCU_idle_timer)
          remove_entry(RCU_idle_timer) // timer out of heap/list
- do_softirq() (we are inside idle_loop())
- softirq_handlers[TIMER_SOFTIRQ]()
- timer_softirq_action()
    // but the timer is not in the heap/list!

Now, as far as the purpose of that patch goes, we're fine. In fact,
there, we "only" needed to avoid that a certain CPU (because of its
role in the grace period) would sleep too long/indefinitely. And the
fact that the CPU does wake up, because of the timer interrupt, is
enough.

However, it's been asked to try to make the logic a bit more clever,
basically for preventing RCU_idle_timer from firing too often. And
that's actually what this series is doing. But now, in order to achieve
that, I do need the timer handler to run.


About the (lack of) breakage of existing use cases. Well, hard to tell
like this, but I'd say that, if, right now, we are not missing any
timer event handling, it means that this situation --where you stop the
timer in between raise_softirq(TIMER_SOFTIRQ) and
softirq_handler[TIMER_SOFTIRQ]()-- never happens.

Inspecting the code, in fact, I can't spot any stop_timer() happening
within that window, which I think it means we're fine (IOW,
RCU_idle_timer is the first, and for now only, timer that is stopped on
the CPU wake-up-from-idle path).

It is important (in the new version of this patch) for deactivation to
be skipped only in stop_timer(), and not, e.g., in kill_timer(). As, if
someone, in future, will want to kill and free the timer during the
window, then in that case the handler *must* not run.

Makes sense?

Thanks and Regards,
Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

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

Re: [PATCH 1/3] xen: timers: don't miss a timer event because of stop_timer()

[Jan Beulich]

>>> On 27.09.17 at 12:18, <dario.faggioli@citrix.com> wrote:
> On Wed, 2017-09-27 at 02:20 -0600, Jan Beulich wrote:
>> In the end what I think I'm missing is a clear description of an
>> actual
>> case where the current behavior causes breakage (plus of course
>> an explanation why the new behavior is unlikely to cause issues with
>> existing users).
>> 
> So, the problem is that the handler of the RCU idle_timer, introduced
> by 2b936ea7b716dc1a13c ("xen: RCU: avoid busy waiting until the end of
> grace period."), never runs.
> 
> And that is because the following happens:
> - the CPU wants to go idle
> - sched_tick_suspend()
>     rcu_idle_timer_start()
>       set_timer(RCU_idle_timer)
> - the CPU goes idle
>   ... ... ...
> - RCU_idle_timer's IRQ arrives
> - the CPU wakes up
> - raise_softirq(TIMER_SOFTIRQ)
> - sched_tick_resume()
>     rcu_idle_timer_stop()
>       stop_timer(RCU_idle_timer)
>         deactivate_timer(RCU_idle_timer)
>           remove_entry(RCU_idle_timer) // timer out of heap/list
> - do_softirq() (we are inside idle_loop())
> - softirq_handlers[TIMER_SOFTIRQ]()
> - timer_softirq_action()
>     // but the timer is not in the heap/list!

But this is an extremely special case, not something likely to
happen anywhere else. Hence I wonder whether it wouldn't
be better to handle the special case in a special way, rather
than making generic code fit the special case. Or wait -
wouldn't all you need be to avoid calling stop_timer() in the
call tree above, if the timer's expiry has passed (suitably
explained in a comment)?

Jan


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

Re: [PATCH 1/3] xen: timers: don't miss a timer event because of stop_timer()

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 2218 bytes --]

On Wed, 2017-09-27 at 04:30 -0600, Jan Beulich wrote:
> > > > On 27.09.17 at 12:18, <dario.faggioli@citrix.com> wrote:
> 
> > And that is because the following happens:
> > - the CPU wants to go idle
> > - sched_tick_suspend()
> >     rcu_idle_timer_start()
> >       set_timer(RCU_idle_timer)
> > - the CPU goes idle
> >   ... ... ...
> > - RCU_idle_timer's IRQ arrives
> > - the CPU wakes up
> > - raise_softirq(TIMER_SOFTIRQ)
> > - sched_tick_resume()
> >     rcu_idle_timer_stop()
> >       stop_timer(RCU_idle_timer)
> >         deactivate_timer(RCU_idle_timer)
> >           remove_entry(RCU_idle_timer) // timer out of heap/list
> > - do_softirq() (we are inside idle_loop())
> > - softirq_handlers[TIMER_SOFTIRQ]()
> > - timer_softirq_action()
> >     // but the timer is not in the heap/list!
> 
> But this is an extremely special case, not something likely to
> happen anywhere else. Hence I wonder whether it wouldn't
> be better to handle the special case in a special way, rather
> than making generic code fit the special case.
>
Well, yes. As said, this "new" timer is the first, and for now the
only, that follow this pattern. And I also agree that this is not
something we must expect to see to happen much more (if at all).

Still, I continue to think that with a timer already expired, its IRQ
already delivered and handled and the relative TIMER_SOFTIRQ already
risen, we should arrange for the timer handler to run, in the general
case.

>  Or wait -
> wouldn't all you need be to avoid calling stop_timer() in the
> call tree above, if the timer's expiry has passed (suitably
> explained in a comment)?
> 
Yes. For the reason stated above, I addressed the problem at the
generic code level. If that doesn't fly, I'll do like this. I had
thought about that, and although I haven't tried, I think it works for
this case.

Thanks and Regards,
Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

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

Re: [PATCH 1/3] xen: timers: don't miss a timer event because of stop_timer()

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 1060 bytes --]

On Wed, 2017-09-27 at 15:39 +0200, Dario Faggioli wrote:
> On Wed, 2017-09-27 at 04:30 -0600, Jan Beulich wrote:
> >  Or wait -
> > wouldn't all you need be to avoid calling stop_timer() in the
> > call tree above, if the timer's expiry has passed (suitably
> > explained in a comment)?
> > 
> Yes. For the reason stated above, I addressed the problem at the
> generic code level. If that doesn't fly, I'll do like this. I had
> thought about that, and although I haven't tried, I think it works
> for
> this case.
> 
Anyway, given the timing, I'll send a v2 of this series, where I do
things as suggested above.

I may want to try again to persuade you (and others) that this needs to
be changed in timer's code. But that will be for another time. :-)

Thanks and Regards,
Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

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