Re: [PATCH v3 2/2] timers: Don't search for expired timers while TIMER_SOFTIRQ is scheduled
From: Haris Okanovic <hidden>
Date: 2018-01-05 19:38:02
Also in:
lkml
It looks like an old version of this patch is included in v4.9*-rt* kernels -- E.g. commit 032f93ca in v4.9.68-rt60. There's nothing functionally wrong with the included version to the best of my knowledge. However, I posted a newer V3 [1][2] based on Thomas' feedback that's substantially cleaner and likely more efficient (haven't measured yet). I think we should include the latter version instead, if only for the cosmetic benefits. Thoughts? [1] https://patchwork.kernel.org/patch/9879825/ [PATCH v3,1/2] [2] https://patchwork.kernel.org/patch/9879827/ [PATCH v3,2/2] -- Haris On 08/03/2017 04:06 PM, Haris Okanovic wrote:
quoted hunk ↗ jump to hunk
This change avoid needlessly searching for more timers in run_local_timers() (hard interrupt context) when they can't fire. For example, when ktimersoftd/run_timer_softirq() is scheduled but preempted due to cpu contention. When it runs, run_timer_softirq() will discover newly expired timers up to current jiffies in addition to firing previously expired timers. However, this change also adds an edge case where non-hrtimer firing is sometimes delayed by an additional tick. This is acceptable since we don't make latency guarantees for non-hrtimers and would prefer to minimize hard interrupt time instead. Signed-off-by: Haris Okanovic <redacted> --- [PATCH v3] - Split block_softirq into separate commit https://github.com/harisokanovic/linux/tree/dev/hokanovi/timer-peek-v5 --- kernel/time/timer.c | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-)diff --git a/kernel/time/timer.c b/kernel/time/timer.c index 078027d8a866..f0ef9675abdf 100644 --- a/kernel/time/timer.c +++ b/kernel/time/timer.c@@ -208,6 +208,7 @@ struct timer_base { bool migration_enabled; bool nohz_active; bool is_idle; + bool block_softirq; DECLARE_BITMAP(pending_map, WHEEL_SIZE); struct hlist_head vectors[WHEEL_SIZE]; struct hlist_head expired_lists[LVL_DEPTH];@@ -1376,9 +1377,11 @@ static int __collect_expired_timers(struct timer_base *base) /* * expire_timers() must be called at least once before we can - * collect more timers. + * collect more timers. We should never hit this case unless + * TIMER_SOFTIRQ got raised without expired timers. */ - if (base->expired_levels) + if (WARN_ONCE(base->expired_levels, + "Must expire collected timers before collecting more")) return base->expired_levels; clk = base->clk;@@ -1702,6 +1705,9 @@ static __latent_entropy void run_timer_softirq(struct softirq_action *h) __run_timers(base); if (IS_ENABLED(CONFIG_NO_HZ_COMMON) && base->nohz_active) __run_timers(this_cpu_ptr(&timer_bases[BASE_DEF])); + + /* Allow new TIMER_SOFTIRQs to get scheduled by run_local_timers() */ + base->block_softirq = false; } /*@@ -1712,6 +1718,14 @@ void run_local_timers(void) struct timer_base *base = this_cpu_ptr(&timer_bases[BASE_STD]); hrtimer_run_queues(); + + /* + * Skip if TIMER_SOFTIRQ is already running on this CPU, since it + * will find and expire all timers up to current jiffies. + */ + if (base->block_softirq) + return; + /* Raise the softirq only if required. */ if (time_before(jiffies, base->clk) || !tick_find_expired(base)) { if (!IS_ENABLED(CONFIG_NO_HZ_COMMON) || !base->nohz_active)@@ -1720,7 +1734,10 @@ void run_local_timers(void) base++; if (time_before(jiffies, base->clk) || !tick_find_expired(base)) return; + base--; } + + base->block_softirq = true; raise_softirq(TIMER_SOFTIRQ); }