timer: upper bound on loops of __run_timers processing

PATCH In review Marcelo Tosatti 4 participants 7 messages Dec 30, 2014 – Feb 17, 2015

Share & tools

Marcelo Tosatti

author

2014-12-30 19:53 UTC #0 lore raw

PATCH 1/1 timer: upper bound on loops of __run_timers processing

Commit "timers: do not raise softirq unconditionally", allows for timer wheel processing (__run_timers) to be delayed for long periods of time.

The effect is that

loops = jiffies - base->timer_jiffies

Can grow to very large values resulting in __run_timers taking hundreds of milliseconds to execute.

Fix by creating an upper bound on the number of loops to be processed. This allows a nohz=off kernel to achieve desired latencies.

Signed-off-by Marcelo Tosatti <[email protected]>

kernel/timer.c +6 -0

index f59e18c..c128416 100644
@@ -1488,6 +1488,12 @@ void run_local_timers(void)
 	}
 #endif
 
+	if (time_after_eq(jiffies, base->timer_jiffies)) {
+		unsigned long jiffies_delta = jiffies - base->timer_jiffies;
+		if (jiffies_delta > TVR_SIZE)
+			raise_softirq(TIMER_SOFTIRQ);
+	}
+
 	if (!base->active_timers)
 		goto out;

Rik van Riel

Acked-by on v1 1/1

2014-12-30 21:05 UTC #1 lore raw

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Quoted 23 lines · Marcelo Tosatti

Commit "timers: do not raise softirq unconditionally", allows for timer wheel processing (__run_timers) to be delayed for long periods of time.
The effect is that
loops = jiffies - base->timer_jiffies
Can grow to very large values resulting in __run_timers taking hundreds of milliseconds to execute.
Fix by creating an upper bound on the number of loops to be processed. This allows a nohz=off kernel to achieve desired latencies.
Signed-off-by Marcelo Tosatti <[email protected]>
kernel/timer.c index f59e18c..c128416 renamed from kernel/timer.c +0 -0
100644 --- a/kernel/timer.c +++ b/kernel/timer.c @@ -1488,6 +1488,12 @@ void run_local_timers(void) } #endif
kernel/timer.c index f59e18c..c128416 renamed from kernel/timer.c +1 -0
+	if (time_after_eq(jiffies, base->timer_jiffies)) { +		unsigned

Quoted 2 lines

long jiffies_delta = jiffies - base->timer_jiffies; + if
(jiffies_delta > TVR_SIZE) + raise_softirq(TIMER_SOFTIRQ); + }

It certainly looks like cascading the timers over and over and over again could be a lot of work.

Acked-by Rik van Riel <[email protected]>

- --
All rights reversed
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQEcBAEBAgAGBQJUoxOfAAoJEM553pKExN6DO+0H/i2TXS2DJ6cObGZytYhsRz/U 4oqH4eCjwK5JczFMsyoUh/kXIkEQukMudj74katoP0TARlcnZF2G+R/Hj4XccCpz X84DI7SCFZIwl3gpiuOoR81mosd1VkLBouwYj1Aglt+5Sh63yq91lreX20Tw/swF jlorAWAAblQdt2zC5smjtbbhfKMuPC8qJFGoiGmLLPC/1l9XmGmfymB2WfN8IZm4 DrqAlCSm8+xxGdwUwAZTuHuHX15VOpGuUuEV2E8BM+6G93HI8iJe5E7g5hRymcNp NPCe+BFgZ9AxHqNokC1cSWrqaS3koBbTJRDKAqxSEptmYQMZ/a61ydYJQmBtImw= =uqAv
-----END PGP SIGNATURE-----

Marcelo Tosatti

author

2015-01-14 17:21 UTC #2 lore raw

In reply to Marcelo Tosatti

Ping?

Quoted 33 lines · Marcelo Tosatti

Commit "timers: do not raise softirq unconditionally", allows for timer wheel processing (__run_timers) to be delayed for long periods of time.
The effect is that
loops = jiffies - base->timer_jiffies
Can grow to very large values resulting in __run_timers taking hundreds of milliseconds to execute.
Fix by creating an upper bound on the number of loops to be processed. This allows a nohz=off kernel to achieve desired latencies.
Signed-off-by Marcelo Tosatti <[email protected]>
kernel/timer.c +6 -0
index f59e18c..c128416 100644
@@ -1488,6 +1488,12 @@ void run_local_timers(void)
 	}
 #endif
 
+	if (time_after_eq(jiffies, base->timer_jiffies)) {
+		unsigned long jiffies_delta = jiffies - base->timer_jiffies;
+		if (jiffies_delta > TVR_SIZE)
+			raise_softirq(TIMER_SOFTIRQ);
+	}
+
 	if (!base->active_timers)
 		goto out;
 

Steven Rostedt

2015-01-14 17:41 UTC #3 lore raw

Quoted 3 lines · On Wed, 14 Jan 2015 15:21:47 -0200 Marcelo Tosatti <[email protected]> wrote:

Ping?

Sorry, I've fallen behind since the holidays and still trying to catch up. I'll try to look at this this week.

-- Steve

Quoted 34 lines · Marcelo Tosatti

Quoted 33 lines
Commit "timers: do not raise softirq unconditionally", allows for timer wheel processing (__run_timers) to be delayed for long periods of time.
The effect is that
loops = jiffies - base->timer_jiffies
Can grow to very large values resulting in __run_timers taking hundreds of milliseconds to execute.
Fix by creating an upper bound on the number of loops to be processed. This allows a nohz=off kernel to achieve desired latencies.
Signed-off-by Marcelo Tosatti <[email protected]>
kernel/timer.c +6 -0
index f59e18c..c128416 100644
@@ -1488,6 +1488,12 @@ void run_local_timers(void)
 	}
 #endif
 
+	if (time_after_eq(jiffies, base->timer_jiffies)) {
+		unsigned long jiffies_delta = jiffies - base->timer_jiffies;
+		if (jiffies_delta > TVR_SIZE)
+			raise_softirq(TIMER_SOFTIRQ);
+	}
+
 	if (!base->active_timers)
 		goto out;
 

Marcelo Tosatti

author

2015-01-26 21:12 UTC #4 lore raw

In reply to Marcelo Tosatti

Ping ?

Quoted 41 lines · Marcelo Tosatti

Ping?
Quoted 23 lines · Marcelo Tosatti
Commit "timers: do not raise softirq unconditionally", allows for timer wheel processing (__run_timers) to be delayed for long periods of time.
The effect is that
loops = jiffies - base->timer_jiffies
Can grow to very large values resulting in __run_timers taking hundreds of milliseconds to execute.
Fix by creating an upper bound on the number of loops to be processed. This allows a nohz=off kernel to achieve desired latencies.
Signed-off-by Marcelo Tosatti <[email protected]>
kernel/timer.c +0 -0
index f59e18c..c128416 100644
@@ -1488,6 +1488,12 @@ void run_local_timers(void)
 	}
 #endif
kernel/timer.c void run_local_timers(void) · line 1499
Quoted 10 lines
kernel/timer.c +6 -0
 
+	if (time_after_eq(jiffies, base->timer_jiffies)) {
+		unsigned long jiffies_delta = jiffies - base->timer_jiffies;
+		if (jiffies_delta > TVR_SIZE)
+			raise_softirq(TIMER_SOFTIRQ);
+	}
+
 	if (!base->active_timers)
 		goto out;
 
--
To unsubscribe from this list: send the line "unsubscribe linux-rt-users" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Sebastian Andrzej Siewior

2015-02-17 17:24 UTC #5 lore raw

Quoted 1 line · * Marcelo Tosatti | 2015-01-26 19:12:31 [-0200]:

Ping ?

please resent.

Sebastian

Rik van Riel

2015-02-17 23:08 UTC #6 lore raw

In reply to Marcelo Tosatti

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Quoted 7 lines · Marcelo Tosatti

Commit "timers: do not raise softirq unconditionally", allows for timer wheel processing (__run_timers) to be delayed for long periods of time.
The effect is that

Quoted 8 lines

loops = jiffies - base->timer_jiffies
Can grow to very large values resulting in __run_timers taking hundreds of milliseconds to execute.
Fix by creating an upper bound on the number of loops to be processed. This allows a nohz=off kernel to achieve desired latencies.

Turns out that when we make nohz=off actually work correctly with realtime tasks, and the scheduler tick is really disabled, run_local_timers does not get called while the scheduler tick is disabled, and this patch does nothing...

Quoted 7 lines

+++ b/kernel/timer.c @@ -1488,6 +1488,12 @@ void
run_local_timers(void) } #endif
(quoted diff) +1 -0
+	if (time_after_eq(jiffies, base->timer_jiffies)) { +		unsigned
long jiffies_delta = jiffies - base->timer_jiffies; + if
(jiffies_delta > TVR_SIZE) + raise_softirq(TIMER_SOFTIRQ); + } +
if (!base->active_timers) goto out;

I suspect we may need an alternate approach, where we change the way __run_timers works, so it does not go around its main loop thousands of times.

Is there any sane way we could going around this list thousands of times?

This is especially annoying when there are no timers pending in the first place :)

static inline void __run_timers(struct tvec_base *base)

{
        struct timer_list *timer;

        spin_lock_irq(&base->lock);
        while (time_after_eq(jiffies, base->timer_jiffies)) {
                struct list_head work_list;
                struct list_head *head = &work_list;
                int index = base->timer_jiffies & TVR_MASK;

                /*
                 * Cascade timers:
                 */
                if (!index &&
                        (!cascade(base, &base->tv2, INDEX(0))) &&
                                (!cascade(base, &base->tv3, INDEX(1))) &&
                                        !cascade(base, &base->tv4,
INDEX(2)))
                        cascade(base, &base->tv5, INDEX(3));
                ++base->timer_jiffies;
                list_replace_init(base->tv1.vec + index, &work_list);
                while (!list_empty(head)) {

- --
All rights reversed
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQEcBAEBAgAGBQJU48nfAAoJEM553pKExN6DKpoH/2Cf/nmra2uhsJzSm+FgbWgi 8EMM7TszF24Ys+JwtbdVmTonBsAXOmjJEGxvc9yknUymO3Wj6Iph4Z1cPk/9nS/v vU7RWBiu9P6lmHfRuK+dN4w7SQnrtkOOuOLWlNSfwklVxCZasIPOsDazt/uivnpk H3AMhGk0LduzAg1GSfJnCawnrIx/BgCu1UzHOMdE21SNZIS8z8o/MPQJ1qMbneQe HbumMiB/0PmSmsMK9SVmYp7oEo0KliMJ+MW09Yrjfg0umz/h3TQ1aY3l5JhYK2rX mmhlOIR4Sg720vZ6DzbDoRaJQHVY9Mc/QammpCU5ZY/f7diLzRsmm1FteNFQeAY= =q2AN
-----END PGP SIGNATURE-----