Re: [patch 2/3] timers: do not raise softirq unconditionally (spinlockless version)

Anna-Maria Gleixner
2019-06-11T11:45:23Z

On Fri, 31 May 2019, Anna-Maria Gleixner wrote:

[...]
> I will think about the problem and your solution a little bit more and
> give you feedback hopefully on monday.

I'm sorry for the delay. But now I'm able to give you a detailed feedback:

The general problem is, that your solution is customized to a single
use-case: preventing softirq raise but only if there is _no_ timer
pending. To reach this goal without using locks, overhead is added to the
formerly optimized add/mod path of a timer. With your code the timer
softirq is raised even when there is a pending timer which does not has to
be expired right now. But there have been requests in the past for this use
case already.

I discussed with Thomas several approaches during the last week how to
solve the unconditional softirq timer raise in a more general way without
loosing the fast add/mod path of a timer. The approach which seems to be
the best has a dependency on a timer code change from a push to pull model
which is still under developement (see v2 patchset:
http://lkml.kernel.org/r/20170418111102.490432548@linutronix.de). The
patchset v2 has several problems but we are working on a solution for those
problems right now. When the timer pull model is in place the approach to
solve the unconditional timer softirq raise could look like the following:

---8<---
The next_expiry value of timer_base struct is used to store the next expiry
value even if timer_base is not idle. Therefore it is udpated after adding
or modifying a timer and also at the end of timer softirq. In case timer
softirq does not has to be raised, the timer_base->clk is incremented to
prevent stale clocks. Checking whether timer softirq has to be raised
cannot be done lockless.

This code is not compile tested nor boot tested.

---
kernel/time/timer.c | 60 +++++++++++++++++++++++++++++++---------------------
1 file changed, 36 insertions(+), 24 deletions(-)

--- a/kernel/time/timer.c
+++ b/kernel/time/timer.c
@@ -552,37 +552,32 @@ static void
static void
trigger_dyntick_cpu(struct timer_base *base, struct timer_list *timer)
{
- if (!is_timers_nohz_active())
- return;
-
- /*
- * TODO: This wants some optimizing similar to the code below, but we
- * will do that when we switch from push to pull for deferrable timers.
- */
- if (timer->flags & TIMER_DEFERRABLE) {
- if (tick_nohz_full_cpu(base->cpu))
- wake_up_nohz_cpu(base->cpu);
- return;
+ if (is_timers_nohz_active()) {
+ /*
+ * TODO: This wants some optimizing similar to the code
+ * below, but we will do that when we switch from push to
+ * pull for deferrable timers.
+ */
+ if (timer->flags & TIMER_DEFERRABLE) {
+ if (tick_nohz_full_cpu(base->cpu))
+ wake_up_nohz_cpu(base->cpu);
+ return;
+ }
}

- /*
- * We might have to IPI the remote CPU if the base is idle and the
- * timer is not deferrable. If the other CPU is on the way to idle
- * then it can't set base->is_idle as we hold the base lock:
- */
- if (!base->is_idle)
- return;
-
/* Check whether this is the new first expiring timer: */
if (time_after_eq(timer->expires, base->next_expiry))
return;
+ /* Update next expiry time */
+ base->next_expiry = timer->expires;

/*
- * Set the next expiry time and kick the CPU so it can reevaluate the
- * wheel:
+ * We might have to IPI the remote CPU if the base is idle and the
+ * timer is not deferrable. If the other CPU is on the way to idle
+ * then it can't set base->is_idle as we hold the base lock:
*/
- base->next_expiry = timer->expires;
- wake_up_nohz_cpu(base->cpu);
+ if (is_timers_nohz_active() && base->is_idle)
+ wake_up_nohz_cpu(base->cpu);
}

static void
@@ -1684,6 +1679,7 @@ static inline void __run_timers(struct t
while (levels--)
expire_timers(base, heads + levels);
}
+ base->next_expiry = __next_timer_interrupt(base);
base->running_timer = NULL;
raw_spin_unlock_irq(&base->lock);
}
@@ -1716,8 +1712,24 @@ void run_local_timers(void)
base++;
if (time_before(jiffies, base->clk))
return;
+ base--;
+ }
+
+ /*
+ * check for next expiry
+ *
+ * deferrable base is igonred here - it is only usable when
+ * switching from push to pull model for deferrable timers
+ */
+ raw_spin_lock_irq(&base->lock);
+ if (base->clk == base->next_expiry) {
+ raw_spin_unlock_irq(&base->lock);
+ raise_softirq(TIMER_SOFTIRQ);
+ } else {
+ base->clk++;
+ raw_spin_unlock_irq(&base->lock);
+ return;
}
- raise_softirq(TIMER_SOFTIRQ);
}

/*
---8<---

Thanks,

Anna-Maria