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

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

[Dario Faggioli]

Hello,

take 2 of this series. v1 is here:

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

In patch 2 and patch 3, I took care of Jan's review comments. That meant, in
patch 2, changing how the boot parameter is parsed and handled, from using a
custom_param() to integer_param(). The rest of the changes were simple and
mechanical.

Patch 1 is the one that changed most. Radically, I would say. :-) In fact, I
think we should leave aside, for now, the debate of whether or not it is
necessary to deal with the 'missed' timer handler invocation, in a generic way,
at the timer's code level. For now, it only cause problems to the --rather
special indeed-- RCU idle timer, and there is an easy way to avoid that,
affecting that specific timer only.

So, I'm going for that now. We can re-open the debate later, when not in almost freezed state. :-)

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               |   64 ++++++++++++++++++++++++++++++++---
 xen/common/timer.c                  |   17 +++++++++
 xen/include/xen/timer.h             |    3 ++
 4 files changed, 88 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 v2 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 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   |   16 +++++++++++++++-
 xen/common/timer.c      |   17 +++++++++++++++++
 xen/include/xen/timer.h |    3 +++
 3 files changed, 35 insertions(+), 1 deletion(-)

diff --git a/xen/common/rcupdate.c b/xen/common/rcupdate.c
index 871936f..4a02cdd 100644
--- a/xen/common/rcupdate.c
+++ b/xen/common/rcupdate.c
@@ -465,7 +465,21 @@ 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, if we stop
+     * it, then the TIMER_SOFTIRQ handler wouldn't find idle_timer among the
+     * active timers any longer, and hence won't call rcu_idle_timer_handler().
+     *
+     * Therefore, if we see that the timer is expired already, leave it alone.
+     * It will be finally deactiveted by the TIMER_SOFTIRQ handler.
+     */
+    if ( !timer_is_expired(&rdp->idle_timer, NOW()) )
+        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..a768aa3 100644
--- a/xen/common/timer.c
+++ b/xen/common/timer.c
@@ -332,6 +332,23 @@ void stop_timer(struct timer *timer)
 }
 
 
+bool timer_is_expired(struct timer *timer, s_time_t now)
+{
+    unsigned long flags;
+    bool ret = false;
+
+    if ( !timer_lock_irqsave(timer, flags) )
+        return ret;
+
+    if ( active_timer(timer) && timer->expires <= now )
+        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..a095007 100644
--- a/xen/include/xen/timer.h
+++ b/xen/include/xen/timer.h
@@ -70,6 +70,9 @@ void set_timer(struct timer *timer, s_time_t expires);
  */
 void stop_timer(struct timer *timer);
 
+/* True if a timer is active, but with its expiry time before now. */
+bool timer_is_expired(struct timer *timer, s_time_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

[PATCH v2 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 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..8eb800e 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; minimum is 1ms,
+maximum is 100.
+
 ### 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 4a02cdd..2381df1 100644
--- a/xen/common/rcupdate.c
+++ b/xen/common/rcupdate.c
@@ -110,10 +110,20 @@ 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 unsigned int __initdata idle_timer_period_ms =
+                                    IDLE_TIMER_PERIOD_DEFAULT / MILLISECS(1);
+integer_param("rcu-idle-timer-period-ms", idle_timer_period_ms);
 
 static DEFINE_PER_CPU(struct rcu_data, rcu_data);
 
@@ -453,7 +463,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;
 }
 
@@ -569,6 +579,16 @@ void __init rcu_init(void)
 {
     void *cpu = (void *)(long)smp_processor_id();
 
+    /* We don't allow 0, or anything higher than IDLE_TIMER_PERIOD_MAX */
+    if ( idle_timer_period_ms < 1 ||
+         idle_timer_period_ms > IDLE_TIMER_PERIOD_MAX / MILLISECS(1) )
+    {
+        printk("WARNING: rcu-idle-timer-period-ms outside of [%d,%ld]ms!\n",
+               1, IDLE_TIMER_PERIOD_MAX / MILLISECS(1));
+        idle_timer_period_ms = IDLE_TIMER_PERIOD_DEFAULT / MILLISECS(1);
+    }
+    idle_timer_period = MILLISECS(idle_timer_period_ms);
+
     cpumask_clear(&rcu_ctrlblk.idle_cpumask);
     cpu_callback(&cpu_nfb, CPU_UP_PREPARE, cpu);
     register_cpu_notifier(&cpu_nfb);


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

[PATCH v2 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>
---
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 2381df1..b2d0e2e 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;
 
@@ -125,6 +126,17 @@ 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);
 
+/*
+ * 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;
@@ -494,8 +506,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

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

[Jan Beulich]

>>> On 28.09.17 at 12:15, <dario.faggioli@citrix.com> wrote:
> --- a/xen/common/rcupdate.c
> +++ b/xen/common/rcupdate.c
> @@ -465,7 +465,21 @@ 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, if we stop
> +     * it, then the TIMER_SOFTIRQ handler wouldn't find idle_timer among the
> +     * active timers any longer, and hence won't call rcu_idle_timer_handler().

I think it would help if you said explicitly that the softirq run
necessarily happens after this code ran.

> +     * Therefore, if we see that the timer is expired already, leave it alone.
> +     * It will be finally deactiveted by the TIMER_SOFTIRQ handler.

deactivated

> --- a/xen/common/timer.c
> +++ b/xen/common/timer.c
> @@ -332,6 +332,23 @@ void stop_timer(struct timer *timer)
>  }
>  
>  
> +bool timer_is_expired(struct timer *timer, s_time_t now)

If you call the parameter now, why is it needed? Wouldn't it be
even more accurate if you instead used ...

> +{
> +    unsigned long flags;
> +    bool ret = false;
> +
> +    if ( !timer_lock_irqsave(timer, flags) )
> +        return ret;
> +
> +    if ( active_timer(timer) && timer->expires <= now )

... NOW() here?

Jan


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

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

[Jan Beulich]

>>> On 28.09.17 at 12:16, <dario.faggioli@citrix.com> wrote:
> @@ -569,6 +579,16 @@ void __init rcu_init(void)
>  {
>      void *cpu = (void *)(long)smp_processor_id();
>  
> +    /* We don't allow 0, or anything higher than IDLE_TIMER_PERIOD_MAX */
> +    if ( idle_timer_period_ms < 1 ||

The literal 1 here looks suspicious. How about simply refusing 0
(as well as too high values)? The also simply document the value
must be non-zero in the command line doc.

> +         idle_timer_period_ms > IDLE_TIMER_PERIOD_MAX / MILLISECS(1) )
> +    {
> +        printk("WARNING: rcu-idle-timer-period-ms outside of [%d,%ld]ms!\n",
> +               1, IDLE_TIMER_PERIOD_MAX / MILLISECS(1));

Clearly the %d can be literal 1 if the above literal 1 was to stay.
If you follow my suggestion, use "(0," instead. As to the %ld -
wouldn't that rather need to be PRI_stime (due to MILLISECS()
returning s_time_t)?

And then, as a cosmetic thing, idle_timer_period_ms now isn't
really needed outside of this function. I'd prefer if you moved it
and the integer_param() into this function, to limit their scopes
as much as possible.

Jan


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

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

[Jan Beulich]

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


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

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

[Dario Faggioli]

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

On Thu, 2017-09-28 at 06:59 -0600, Jan Beulich wrote:
> > > > On 28.09.17 at 12:15, <dario.faggioli@citrix.com> wrote:
> > --- a/xen/common/rcupdate.c
> > +++ b/xen/common/rcupdate.c
> > @@ -465,7 +465,21 @@ 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, if
> > we stop
> > +     * it, then the TIMER_SOFTIRQ handler wouldn't find idle_timer
> > among the
> > +     * active timers any longer, and hence won't call
> > rcu_idle_timer_handler().
> 
> I think it would help if you said explicitly that the softirq run
> necessarily happens after this code ran.
> 
Ok.

> > --- a/xen/common/timer.c
> > +++ b/xen/common/timer.c
> > @@ -332,6 +332,23 @@ void stop_timer(struct timer *timer)
> >  }
> >  
> >  
> > +bool timer_is_expired(struct timer *timer, s_time_t now)
> 
> If you call the parameter now, why is it needed? Wouldn't it be
> even more accurate if you instead used ...
> 
> > +{
> > +    unsigned long flags;
> > +    bool ret = false;
> > +
> > +    if ( !timer_lock_irqsave(timer, flags) )
> > +        return ret;
> > +
> > +    if ( active_timer(timer) && timer->expires <= now )
> 
> ... NOW() here?
> 
So, the cases where you happen to need the current time multiple times,
and you expect the difference between calling NOW() repeatedly, and
using a value sampled at the beginning is small enough, or does not
matter (and therefore you decide to save the overhead).

foo()
{
 s_time_t now = NOW();
 ...
 bar(now);
 ...
 bar2(barbar, now);
 ...
 if ( timer_is_expired(timer, now) )
 {
  ...
 }
}

This is something we do, some of the times. And a function that takes a
'now' parameter, allows both use cases: the (more natural?) one, where
you pass it NOW(), and the least-overhead one, where you pass it a
cached value of NOW().

But I don't feel like arguing too much about this (especially now that
this is patch is the only use case).

If the problem is "just" the parameter (or maybe both the parameter's
and the function's) name(s), I 'd be happy to change the parameter name
to 't', or 'time' (and the function to 'timer_expires_before()'), and
this is my preference.

But if you strongly prefer it to just use NOW() inside, I'll go for it.

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 v2 2/3] xen: RCU: make the period of the idle timer configurable.

[Dario Faggioli]

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

On Thu, 2017-09-28 at 07:06 -0600, Jan Beulich wrote:
> > > > On 28.09.17 at 12:16, <dario.faggioli@citrix.com> wrote:
> > 
> > @@ -569,6 +579,16 @@ void __init rcu_init(void)
> >  {
> >      void *cpu = (void *)(long)smp_processor_id();
> >  
> > +    /* We don't allow 0, or anything higher than
> > IDLE_TIMER_PERIOD_MAX */
> > +    if ( idle_timer_period_ms < 1 ||
> 
> The literal 1 here looks suspicious. How about simply refusing 0
> (as well as too high values)? The also simply document the value
> must be non-zero in the command line doc.
> 
Ok, sure.

> > +         idle_timer_period_ms > IDLE_TIMER_PERIOD_MAX /
> > MILLISECS(1) )
> > +    {
> > +        printk("WARNING: rcu-idle-timer-period-ms outside of
> > [%d,%ld]ms!\n",
> > +               1, IDLE_TIMER_PERIOD_MAX / MILLISECS(1));
> 
> Clearly the %d can be literal 1 if the above literal 1 was to stay.
>
Yes it can. It's actually rather ugly to look at it, the way I managed
to write it... Ewww... sorry! :-P

> If you follow my suggestion, use "(0," instead. 
>
Yeah, I like this too. I was afraid it was a bit too formal, that not
everyone would understand it, but I guess it's actually fine.

> As to the %ld -
> wouldn't that rather need to be PRI_stime (due to MILLISECS()
> returning s_time_t)?
> 
Yes.

> And then, as a cosmetic thing, idle_timer_period_ms now isn't
> really needed outside of this function. I'd prefer if you moved it
> and the integer_param() into this function, to limit their scopes
> as much as possible.
> 
Ok. idle_timer_period_ms still wants to go into __initdata, right?

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 v2 1/3] xen: RCU: let the RCU idle timer handler run

[Jan Beulich]

>>> On 28.09.17 at 16:08, <dario.faggioli@citrix.com> wrote:
> If the problem is "just" the parameter (or maybe both the parameter's
> and the function's) name(s), I 'd be happy to change the parameter name
> to 't', or 'time' (and the function to 'timer_expires_before()'), and
> this is my preference.
> 
> But if you strongly prefer it to just use NOW() inside, I'll go for it.

I'm fine with either, just not "now" when other than "NOW()"
may be passed, or when the caller is expected to always
pass NOW().

Jan


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

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

[Jan Beulich]

>>> On 28.09.17 at 16:58, <dario.faggioli@citrix.com> wrote:
> On Thu, 2017-09-28 at 07:06 -0600, Jan Beulich wrote:
>> And then, as a cosmetic thing, idle_timer_period_ms now isn't
>> really needed outside of this function. I'd prefer if you moved it
>> and the integer_param() into this function, to limit their scopes
>> as much as possible.
>> 
> Ok. idle_timer_period_ms still wants to go into __initdata, right?

Sure.

Jan


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

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

[Dario Faggioli]

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

On Thu, 2017-09-28 at 07:06 -0600, Jan Beulich wrote:
> > > > On 28.09.17 at 12:16, <dario.faggioli@citrix.com> wrote:
> > 
> And then, as a cosmetic thing, idle_timer_period_ms now isn't
> really needed outside of this function. I'd prefer if you moved it
> and the integer_param() into this function, to limit their scopes
> as much as possible.
> 
On an unrelated (to this series) note, does this means that patches
like the one below are welcome/accepted?

Not that I plan to start sending them (not right now, at least). I was
rather thinking that it could be a nice bite project for beginners and
GSoC/Outreacy applicants.

Dario
---
diff --git a/xen/arch/x86/acpi/boot.c b/xen/arch/x86/acpi/boot.c
index 8e6c96d..a6cb263 100644
--- a/xen/arch/x86/acpi/boot.c
+++ b/xen/arch/x86/acpi/boot.c
@@ -50,10 +50,6 @@ bool __initdata acpi_ht = true; /* enable HT */
 bool __initdata acpi_lapic;
 bool __initdata acpi_ioapic;
 
-/* acpi_skip_timer_override: Skip IRQ0 overrides. */
-static bool __initdata acpi_skip_timer_override;
-boolean_param("acpi_skip_timer_override", acpi_skip_timer_override);
-
 static u64 acpi_lapic_addr __initdata = APIC_DEFAULT_PHYS_BASE;
 
 /* --------------------------------------------------------------------------
@@ -225,6 +221,9 @@ static int __init
 acpi_parse_int_src_ovr(struct acpi_subtable_header * header,
                       const unsigned long end)
 {
+       /* acpi_skip_timer_override: Skip IRQ0 overrides. */
+       static bool __initdata acpi_skip_timer_override;
+       boolean_param("acpi_skip_timer_override", acpi_skip_timer_override);
        struct acpi_madt_interrupt_override *intsrc =
                container_of(header, struct acpi_madt_interrupt_override,
                             header);
---
-- 
<<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 v2 2/3] xen: RCU: make the period of the idle timer configurable.

[Jan Beulich]

>>> Dario Faggioli <dario.faggioli@citrix.com> 09/28/17 7:17 PM >>>
>On Thu, 2017-09-28 at 07:06 -0600, Jan Beulich wrote:
>> > > > On 28.09.17 at 12:16, <dario.faggioli@citrix.com> wrote:
>> > 
>> And then, as a cosmetic thing, idle_timer_period_ms now isn't
>> really needed outside of this function. I'd prefer if you moved it
>> and the integer_param() into this function, to limit their scopes
>> as much as possible.
>> 
>On an unrelated (to this series) note, does this means that patches
>like the one below are welcome/accepted?

Yes, at least I would appreciate any variables in obviously too wide
scopes to be moved into the scope they're really needed in. But as
with most cleanup, I generally prefer such to be done on the go,
unless only very few instances of a certain pattern are left. Even if
such changes generally are relatively easy to review, they still need
looking at and hence consume review bandwidth better imo spent
elsewhere.

Jan


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