Thread (34 messages) 34 messages, 4 authors, 2018-03-06

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);
  }
  
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help