Crude timer-wheel latency hacks
cover Crude timer-wheel latency hacks
Hello!
The following three patches provide some crude timer-wheel latency patches. I understand that a more comprehensive solution is in progress, but in the meantime, these patches work well in cases where a given CPU has either zero or one timers pending, which is a common case for NO_HZ_FULL kernels. So, on the off-chance that this is helpful to someone, the individual patches are as follows:
1. Avoid jiffy-at-a-time stepping when the timer wheel is empty.
2. Avoid jiffy-at-a-time stepping when the timer wheel transitions
to empty.
3. Avoid jiffy-at-a-time stepping after a timer is added to an
initially empty timer wheel.
Thanx, Paul
------------------------------------------------------------------------
b/kernel/timer.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)
PATCH 1/3 timers: Reduce __run_timers() latency for empty list
From: "Paul E. McKenney" <[email protected]>
The __run_timers() function currently steps through the list one jiffy at a time in order to update the timer wheel. However, if the timer wheel is empty, no adjustment is needed other than updating ->timer_jiffies. In this case, which is likely to be common for NO_HZ_FULL kernels, the kernel currently incurs a large latency for no good reason. This commit therefore short-circuits this case.
--- kernel/timer.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)
kernel/timer.c +15 -0
@@ -337,6 +337,17 @@ void set_timer_slack(struct timer_list *timer, int slack_hz) }EXPORT_SYMBOL_GPL (set_timer_slack); +static bool catchup_timer_jiffies (struct tvec_base *base) +{ +#ifdef CONFIG_NO_HZ_FULL +if (!base->active_timers) { + base->timer_jiffies = jiffies; +return 1 ; + } +#endif /* #ifdef CONFIG_NO_HZ_FULL */ +return 0 ; +} +static void __internal_add_timer (struct tvec_base *base,struct timer_list *timer) {
@@ -1146,6 +1157,10 @@ static inline void __run_timers(struct tvec_base *base)struct timer_list *timer;spin_lock_irq (&base->lock); +if (catchup_timer_jiffies (base)) { +spin_unlock_irq (&base->lock); +return ; + }while (time_after_eq (jiffies, base->timer_jiffies)) {struct list_head work_list;struct list_head *head = &work_list;
Signature · 1 line
1.8.1.5
PATCH 2/3 timers: Reduce future __run_timers() latency for newly emptied list
From: "Paul E. McKenney" <[email protected]>
The __run_timers() function currently steps through the list one jiffy at a time in order to update the timer wheel. However, if the timer wheel is empty, no adjustment is needed other than updating ->timer_jiffies. Therefore, if we just emptied the timer wheel, for example, by deleting the last timer, we should mark the timer wheel as being up to date. This marking will reduce (and perhaps eliminate) the jiffy-stepping that a future __run_timers() call will need to do in response to some future timer posting or migration. This commit therefore catches ->timer_jiffies for this case.
--- kernel/timer.c | 1 + 1 file changed, 1 insertion(+)
kernel/timer.c +1 -0
@@ -693,6 +693,7 @@ static int detach_if_pending(struct timer_list *timer, struct tvec_base *base,detach_timer (timer, clear_pending);if (!tbase_get_deferrable (timer->base)) { base->active_timers--; + (void )catchup_timer_jiffies (base);if (timer->expires == base->next_timer) base->next_timer = base->timer_jiffies; }
Signature · 1 line
1.8.1.5
PATCH 3/3 timers: Reduce future __run_timers() latency for first add to empty list
In reply to Paul E. McKenneyFrom: "Paul E. McKenney" <[email protected]>
The __run_timers() function currently steps through the list one jiffy at a time in order to update the timer wheel. However, if the timer wheel is empty, no adjustment is needed other than updating ->timer_jiffies. Therefore, just before we add a timer to an empty timer wheel, we should mark the timer wheel as being up to date. This marking will reduce (and perhaps eliminate) the jiffy-stepping that a future __run_timers() call will need to do in response to some future timer posting or migration. This commit therefore updates ->timer_jiffies for this case.
--- kernel/timer.c | 1 + 1 file changed, 1 insertion(+)
kernel/timer.c +1 -0
@@ -745,6 +745,7 @@ __mod_timer(struct timer_list *timer, unsigned long expires, base =lock_timer_base (timer, &flags); + (void )catchup_timer_jiffies (base); ret =detach_if_pending (timer, base, false);if (!ret && pending_only) goto out_unlock;
Signature · 1 line
1.8.1.5
Re: [PATCH tip/core/timers 1/3] timers: Reduce __run_timers() latency for empty list
In reply to Paul E. McKenneyQuoted 4 lines · Paul E. McKenney
The
__run_timers()function currently steps through the list one jiffy at a time in order to update the timer wheel. However, if the timer wheel is empty, no adjustment is needed other than updating ->timer_jiffies.
Yes, but ->active_timers == 0 doesn't necessarily mean "empty", it only counts the non-deferrable timers?
Quoted 32 lines
In this case, which is likely to be common for
NO_HZ_FULLkernels, the kernel currently incurs a large latency for no good reason. This commit therefore short-circuits this case.Signed-off-by Paul E. McKenney <[email protected]>--- kernel/timer.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)kernel/timer.c +11
-0@@ -337,6 +337,17 @@ void set_timer_slack(struct timer_list *timer, int slack_hz) }EXPORT_SYMBOL_GPL (set_timer_slack); +static bool catchup_timer_jiffies (struct tvec_base *base) +{ +#ifdef CONFIG_NO_HZ_FULL +if (!base->active_timers) { + base->timer_jiffies = jiffies; +return 1 ; + } +#endif /* #ifdef CONFIG_NO_HZ_FULL */ +return 0 ; +} +static void __internal_add_timer (struct tvec_base *base,struct timer_list *timer) {
kernel/timer.c static inline void __run_timers(struct tvec_base *base) · line 1159
Quoted 3 lines
kernel/timer.c +0
-0@@ -1146,6 +1157,10 @@ static inline void __run_timers(struct tvec_base *base)struct timer_list *timer;spin_lock_irq (&base->lock);
Do we really need to take base->lock before catchup_timer_jiffies() ? ->timer_jiffies can only be changed by us, and it seems that we do not care if we race with base->active_timers++.
kernel/timer.c static inline void __run_timers(struct tvec_base *base) · line 1162
Quoted 3 lines
kernel/timer.c +3
-0+if (catchup_timer_jiffies (base)) { +spin_unlock_irq (&base->lock); +return ;
This is what I can't understand... Doesn't this mean that, unless this base have a non-deferrable timer, we can never run the pending deferrable timers even if the system/cpu is "busy" ?
Oleg.
Re: [PATCH tip/core/timers 1/3] timers: Reduce __run_timers() latency for empty list
Quoted 8 lines · Oleg Nesterov
Quoted 4 lines · Paul E. McKenney
The
__run_timers()function currently steps through the list one jiffy at a time in order to update the timer wheel. However, if the timer wheel is empty, no adjustment is needed other than updating ->timer_jiffies.Yes, but ->active_timers == 0 doesn't necessarily mean "empty", it only counts the non-deferrable timers?
Right you are! Color me slow and stupid...
Separate counter clearly required.
Quoted 34 lines
Quoted 34 lines
In this case, which is likely to be common for
NO_HZ_FULLkernels, the kernel currently incurs a large latency for no good reason. This commit therefore short-circuits this case.Signed-off-by Paul E. McKenney <[email protected]>--- kernel/timer.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)kernel/timer.c +11
-0@@ -337,6 +337,17 @@ void set_timer_slack(struct timer_list *timer, int slack_hz) }EXPORT_SYMBOL_GPL (set_timer_slack); +static bool catchup_timer_jiffies (struct tvec_base *base) +{ +#ifdef CONFIG_NO_HZ_FULL +if (!base->active_timers) { + base->timer_jiffies = jiffies; +return 1 ; + } +#endif /* #ifdef CONFIG_NO_HZ_FULL */ +return 0 ; +} +static void __internal_add_timer (struct tvec_base *base,struct timer_list *timer) {@@ -1146,6 +1157,10 @@ static inline void __run_timers(struct tvec_base *base)struct timer_list *timer;
Quoted 5 lines
kernel/timer.c static inline void __run_timers(struct tvec_base *base) · line 1159
Quoted 1 line
kernel/timer.c +0
-0spin_lock_irq (&base->lock);Do we really need to take
base->lockbeforecatchup_timer_jiffies()? ->timer_jiffies can only be changed by us, and it seems that we do not care if we race with base->active_timers++.
Given that this lock should be almost always acquired by the current CPU, the penalty for acquiring it should be low. After all, we were acquiring it prior to this patch as many times as we are after this patch, right?
Quoted 7 lines
kernel/timer.c static inline void __run_timers(struct tvec_base *base) · line 1162
Quoted 3 lines
kernel/timer.c +3
-0+if (catchup_timer_jiffies (base)) { +spin_unlock_irq (&base->lock); +return ;This is what I can't understand... Doesn't this mean that, unless this base have a non-deferrable timer, we can never run the pending deferrable timers even if the system/cpu is "busy" ?
It does, and that is a bug in my patch. Good catch!
Thanx, Paul
Re: [PATCH tip/core/timers 1/3] timers: Reduce __run_timers() latency for empty list
Quoted 13 lines · Paul E. McKenney
Quoted 10 lines · Oleg Nesterov
kernel/timer.c static inline void __run_timers(struct tvec_base *base) · line 1159
Quoted 6 lines
__internal_add_timer(struct tvec_base *base, struct timer_list *timer){kernel/timer.c +0
-0@@ -1146,6 +1157,10 @@ static inline void __run_timers(struct tvec_base *base)struct timer_list *timer;spin_lock_irq (&base->lock);Do we really need to take
base->lockbeforecatchup_timer_jiffies()? ->timer_jiffies can only be changed by us, and it seems that we do not care if we race with base->active_timers++.
Quoted 4 lines
Given that this lock should be almost always acquired by the current CPU, the penalty for acquiring it should be low. After all, we were acquiring it prior to this patch as many times as we are after this patch, right?
Yes. But
if (catchup_timer_jiffies()) return;
looks a bit simpler and can save a couple of insn. I won't argue of course, this is minor. And you already sent v2, I'll try add some comments...
Oleg.