| Message ID | 150659376652.4057.12723423135216244659.stgit@Solace.fritz.box (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> 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
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
>>> 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
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);
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(-)