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