| Message ID | 1403393357-2070-5-git-send-email-fweisbec@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
On Sun, 22 Jun 2014, Frederic Weisbecker wrote: > From: Viresh Kumar <viresh.kumar@linaro.org> > > In lowres mode, hrtimers are serviced by the tick instead of a clock > event. It works well as long as the tick stays periodic but we must also > make sure that the hrtimers are serviced in dynticks mode targets, > pretty much like timer list timers do. > > Note that all dynticks modes are concerned: get_nohz_timer_target() > tries not to return remote idle CPUs but there is nothing to prevent > the elected target from entering dynticks idle mode until we lock its > base. It's also prefectly legal to enqueue hrtimers on full dynticks CPU. > > So there are two requirements to correctly handle dynticks: > > 1) On target's tick stop time, we must not delay the next tick further > the next hrtimer. > > 2) On hrtimer queue time. If the tick of the target is stopped, we must > wake up that CPU such that it sees the new hrtimer and recalculate > the next tick accordingly. > > The point 1 is well handled currently through get_nohz_timer_interrupt() and > cmp_next_hrtimer_event(). > > But the point 2 isn't handled at all. > > Fixing this is easy though as we have the necessary API ready for that. > All we need is to call wake_up_nohz_cpu() on a target when a newly > enqueued hrtimer requires tick rescheduling, like timer list timer do. > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com> > --- > kernel/hrtimer.c | 27 +++++++++++++++++++-------- > 1 file changed, 19 insertions(+), 8 deletions(-) > > diff --git a/kernel/hrtimer.c b/kernel/hrtimer.c > index 0e32d4e..5f30917 100644 > --- a/kernel/hrtimer.c > +++ b/kernel/hrtimer.c > @@ -1013,14 +1013,25 @@ int __hrtimer_start_range_ns(struct hrtimer *timer, ktime_t tim, > > leftmost = enqueue_hrtimer(timer, new_base); > > - /* > - * Only allow reprogramming if the new base is on this CPU. > - * (it might still be on another CPU if the timer was pending) > - * > - * XXX send_remote_softirq() ? > - */ > - if (leftmost && new_base->cpu_base == &__get_cpu_var(hrtimer_bases) > - && hrtimer_enqueue_reprogram(timer, new_base)) { > + if (!leftmost) { > + unlock_hrtimer_base(timer, &flags); > + return ret; > + } > + > + if (!hrtimer_is_hres_active(timer)) { > + /* > + * Kick to reschedule the next tick to handle the new timer > + * on dynticks target. > + */ > + wake_up_nohz_cpu(base->cpu_base->cpu); The timer is queued on new_base not on base. Thanks, tglx -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
On 22 June 2014 19:06, Thomas Gleixner <tglx@linutronix.de> wrote: > On Sun, 22 Jun 2014, Frederic Weisbecker wrote: >> + wake_up_nohz_cpu(base->cpu_base->cpu); > > The timer is queued on new_base not on base. Oops, thanks for spotting this bug. Will be fixed in next version. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
diff --git a/kernel/hrtimer.c b/kernel/hrtimer.c index 0e32d4e..5f30917 100644 --- a/kernel/hrtimer.c +++ b/kernel/hrtimer.c @@ -1013,14 +1013,25 @@ int __hrtimer_start_range_ns(struct hrtimer *timer, ktime_t tim, leftmost = enqueue_hrtimer(timer, new_base); - /* - * Only allow reprogramming if the new base is on this CPU. - * (it might still be on another CPU if the timer was pending) - * - * XXX send_remote_softirq() ? - */ - if (leftmost && new_base->cpu_base == &__get_cpu_var(hrtimer_bases) - && hrtimer_enqueue_reprogram(timer, new_base)) { + if (!leftmost) { + unlock_hrtimer_base(timer, &flags); + return ret; + } + + if (!hrtimer_is_hres_active(timer)) { + /* + * Kick to reschedule the next tick to handle the new timer + * on dynticks target. + */ + wake_up_nohz_cpu(base->cpu_base->cpu); + } else if (new_base->cpu_base == &__get_cpu_var(hrtimer_bases) && + hrtimer_enqueue_reprogram(timer, new_base)) { + /* + * Only allow reprogramming if the new base is on this CPU. + * (it might still be on another CPU if the timer was pending) + * + * XXX send_remote_softirq() ? + */ if (wakeup) { /* * We need to drop cpu_base->lock to avoid a