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

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

[Dario Faggioli]

And now take 3.

v1:
 https://lists.xen.org/archives/html/xen-devel/2017-09/msg01855.html

v2:
 https://lists.xen.org/archives/html/xen-devel/2017-09/msg03515.html

I've just took care of Jan's comments to v2. Details in single patches.

Thanks and Regards,
Dario
---
Dario Faggioli (3):
      xen: RCU: let the RCU idle timer handler run
      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 |   10 +++++
 xen/common/rcupdate.c               |   67 ++++++++++++++++++++++++++++++++---
 xen/common/timer.c                  |   17 +++++++++
 xen/include/xen/timer.h             |    5 +++
 4 files changed, 93 insertions(+), 6 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 v3 1/3] xen: RCU: let the RCU idle timer handler run

[Dario Faggioli]

If stop_timer() is called between when the RCU
idle timer's interrupt arrives (and TIMER_SOFTIRQ is
raised) and when softirqs are checked and handled, the
timer is deactivated, and the handler never runs.

This happens to the RCU idle timer because stop_timer()
is called on it during the wakeup from idle (e.g., C-states,
on x86) path.

To fix that, we avoid calling stop_timer(), in case we see
that the timer itself is:
- still active,
- expired (i.e., it's expiry time is in the past).
In fact, that indicates (for this particular timer) that
it has fired, and we are just about to handle the TIMER_SOFTIRQ
(which will perform the timer deactivation and run its handler).

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>
---
Changes from v2:
- improved comment in rcu_idle_timer_stop();
- introduce a more generic timer_expires_before() function, and make
  timer_is_expired() a macro which does not take any time parameter.

Changes from v1:
- logic changed completely: instead of avoiding deactivate_timer() in
  stop_timer(), we avoid stop_timer() in rcu_idle_timer_stop() (if
  appropriate, of course).
---
 xen/common/rcupdate.c   |   19 ++++++++++++++++++-
 xen/common/timer.c      |   17 +++++++++++++++++
 xen/include/xen/timer.h |    5 +++++
 3 files changed, 40 insertions(+), 1 deletion(-)

diff --git a/xen/common/rcupdate.c b/xen/common/rcupdate.c
index 871936f..252e01b 100644
--- a/xen/common/rcupdate.c
+++ b/xen/common/rcupdate.c
@@ -465,7 +465,24 @@ void rcu_idle_timer_stop()
         return;
 
     rdp->idle_timer_active = false;
-    stop_timer(&rdp->idle_timer);
+
+    /*
+     * In general, as the CPU is becoming active again, we don't need the
+     * idle timer, and so we want to stop it.
+     *
+     * However, in case we are here because idle_timer has (just) fired and
+     * has woken up the CPU, we skip stop_timer() now. In fact, when a CPU
+     * wakes up from idle, this code always runs before do_softirq() has the
+     * chance to check and deal with TIMER_SOFTIRQ. And if we stop the timer
+     * now, the TIMER_SOFTIRQ handler will see it as inactive, and will not
+     * call rcu_idle_timer_handler().
+     *
+     * Therefore, if we see that the timer is expired already, we leave it
+     * alone. The TIMER_SOFTIRQ handler will then run the timer routine, and
+     * deactivate it.
+     */
+    if ( !timer_is_expired(&rdp->idle_timer) )
+        stop_timer(&rdp->idle_timer);
 }
 
 static void rcu_idle_timer_handler(void* data)
diff --git a/xen/common/timer.c b/xen/common/timer.c
index d9ff669..fa45db2 100644
--- a/xen/common/timer.c
+++ b/xen/common/timer.c
@@ -332,6 +332,23 @@ void stop_timer(struct timer *timer)
 }
 
 
+bool timer_expires_before(struct timer *timer, s_time_t t)
+{
+    unsigned long flags;
+    bool ret = false;
+
+    if ( !timer_lock_irqsave(timer, flags) )
+        return ret;
+
+    if ( active_timer(timer) && timer->expires <= t )
+        ret = true;
+
+    timer_unlock_irqrestore(timer, flags);
+
+    return ret;
+}
+
+
 void migrate_timer(struct timer *timer, unsigned int new_cpu)
 {
     unsigned int old_cpu;
diff --git a/xen/include/xen/timer.h b/xen/include/xen/timer.h
index 9531800..4513260 100644
--- a/xen/include/xen/timer.h
+++ b/xen/include/xen/timer.h
@@ -70,6 +70,11 @@ void set_timer(struct timer *timer, s_time_t expires);
  */
 void stop_timer(struct timer *timer);
 
+/* True if a timer is active, and its expiry time is earlier than t. */
+bool timer_expires_before(struct timer *timer, s_time_t t);
+
+#define timer_is_expired(t) timer_expires_before(t, NOW())
+
 /* Migrate a timer to a different CPU. The timer may be currently active. */
 void migrate_timer(struct timer *timer, unsigned int new_cpu);
 


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

Re: [PATCH v3 1/3] xen: RCU: let the RCU idle timer handler run

[Jan Beulich]

>>> On 28.09.17 at 19:06, <dario.faggioli@citrix.com> wrote:
> --- a/xen/common/timer.c
> +++ b/xen/common/timer.c
> @@ -332,6 +332,23 @@ void stop_timer(struct timer *timer)
>  }
>  
>  
> +bool timer_expires_before(struct timer *timer, s_time_t t)
> +{
> +    unsigned long flags;
> +    bool ret = false;
> +
> +    if ( !timer_lock_irqsave(timer, flags) )
> +        return ret;

I think it would be easier to follow if you used just "false" here.

> +    if ( active_timer(timer) && timer->expires <= t )
> +        ret = true;

In which case this could then be a simple assignment, with the
variable's initializer dropped.

> +    timer_unlock_irqrestore(timer, flags);
> +
> +    return ret;
> +}
> +
> +
>  void migrate_timer(struct timer *timer, unsigned int new_cpu)

Please don't introduce further double blank lines. Instead, insert the
new function between the two existing ones.

With at least the latter addressed
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Of course both should be easy to take care of while committing,
should no other reason arise for sending v4.

Jan


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

Re: [PATCH v3 1/3] xen: RCU: let the RCU idle timer handler run

[Dario Faggioli]

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

On Mon, 2017-10-09 at 03:49 -0600, Jan Beulich wrote:
> > > > On 28.09.17 at 19:06, <dario.faggioli@citrix.com> wrote:
> > 
> With at least the latter addressed
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> Of course both should be easy to take care of while committing,
> should no other reason arise for sending v4.
> 
Which, AFAICS, you've done, and checked in the series.

Thanks!
Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli

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

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

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

[PATCH v3 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>
---
Changes from v2:
- use '!= 0' and "(0,..]", while sanitizing the boot parameter value;
- move the param variable, as well as the integer_param() inside rcu_init().

Changes from v1:
- "-" instead of "_" in the boot parameter name;
- enforce a minimum value as well;
- use integer_param(), instead of custom_param().
---
 docs/misc/xen-command-line.markdown |   10 ++++++++++
 xen/common/rcupdate.c               |   28 ++++++++++++++++++++++++----
 2 files changed, 34 insertions(+), 4 deletions(-)

diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
index 9797c8d..3551143 100644
--- a/docs/misc/xen-command-line.markdown
+++ b/docs/misc/xen-command-line.markdown
@@ -1422,6 +1422,16 @@ 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
+100, and it can't be 0.
+
 ### 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 252e01b..f07185f 100644
--- a/xen/common/rcupdate.c
+++ b/xen/common/rcupdate.c
@@ -110,10 +110,16 @@ 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;
 
 static DEFINE_PER_CPU(struct rcu_data, rcu_data);
 
@@ -453,7 +459,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;
 }
 
@@ -571,6 +577,20 @@ static struct notifier_block cpu_nfb = {
 void __init rcu_init(void)
 {
     void *cpu = (void *)(long)smp_processor_id();
+    static unsigned int __initdata idle_timer_period_ms =
+                                    IDLE_TIMER_PERIOD_DEFAULT / MILLISECS(1);
+    integer_param("rcu-idle-timer-period-ms", idle_timer_period_ms);
+
+    /* We don't allow 0, or anything higher than IDLE_TIMER_PERIOD_MAX */
+    if ( idle_timer_period_ms == 0 ||
+         idle_timer_period_ms > IDLE_TIMER_PERIOD_MAX / MILLISECS(1) )
+    {
+        idle_timer_period_ms = IDLE_TIMER_PERIOD_DEFAULT / MILLISECS(1);
+        printk("WARNING: rcu-idle-timer-period-ms outside of "
+               "(0,%"PRI_stime"]. Resetting it to %u.\n",
+               IDLE_TIMER_PERIOD_MAX / MILLISECS(1), idle_timer_period_ms);
+    }
+    idle_timer_period = MILLISECS(idle_timer_period_ms);
 
     cpumask_clear(&rcu_ctrlblk.idle_cpumask);
     cpu_callback(&cpu_nfb, CPU_UP_PREPARE, cpu);


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

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

[Jan Beulich]

>>> On 28.09.17 at 19:06, <dario.faggioli@citrix.com> wrote:
> 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>

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



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

[PATCH v3 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>
Reviewed-by: 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>
---
Changes from v1:
- removed pointless braches;
- switched from min_t() to min(), and from max_t() to max().
---
 xen/common/rcupdate.c |   20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/xen/common/rcupdate.c b/xen/common/rcupdate.c
index f07185f..3517790 100644
--- a/xen/common/rcupdate.c
+++ b/xen/common/rcupdate.c
@@ -118,9 +118,21 @@ 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;
 
+/*
+ * 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;
@@ -493,8 +505,14 @@ 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(idle_timer_period + IDLE_TIMER_PERIOD_INCR,
+                                IDLE_TIMER_PERIOD_MAX);
+    else
+        idle_timer_period = max(idle_timer_period - IDLE_TIMER_PERIOD_DECR,
+                                IDLE_TIMER_PERIOD_MIN);
 }
 
 void rcu_check_callbacks(int cpu)


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