timer: Refactor the timer wheel
Subsystems (20)
cover timer: Refactor the timer wheel
The current timer wheel has some drawbacks:
1) Cascading
Cascading can be an unbound operation and is completely pointless in most cases because the vast majority of the timer wheel timers are canceled or rearmed before expiration.
2) No fast lookup of the next expiring timer
In NOHZ scenarios the first timer soft interrupt after a long NOHZ period must fast forward the base time to current jiffies. As we have no way to find the next expiring timer fast, the code loops and increments the base time by one and checks for expired timers in each step. I've observed loops lasting 1 ms!
There are some other issues caused by the above, but they are minor compare to those.
After a thorough analysis of real world data gathered on laptops, workstations, webservers and other machines (thanks Chris!) I came to the conclusion that the current 'classic' timer wheel implementation can be modified to address the above issues.
The vast majority of timer wheel timers is canceled or rearmed before expiry. Most of them are timeouts for networking and other I/O tasks. The nature of timeouts is to catch the exception from normal operation (TCP ack timed out, disk does not respond, etc.). For these kind of timeouts the accuracy is not really a concern. In case the timeout fires, performance is down the drain already.
The few timers which actually expire can be split into two categories:
1) Short expiry times which expect halfways accurate expiry
2) Long term expiry times are inaccurate today already due to the batching which is done for NOHZ.
So for long term expiry timers we can avoid the cascading property and just leave them in the less granular outer wheels until expiry or cancelation.
Contrary to the classic wheel the granularities of the next wheel is not the capacity of the first wheel. The granularities of the wheels are in the currently chosen setting 8 times the granularity of the previous wheel. So for HZ=250 we end up with the following granularity levels:
Level Offset Granularity Range
0 0 4 ms 0 ms - 252 ms
1 64 32 ms 256 ms - 2044 ms (256ms - ~2s)
2 128 256 ms 2048 ms - 16380 ms (~2s - ~16s)
3 192 2048 ms (~2s) 16384 ms - 131068 ms (~16s - ~2m)
4 256 16384 ms (~16s) 131072 ms - 1048572 ms (~2m - ~17m)
5 320 131072 ms (~2m) 1048576 ms - 8388604 ms (~17m - ~2h)
That's a worst case inaccuracy of 12.5% for the timers which are queued at the beginning of a level.
So the new wheel concept addresses the old issues:
1) Cascading is avoided (except for extreme long time timers)
2) By keeping the timers in the bucket until expiry/cancelation we can track the buckets which have timers enqueued in a bucket bitmap and therefor can lookup the next expiring timer fast and time bound.
A further benefit of the concept is, that the slack calculation which is done on every timer start is not longer necessary because the granularity levels provide natural batching already.
Our extensive testing with various loads did not show any performance degradation vs. the current wheel implementation.
This series has also preparatory patches for changing the NOHZ timer handling from the current push to a pull model. Currently we decide at timer enqueue time on which cpu we queue the timer. This is exceptionally silly because there is no way to predict at enqueue time which cpu will be idle when the timer expires. Given the fact that most timers are canceled or rearmed before expiry this is even more silly. We trade a expensive decision and cross cpu access for a very doubtful benefit.
The solution to this is to store the migrateable timers in a seperate storage space on the local cpu and when the cpu goes idle tell the others about its next expiring timer and let the other cpu pull the timer in case of expiry. We have a proof of concept implementation for this, but it's not yet ready for posting.
Thanks,
tglx
--- arch/x86/kernel/apic/x2apic_uv_x.c | 4 arch/x86/kernel/cpu/mcheck/mce.c | 4 block/genhd.c | 5 drivers/cpufreq/powernv-cpufreq.c | 5 drivers/mmc/host/jz4740_mmc.c | 2 drivers/net/ethernet/tile/tilepro.c | 4 drivers/power/bq27xxx_battery.c | 5 drivers/tty/metag_da.c | 4 drivers/tty/mips_ejtag_fdc.c | 4 drivers/usb/host/ohci-hcd.c | 1 drivers/usb/host/xhci.c | 2 include/linux/list.h | 10 include/linux/timer.h | 30 include/trace/events/timer.h | 11 kernel/time/tick-internal.h | 1 kernel/time/tick-sched.c | 46 - kernel/time/timer.c | 1090 +++++++++++++++++++++--------------- lib/random32.c | 1 net/ipv4/inet_connection_sock.c | 7 net/ipv4/inet_timewait_sock.c | 5 20 files changed, 731 insertions(+), 510 deletions(-)
PATCH 1/20 timer: Make pinned a timer property
We want to move the timer migration from a push to a pull model. This requires to store the pinned attribute of a timer in the timer itself. This must happen at initialization time.
Add the helper macros for this.
--- include/linux/timer.h | 25 ++++++++++++++++++++++--- kernel/time/timer.c | 10 +++++----- 2 files changed, 27 insertions(+), 8 deletions(-)
include/linux/timer.h +22 -3
@@ -62,7 +62,8 @@ struct timer_list {#define TIMER_MIGRATING 0x00080000#define TIMER_BASEMASK (TIMER_CPUMASK | TIMER_MIGRATING)#define TIMER_DEFERRABLE 0x00100000 -#define TIMER_IRQSAFE 0x00200000 +#define TIMER_PINNED 0x00200000 +#define TIMER_IRQSAFE 0x00400000#define __TIMER_INITIALIZER (_function, _expires, _data, _flags) { \ .entry = { .next = TIMER_ENTRY_STATIC }, \
@@ -78,9 +79,15 @@ struct timer_list {#define TIMER_INITIALIZER (_function, _expires, _data) \ __TIMER_INITIALIZER((_function), (_expires), (_data), 0) +#define TIMER_PINNED_INITIALIZER (_function, _expires, _data) \ + __TIMER_INITIALIZER((_function), (_expires), (_data), TIMER_PINNED) +#define TIMER_DEFERRED_INITIALIZER (_function, _expires, _data) \ __TIMER_INITIALIZER((_function), (_expires), (_data), TIMER_DEFERRABLE) +#define TIMER_PINNED_DEFERRED_INITIALIZER (_function, _expires, _data) \ + __TIMER_INITIALIZER((_function), (_expires), (_data), TIMER_DEFERRABLE | TIMER_PINNED) +#define DEFINE_TIMER (_name, _function, _expires, _data) \ struct timer_list _name = \ TIMER_INITIALIZER(_function, _expires, _data)
@@ -124,8 +131,12 @@ static inline void init_timer_on_stack_k#define init_timer (timer) \ __init_timer((timer), 0) +#define init_timer_pinned (timer) \ + __init_timer((timer), TIMER_PINNED)#define init_timer_deferrable (timer) \ __init_timer((timer), TIMER_DEFERRABLE) +#define init_timer_pinned_deferrable (timer) \ + __init_timer((timer), TIMER_DEFERRABLE | TIMER_PINNED)#define init_timer_on_stack (timer) \ __init_timer_on_stack((timer), 0)
@@ -145,12 +156,20 @@ static inline void init_timer_on_stack_k#define setup_timer (timer, fn, data) \ __setup_timer((timer), (fn), (data), 0) +#define setup_pinned_timer (timer, fn, data) \ + __setup_timer((timer), (fn), (data), TIMER_PINNED)#define setup_deferrable_timer (timer, fn, data) \ __setup_timer((timer), (fn), (data), TIMER_DEFERRABLE) +#define setup_pinned_deferrable_timer (timer, fn, data) \ + __setup_timer((timer), (fn), (data), TIMER_DEFERRABLE | TIMER_PINNED)#define setup_timer_on_stack (timer, fn, data) \ __setup_timer_on_stack((timer), (fn), (data), 0) +#define setup_pinned_timer_on_stack (timer, fn, data) \ + __setup_timer_on_stack((timer), (fn), (data), TIMER_PINNED)#define setup_deferrable_timer_on_stack (timer, fn, data) \ __setup_timer_on_stack((timer), (fn), (data), TIMER_DEFERRABLE) +#define setup_pinned_deferrable_timer_on_stack (timer, fn, data) \ + __setup_timer_on_stack((timer), (fn), (data), TIMER_DEFERRABLE | TIMER_PINNED)/** * timer_pending - is a timer pending?
@@ -175,8 +194,8 @@ extern int mod_timer_pinned(struct timerextern void set_timer_slack (struct timer_list *time,int slack_hz); -#define TIMER_NOT_PINNED 0 -#define TIMER_PINNED 1 +#define MOD_TIMER_NOT_PINNED 0 +#define MOD_TIMER_PINNED 1/* * The jiffies value which is added to now, when there is no timer * in the timer wheel:
kernel/time/timer.c +5 -5
@@ -782,7 +782,7 @@ static inline intdebug_activate (timer, expires); - new_base =get_target_base (base, pinned); + new_base =get_target_base (base, pinned || timer->flags &TIMER_PINNED );if (base != new_base) {/*
@@ -825,7 +825,7 @@ static inline int*/ int mod_timer_pending (struct timer_list *timer,unsigned long expires) { -return __mod_timer (timer, expires, true,TIMER_NOT_PINNED ); +return __mod_timer (timer, expires, true,MOD_TIMER_NOT_PINNED ); }EXPORT_SYMBOL (mod_timer_pending);
@@ -900,7 +900,7 @@ int mod_timer(struct timer_list *timer,if (timer_pending (timer) && timer->expires == expires)return 1 ; -return __mod_timer (timer, expires, false,TIMER_NOT_PINNED ); +return __mod_timer (timer, expires, false,MOD_TIMER_NOT_PINNED ); }EXPORT_SYMBOL (mod_timer);
@@ -928,7 +928,7 @@ int mod_timer_pinned(struct timer_list *if (timer->expires == expires &&timer_pending (timer))return 1 ; -return __mod_timer (timer, expires, false,TIMER_PINNED ); +return __mod_timer (timer, expires, false,MOD_TIMER_PINNED ); }EXPORT_SYMBOL (mod_timer_pinned);
@@ -1512,7 +1512,7 @@ signed long __sched schedule_timeout(sig expire = timeout + jiffies;setup_timer_on_stack (&timer, process_timeout, (unsigned long )current); -__mod_timer (&timer, expire, false,TIMER_NOT_PINNED ); +__mod_timer (&timer, expire, false,MOD_TIMER_NOT_PINNED );schedule ();del_singleshot_timer_sync (&timer);
PATCH 2/20 x86/apic/uv: Initialize timer as pinned
In reply to Thomas GleixnerPinned timers must carry that attribute in the timer itself. No functional change.
--- arch/x86/kernel/apic/x2apic_uv_x.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
arch/x86/kernel/apic/x2apic_uv_x.c +2 -2
@@ -919,7 +919,7 @@ static void uv_heartbeat(unsigned long iuv_set_scir_bits (bits);/* enable next timer period */ -mod_timer_pinned (timer, jiffies +SCIR_CPU_HB_INTERVAL ); +mod_timer (timer, jiffies +SCIR_CPU_HB_INTERVAL ); }static void uv_heartbeat_enable (int cpu)
@@ -928,7 +928,7 @@ static void uv_heartbeat_enable(int cpu)struct timer_list *timer = &uv_cpu_scir_info (cpu)->timer;uv_set_cpu_scir_bits (cpu,SCIR_CPU_HEARTBEAT |SCIR_CPU_ACTIVITY ); -setup_timer (timer, uv_heartbeat, cpu); +setup_pinned_timer (timer, uv_heartbeat, cpu); timer->expires = jiffies +SCIR_CPU_HB_INTERVAL ;add_timer_on (timer, cpu);uv_cpu_scir_info (cpu)->enabled =1 ;
PATCH 3/20 x86/mce: Initialize timer as pinned
In reply to Thomas GleixnerPinned timers must carry that attribute in the timer itself. No functional change.
--- arch/x86/kernel/cpu/mcheck/mce.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
arch/x86/kernel/cpu/mcheck/mce.c +2 -2
@@ -1309,7 +1309,7 @@ static void __restart_timer(struct timerif (timer_pending (t)) {if (time_before (when, t->expires)) -mod_timer_pinned (t, when); +mod_timer (t, when); }else { t->expires =round_jiffies (when);add_timer_on (t,smp_processor_id ());
@@ -1735,7 +1735,7 @@ static void __mcheck_cpu_init_timer(voidstruct timer_list *t =this_cpu_ptr (&mce_timer);unsigned int cpu =smp_processor_id (); -setup_timer (t, mce_timer_fn, cpu); +setup_pinned_timer (t, mce_timer_fn, cpu);mce_start_timer (cpu, t); }
PATCH 4/20 cpufreq/powernv: Initialize timer as pinned
In reply to Thomas GleixnerPinned timers must carry that attribute in the timer itself. No functional change.
--- drivers/cpufreq/powernv-cpufreq.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
drivers/cpufreq/powernv-cpufreq.c +2 -3
@@ -530,8 +530,7 @@ static inline void queue_gpstate_timer(else timer_interval =GPSTATE_TIMER_INTERVAL ; -mod_timer_pinned (&gpstates->timer, jiffies + -msecs_to_jiffies (timer_interval)); +mod_timer (&gpstates->timer, jiffies +msecs_to_jiffies (timer_interval)); }/**
@@ -699,7 +698,7 @@ static int powernv_cpufreq_cpu_init(stru policy->driver_data = gpstates;/* initialize timer */ -init_timer_deferrable (&gpstates->timer); +init_timer_pinned_deferrable (&gpstates->timer); gpstates->timer.data = (unsigned long )policy; gpstates->timer.function = gpstate_timer_handler; gpstates->timer.expires = jiffies +
Re: [patch 04/20] cpufreq/powernv: Initialize timer as pinned
Quoted 1 line · Thomas Gleixner
mod_timer(&gpstates->timer, jiffies + msecs_to_jiffies(timer_interval));
are you sure this is right? the others did not get replaced by mod_timer().. (and this is more evidence that a relative API in msecs is what drivers really want)
PATCH 5/20 driver/net/ethernet/tile: Initialize timer as pinned
In reply to Thomas GleixnerPinned timers must carry that attribute in the timer itself. No functional change.
--- drivers/net/ethernet/tile/tilepro.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/net/ethernet/tile/tilepro.c +2 -2
@@ -588,7 +588,7 @@ static bool tile_net_lepp_free_comps(strstatic void tile_net_schedule_egress_timer (struct tile_net_cpu *info) {if (!info->egress_timer_scheduled) { -mod_timer_pinned (&info->egress_timer, jiffies +1 ); +mod_timer (&info->egress_timer, jiffies +1 ); info->egress_timer_scheduled = true; } }
@@ -1004,7 +1004,7 @@ static void tile_net_register(void *dev_BUG ();/* Initialize the egress timer. */ -init_timer (&info->egress_timer); +init_pinned_timer (&info->egress_timer); info->egress_timer.data = (long )info; info->egress_timer.function = tile_net_handle_egress_timer;
PATCH 6/20 drivers/tty/metag_da: Initialize timer as pinned
In reply to Thomas GleixnerPinned timers must carry that attribute in the timer itself. No functional change.
--- drivers/tty/metag_da.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/tty/metag_da.c +2 -2
@@ -323,12 +323,12 @@ static void dashtty_timer(unsigned longif (channel >=0 )fetch_data (channel); -mod_timer_pinned (&poll_timer, jiffies +DA_TTY_POLL ); +mod_pinned (&poll_timer, jiffies +DA_TTY_POLL ); }static void add_poll_timer (struct timer_list *poll_timer) { -setup_timer (poll_timer, dashtty_timer,0 ); +setup_pinned_timer (poll_timer, dashtty_timer,0 ); poll_timer->expires = jiffies +DA_TTY_POLL ;/*
Re: [patch 06/20] drivers/tty/metag_da: Initialize timer as pinned
I know it's not related to this patch, but it'd be nice to, as you're changing the api name anyway, make a mod_pinned_relative() so that more direct users of jiffies can go away...
or even better, mod_pinned_relative_ms() so that these drivers also do not need to care about HZ.
Quoted 29 lines · Thomas Gleixner
Pinned timers must carry that attribute in the timer itself. No functional change.
Signed-off-by Thomas Gleixner <[email protected]>--- drivers/tty/metag_da.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)Index:
b/drivers/tty/metag_da.c
===================================================================drivers/tty/metag_da.c +2
-2@@ -323,12 +323,12 @@ static void dashtty_timer(unsigned longif (channel >=0 )fetch_data (channel); -mod_timer_pinned (&poll_timer, jiffies +DA_TTY_POLL ); +mod_pinned (&poll_timer, jiffies +DA_TTY_POLL ); }static void add_poll_timer (struct timer_list *poll_timer) { -setup_timer (poll_timer, dashtty_timer,0 ); +setup_pinned_timer (poll_timer, dashtty_timer,0 ); poll_timer->expires = jiffies +DA_TTY_POLL ;/*
PATCH 7/20 drivers/tty/mips_ejtag: Initialize timer as pinned
In reply to Thomas GleixnerPinned timers must carry that attribute in the timer itself. No functional change.
--- drivers/tty/mips_ejtag_fdc.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/tty/mips_ejtag_fdc.c +2 -2
@@ -689,7 +689,7 @@ static void mips_ejtag_fdc_tty_timer(unsmips_ejtag_fdc_handle (priv);if (!priv->removing) -mod_timer_pinned (&priv->poll_timer, jiffies +FDC_TTY_POLL ); +mod_timer (&priv->poll_timer, jiffies +FDC_TTY_POLL ); }/* TTY Port operations */
@@ -1002,7 +1002,7 @@ static int mips_ejtag_fdc_tty_probe(struraw_spin_unlock_irq (&priv->lock); }else {/* If we didn't get an usable IRQ, poll instead */ -setup_timer (&priv->poll_timer, mips_ejtag_fdc_tty_timer, +setup_pinned_timer (&priv->poll_timer, mips_ejtag_fdc_tty_timer, (unsigned long )priv); priv->poll_timer.expires = jiffies +FDC_TTY_POLL ;/*
PATCH 8/20 net/ipv4/inet: Initialize timers as pinned
In reply to Thomas GleixnerPinned timers must carry that attribute in the timer itself. No functional change.
--- net/ipv4/inet_connection_sock.c | 7 ++++--- net/ipv4/inet_timewait_sock.c | 5 +++-- 2 files changed, 7 insertions(+), 5 deletions(-)
net/ipv4/inet_connection_sock.c +4 -3
@@ -603,7 +603,7 @@ static void reqsk_timer_handler(unsignedif (req->num_timeout++ ==0 )atomic_dec (&queue->young); timeo =min (TCP_TIMEOUT_INIT << req->num_timeout,TCP_RTO_MAX ); -mod_timer_pinned (&req->rsk_timer, jiffies + timeo); +mod_timer (&req->rsk_timer, jiffies + timeo);return ; } drop:
@@ -617,8 +617,9 @@ static void reqsk_queue_hash_req(struct req->num_timeout =0 ; req->sk =NULL ; -setup_timer (&req->rsk_timer, reqsk_timer_handler, (unsigned long )req); -mod_timer_pinned (&req->rsk_timer, jiffies + timeout); +setup_pinned_timer (&req->rsk_timer, reqsk_timer_handler, + (unsigned long )req); +mod_timer (&req->rsk_timer, jiffies + timeout);inet_ehash_insert (req_to_sk (req),NULL );/* before letting lookups find us, make sure all req fields
net/ipv4/inet_timewait_sock.c +3 -2
@@ -188,7 +188,8 @@ struct inet_timewait_sock *inet_twsk_all tw->tw_prot = sk->sk_prot_creator;atomic64_set (&tw->tw_cookie,atomic64_read (&sk->sk_cookie));twsk_net_set (tw,sock_net (sk)); -setup_timer (&tw->tw_timer, tw_timer_handler, (unsigned long )tw); +setup_pinned_timer (&tw->tw_timer, tw_timer_handler, + (unsigned long )tw);/* * Because we use RCU lookups, we should not set tw_refcnt * to a non null value before everything is setup for this
@@ -248,7 +249,7 @@ void __inet_twsk_schedule(struct inet_ti tw->tw_kill = timeo <=4 *HZ ;if (!rearm) { -BUG_ON (mod_timer_pinned (&tw->tw_timer, jiffies + timeo)); +BUG_ON (mod_timer (&tw->tw_timer, jiffies + timeo));atomic_inc (&tw->tw_dr->tw_count); }else {mod_timer_pending (&tw->tw_timer, jiffies + timeo);
PATCH 9/20 timer: Remove mod_timer_pinned
In reply to Thomas GleixnerWe switched all users to initialize the timers as pinned and call mod_timer(). Remove the now unused function.
--- include/linux/timer.h | 3 --- kernel/time/timer.c | 39 +++++---------------------------------- 2 files changed, 5 insertions(+), 37 deletions(-)
include/linux/timer.h +0 -3
@@ -190,12 +190,9 @@ extern void add_timer_on(struct timer_liextern int del_timer (struct timer_list * timer);extern int mod_timer (struct timer_list *timer,unsigned long expires);extern int mod_timer_pending (struct timer_list *timer,unsigned long expires); -extern int mod_timer_pinned (struct timer_list *timer,unsigned long expires);extern void set_timer_slack (struct timer_list *time,int slack_hz); -#define MOD_TIMER_NOT_PINNED 0 -#define MOD_TIMER_PINNED 1/* * The jiffies value which is added to now, when there is no timer * in the timer wheel:
kernel/time/timer.c +5 -34
@@ -764,8 +764,7 @@ static struct tvec_base *lock_timer_base }static inline int -__mod_timer (struct timer_list *timer,unsigned long expires, -bool pending_only,int pinned) +__mod_timer (struct timer_list *timer,unsigned long expires,bool pending_only) {struct tvec_base *base, *new_base;unsigned long flags;
@@ -782,7 +781,7 @@ static inline intdebug_activate (timer, expires); - new_base =get_target_base (base, pinned || timer->flags &TIMER_PINNED ); + new_base =get_target_base (base, timer->flags &TIMER_PINNED );if (base != new_base) {/*
@@ -825,7 +824,7 @@ static inline int*/ int mod_timer_pending (struct timer_list *timer,unsigned long expires) { -return __mod_timer (timer, expires, true,MOD_TIMER_NOT_PINNED ); +return __mod_timer (timer, expires, true); }EXPORT_SYMBOL (mod_timer_pending);
@@ -900,39 +899,11 @@ int mod_timer(struct timer_list *timer,if (timer_pending (timer) && timer->expires == expires)return 1 ; -return __mod_timer (timer, expires, false,MOD_TIMER_NOT_PINNED ); +return __mod_timer (timer, expires, false); }EXPORT_SYMBOL (mod_timer);/** -* mod_timer_pinned - modify a timer's timeout -* @timer: the timer to be modified -* @expires: new timeout in jiffies -* -* mod_timer_pinned() is a way to update the expire field of an -* active timer (if the timer is inactive it will be activated) -* and to ensure that the timer is scheduled on the current CPU. -* -* Note that this does not prevent the timer from being migrated -* when the current CPU goes offline. If this is a problem for -* you, use CPU-hotplug notifiers to handle it correctly, for -* example, cancelling the timer when the corresponding CPU goes -* offline. -* -* mod_timer_pinned(timer, expires) is equivalent to: -* -* del_timer(timer); timer->expires = expires; add_timer(timer); -*/ -int mod_timer_pinned (struct timer_list *timer,unsigned long expires) -{ -if (timer->expires == expires &&timer_pending (timer)) -return 1 ; - -return __mod_timer (timer, expires, false,MOD_TIMER_PINNED ); -} -EXPORT_SYMBOL (mod_timer_pinned); - -/** * add_timer - start a timer * @timer: the timer to be added *
@@ -1512,7 +1483,7 @@ signed long __sched schedule_timeout(sig expire = timeout + jiffies;setup_timer_on_stack (&timer, process_timeout, (unsigned long )current); -__mod_timer (&timer, expire, false,MOD_TIMER_NOT_PINNED ); +__mod_timer (&timer, expire, false);schedule ();del_singleshot_timer_sync (&timer);
PATCH 10/20 timer: Add a cascading tracepoint
In reply to Thomas GleixnerFor analysis of the timer wheel its necessary to trace the cascading. Add a tracepoint.
--- include/trace/events/timer.h | 11 +++++++++++ kernel/time/timer.c | 1 + 2 files changed, 12 insertions(+)
include/trace/events/timer.h +11 -0
@@ -126,6 +126,17 @@ DEFINE_EVENT(timer_class, timer_cancel, );/** +* timer_cascade - called when the timer is cascaded +* @timer: pointer to struct timer_list +*/ +DEFINE_EVENT (timer_class, timer_cascade, + + TP_PROTO(struct timer_list *timer), + +TP_ARGS (timer) +); + +/** * hrtimer_init - called when the hrtimer is initialized * @hrtimer: pointer to struct hrtimer * @clockid: the hrtimers clock
kernel/time/timer.c +1 -0
@@ -1099,6 +1099,7 @@ static int cascade(struct tvec_base *bas* don't have to detach them individually. */ hlist_for_each_entry_safe (timer, tmp, &tv_list, entry) { +trace_timer_cascade (timer);/* No accounting, while moving them */ __internal_add_timer (base, timer); }
PATCH 11/20 hlist: Add hlist_is_last_node() helper
In reply to Thomas GleixnerRequired to figure out whether the entry is the last one in the hlist.
--- include/linux/list.h | 10 ++++++++++ 1 file changed, 10 insertions(+)
include/linux/list.h +10 -0
@@ -679,6 +679,16 @@ static inline bool hlist_fake(struct hli }/* +* Check whether the node is the last node of the head without +* accessing head. +*/ +static inline bool hlist_is_last_node (struct hlist_node *n, +struct hlist_head *h) +{ +return !n->next && n->pprev == &h->first; +} + +/* * Move a list from one list head to another. Fixup the pprev * reference of the first entry if it exists. */
Re: [patch 11/20] hlist: Add hlist_is_last_node() helper
include/linux/list.h static inline bool hlist_fake(struct hli · line 689
Quoted 9 lines · Thomas Gleixner
/*
+ * Check whether the node is the last node of the head without
+ * accessing head.
+ */include/linux/list.h +5
-0+static inline bool hlist_is_last_node (struct hlist_node *n, +struct hlist_head *h) +{ +return !n->next && n->pprev == &h->first; +}
Perhaps hlist_is_only_node, or hlist_is_singular_node (which is ugly but matches list_is_singular)? last_node seems to check only for !n->next.
Thanks,
Paolo
PATCH 12/20 timer: Give a few structs and members proper names
In reply to Thomas GleixnerSome of the names are not longer correct and others are simply too long to type. Clean it up before we switch the wheel implementation over to the new scheme.
No functional change.
--- kernel/time/timer.c | 118 ++++++++++++++++++++++++++-------------------------- 1 file changed, 59 insertions(+), 59 deletions(-)
kernel/time/timer.c +59 -59
@@ -77,10 +77,10 @@ struct tvec_root {struct hlist_head vec[TVR_SIZE ]; }; -struct tvec_base { +struct timer_base {spinlock_t lock;struct timer_list *running_timer; -unsigned long timer_jiffies; +unsigned long clk;unsigned long next_timer;unsigned long active_timers;unsigned long all_timers;
@@ -95,7 +95,7 @@ struct tvec_base { }____cacheline_aligned ; -static DEFINE_PER_CPU(struct tvec_base, tvec_bases); +static DEFINE_PER_CPU(struct timer_base, timer_bases);#if defined(CONFIG_SMP ) && defined(CONFIG_NO_HZ_COMMON )unsigned int sysctl_timer_migration =1 ;
@@ -106,15 +106,15 @@ void timers_update_migration(bool updateunsigned int cpu;/* Avoid the loop, if nothing to update */ -if (this_cpu_read (tvec_bases.migration_enabled) == on) +if (this_cpu_read (timer_bases.migration_enabled) == on)return ;for_each_possible_cpu (cpu) { -per_cpu (tvec_bases.migration_enabled, cpu) = on; +per_cpu (timer_bases.migration_enabled, cpu) = on;per_cpu (hrtimer_bases.migration_enabled, cpu) = on;if (!update_nohz)continue ; -per_cpu (tvec_bases.nohz_active, cpu) = true; +per_cpu (timer_bases.nohz_active, cpu) = true;per_cpu (hrtimer_bases.nohz_active, cpu) = true; } }
@@ -134,18 +134,18 @@ int timer_migration_handler(struct ctl_t return ret; } -static inline struct tvec_base *get_target_base (struct tvec_base *base, +static inline struct timer_base *get_target_base (struct timer_base *base,int pinned) {if (pinned || !base->migration_enabled) -return this_cpu_ptr (&tvec_bases); -return per_cpu_ptr (&tvec_bases,get_nohz_timer_target ()); +return this_cpu_ptr (&timer_bases); +return per_cpu_ptr (&timer_bases,get_nohz_timer_target ()); }#else -static inline struct tvec_base *get_target_base (struct tvec_base *base, +static inline struct timer_base *get_target_base (struct timer_base *base,int pinned) { -return this_cpu_ptr (&tvec_bases); +return this_cpu_ptr (&timer_bases); }#endif
@@ -371,10 +371,10 @@ void set_timer_slack(struct timer_list *EXPORT_SYMBOL_GPL (set_timer_slack);static void -__internal_add_timer (struct tvec_base *base,struct timer_list *timer) +__internal_add_timer (struct timer_base *base,struct timer_list *timer) {unsigned long expires = timer->expires; -unsigned long idx = expires - base->timer_jiffies; +unsigned long idx = expires - base->clk;struct hlist_head *vec;if (idx <TVR_SIZE ) {
@@ -394,7 +394,7 @@ static void* Can happen if you add a timer with expires == jiffies, * or you set a timer to go off in the past */ - vec = base->tv1.vec + (base->timer_jiffies &TVR_MASK ); + vec = base->tv1.vec + (base->clk &TVR_MASK ); }else {int i;/* If the timeout is larger than MAX_TVAL (on 64-bit
@@ -403,7 +403,7 @@ static void*/ if (idx >MAX_TVAL ) { idx =MAX_TVAL ; - expires = idx + base->timer_jiffies; + expires = idx + base->clk; } i = (expires >> (TVR_BITS +3 *TVN_BITS )) &TVN_MASK ; vec = base->tv5.vec + i;
@@ -412,11 +412,11 @@ static voidhlist_add_head (&timer->entry, vec); } -static void internal_add_timer (struct tvec_base *base,struct timer_list *timer) +static void internal_add_timer (struct timer_base *base,struct timer_list *timer) {/* Advance base->jiffies, if the base is empty */ if (!base->all_timers++) - base->timer_jiffies = jiffies; + base->clk = jiffies;__internal_add_timer (base, timer);/*
@@ -707,7 +707,7 @@ static inline void detach_timer(struct t }static inline void -detach_expired_timer (struct timer_list *timer,struct tvec_base *base) +detach_expired_timer (struct timer_list *timer,struct timer_base *base) {detach_timer (timer, true);if (!(timer->flags &TIMER_DEFERRABLE ))
@@ -715,7 +715,7 @@ detach_expired_timer(struct timer_list * base->all_timers--; } -static int detach_if_pending (struct timer_list *timer,struct tvec_base *base, +static int detach_if_pending (struct timer_list *timer,struct timer_base *base,bool clear_pending) {if (!timer_pending (timer))
@@ -725,16 +725,16 @@ static int detach_if_pending(struct timeif (!(timer->flags &TIMER_DEFERRABLE )) { base->active_timers--;if (timer->expires == base->next_timer) - base->next_timer = base->timer_jiffies; + base->next_timer = base->clk; }/* If this was the last timer, advance base->jiffies */ if (!--base->all_timers) - base->timer_jiffies = jiffies; + base->clk = jiffies;return 1 ; }/* -* We are using hashed locking: holding per_cpu(tvec_bases).lock +* We are using hashed locking: holding per_cpu(timer_bases).lock * means that all timers which are tied to this base via timer->base are * locked, and the base itself is locked too. *
@@ -744,16 +744,16 @@ static int detach_if_pending(struct time* When the timer's base is locked and removed from the list, the * TIMER_MIGRATING flag is set, FIXME */ -static struct tvec_base *lock_timer_base (struct timer_list *timer, +static struct timer_base *lock_timer_base (struct timer_list *timer,unsigned long *flags) __acquires(timer->base->lock) {for (;;) {u32 tf = timer->flags; -struct tvec_base *base; +struct timer_base *base;if (!(tf &TIMER_MIGRATING )) { - base =per_cpu_ptr (&tvec_bases, tf &TIMER_CPUMASK ); + base =per_cpu_ptr (&timer_bases, tf &TIMER_CPUMASK );spin_lock_irqsave (&base->lock, *flags);if (timer->flags == tf)return base;
@@ -766,7 +766,7 @@ static struct tvec_base *lock_timer_basestatic inline int __mod_timer (struct timer_list *timer,unsigned long expires,bool pending_only) { -struct tvec_base *base, *new_base; +struct timer_base *base, *new_base;unsigned long flags;int ret =0 ;
@@ -933,8 +933,8 @@ EXPORT_SYMBOL(add_timer);*/ void add_timer_on (struct timer_list *timer,int cpu) { -struct tvec_base *new_base =per_cpu_ptr (&tvec_bases, cpu); -struct tvec_base *base; +struct timer_base *new_base =per_cpu_ptr (&timer_bases, cpu); +struct timer_base *base;unsigned long flags;timer_stats_timer_set_start_info (timer);
@@ -975,7 +975,7 @@ EXPORT_SYMBOL_GPL(add_timer_on);*/ int del_timer (struct timer_list *timer) { -struct tvec_base *base; +struct timer_base *base;unsigned long flags;int ret =0 ;
@@ -1001,7 +1001,7 @@ EXPORT_SYMBOL(del_timer);*/ int try_to_del_timer_sync (struct timer_list *timer) { -struct tvec_base *base; +struct timer_base *base;unsigned long flags;int ret =-1 ;
@@ -1085,7 +1085,7 @@ int del_timer_sync(struct timer_list *tiEXPORT_SYMBOL (del_timer_sync);#endif -static int cascade (struct tvec_base *base,struct tvec *tv,int index) +static int cascade (struct timer_base *base,struct tvec *tv,int index) {/* cascade all the timers from tv up one level */ struct timer_list *timer;
@@ -1150,7 +1150,7 @@ static void call_timer_fn(struct timer_l } } -#define INDEX (N ) ((base->timer_jiffies >> (TVR_BITS + (N) * TVN_BITS)) & TVN_MASK) +#define INDEX (N ) ((base->clk >> (TVR_BITS + (N) * TVN_BITS)) & TVN_MASK)/** * __run_timers - run all expired timers (if any) on this CPU.
@@ -1159,23 +1159,23 @@ static void call_timer_fn(struct timer_l* This function cascades all vectors and executes all expired timer * vectors. */ -static inline void __run_timers (struct tvec_base *base) +static inline void __run_timers (struct timer_base *base) {struct timer_list *timer;spin_lock_irq (&base->lock); -while (time_after_eq (jiffies, base->timer_jiffies)) { +while (time_after_eq (jiffies, base->clk)) {struct hlist_head work_list;struct hlist_head *head = &work_list;int index;if (!base->all_timers) { - base->timer_jiffies = jiffies; + base->clk = jiffies;break ; } - index = base->timer_jiffies &TVR_MASK ; + index = base->clk &TVR_MASK ;/* * Cascade timers:
@@ -1185,7 +1185,7 @@ static inline void __run_timers(struct t (!cascade(base, &base->tv3, INDEX(1))) && !cascade(base, &base->tv4, INDEX(2)))cascade (base, &base->tv5,INDEX (3 )); - ++base->timer_jiffies; + ++base->clk;hlist_move_list (base->tv1.vec + index, head);while (!hlist_empty (head)) {void (*fn)(unsigned long );
@@ -1223,16 +1223,16 @@ static inline void __run_timers(struct t* is used on S/390 to stop all activity when a CPU is idle. * This function needs to be called with interrupts disabled. */ -static unsigned long __next_timer_interrupt (struct tvec_base *base) +static unsigned long __next_timer_interrupt (struct timer_base *base) { -unsigned long timer_jiffies = base->timer_jiffies; -unsigned long expires = timer_jiffies +NEXT_TIMER_MAX_DELTA ; +unsigned long clk = base->clk; +unsigned long expires = clk +NEXT_TIMER_MAX_DELTA ;int index, slot, array, found =0 ;struct timer_list *nte;struct tvec *varray[4 ];/* Look for timer events in tv1. */ - index = slot = timer_jiffies &TVR_MASK ; + index = slot = clk &TVR_MASK ; do { hlist_for_each_entry(nte, base->tv1.vec + slot, entry) { if (nte->flags & TIMER_DEFERRABLE)
@@ -1251,8 +1251,8 @@ static unsigned long __next_timer_interr cascade:/* Calculate the next cascade event */ if (index) - timer_jiffies +=TVR_SIZE - index; - timer_jiffies >>=TVR_BITS ; + clk +=TVR_SIZE - index; + clk >>=TVR_BITS ;/* Check tv2-tv5. */ varray[0 ] = &base->tv2;
@@ -1263,7 +1263,7 @@ static unsigned long __next_timer_interrfor (array =0 ; array <4 ; array++) {struct tvec *varp = varray[array]; - index = slot = timer_jiffies &TVN_MASK ; + index = slot = clk &TVN_MASK ; do { hlist_for_each_entry(nte, varp->vec + slot, entry) { if (nte->flags & TIMER_DEFERRABLE)
@@ -1287,8 +1287,8 @@ static unsigned long __next_timer_interr }while (slot != index);if (index) - timer_jiffies +=TVN_SIZE - index; - timer_jiffies >>=TVN_BITS ; + clk +=TVN_SIZE - index; + clk >>=TVN_BITS ; }return expires; }
@@ -1336,7 +1336,7 @@ static u64 cmp_next_hrtimer_event(u64 ba*/ u64 get_next_timer_interrupt (unsigned long basej,u64 basem) { -struct tvec_base *base =this_cpu_ptr (&tvec_bases); +struct timer_base *base =this_cpu_ptr (&timer_bases);u64 expires =KTIME_MAX ;unsigned long nextevt;
@@ -1349,7 +1349,7 @@ u64 get_next_timer_interrupt(unsigned lospin_lock (&base->lock);if (base->active_timers) { -if (time_before_eq (base->next_timer, base->timer_jiffies)) +if (time_before_eq (base->next_timer, base->clk)) base->next_timer =__next_timer_interrupt (base); nextevt = base->next_timer;if (time_before_eq (nextevt, basej))
@@ -1388,9 +1388,9 @@ void update_process_times(int user_tick)*/ static void run_timer_softirq (struct softirq_action *h) { -struct tvec_base *base =this_cpu_ptr (&tvec_bases); +struct timer_base *base =this_cpu_ptr (&timer_bases); -if (time_after_eq (jiffies, base->timer_jiffies)) +if (time_after_eq (jiffies, base->clk))__run_timers (base); }
@@ -1535,7 +1535,7 @@ signed long __sched schedule_timeout_idlEXPORT_SYMBOL (schedule_timeout_idle);#ifdef CONFIG_HOTPLUG_CPU -static void migrate_timer_list (struct tvec_base *new_base,struct hlist_head *head) +static void migrate_timer_list (struct timer_base *new_base,struct hlist_head *head) {struct timer_list *timer;int cpu = new_base->cpu;
@@ -1551,13 +1551,13 @@ static void migrate_timer_list(struct tvstatic void migrate_timers (int cpu) { -struct tvec_base *old_base; -struct tvec_base *new_base; +struct timer_base *old_base; +struct timer_base *new_base;int i;BUG_ON (cpu_online (cpu)); - old_base =per_cpu_ptr (&tvec_bases, cpu); - new_base =get_cpu_ptr (&tvec_bases); + old_base =per_cpu_ptr (&timer_bases, cpu); + new_base =get_cpu_ptr (&timer_bases);/* * The caller is globally serialized and nobody else * takes two locks at once, deadlock is not possible.
@@ -1581,7 +1581,7 @@ static void migrate_timers(int cpu)spin_unlock (&old_base->lock);spin_unlock_irq (&new_base->lock); -put_cpu_ptr (&tvec_bases); +put_cpu_ptr (&timer_bases); } static int timer_cpu_notify(struct notifier_block *self,
@@ -1609,13 +1609,13 @@ static inline void timer_register_cpu_nostatic void __init init_timer_cpu (int cpu) { -struct tvec_base *base =per_cpu_ptr (&tvec_bases, cpu); +struct timer_base *base =per_cpu_ptr (&timer_bases, cpu); base->cpu = cpu;spin_lock_init (&base->lock); - base->timer_jiffies = jiffies; - base->next_timer = base->timer_jiffies; + base->clk = jiffies; + base->next_timer = base->clk; }static void __init init_timer_cpus (void )
PATCH 13/20 timer: Switch to a non cascading wheel
In reply to Thomas GleixnerThe current timer wheel has some drawbacks:
1) Cascading
Cascading can be an unbound operation and is completely pointless in most cases because the vast majority of the timer wheel timers are canceled or rearmed before expiration.
2) No fast lookup of the next expiring timer
In NOHZ scenarios the first timer soft interrupt after a long NOHZ period must fast forward the base time to current jiffies. As we have no way to find the next expiring timer fast, the code loops and increments the base time by one and checks for expired timers in each step.
After a thorough analysis of real world data gathered on laptops, workstations, webservers and other machines (thanks Chris!) I came to the conclusion that the current 'classic' timer wheel implementation can be modified to address the above issues.
The vast majority of timer wheel timers is canceled or rearmed before expiry. Most of them are timeouts for networking and other I/O tasks. The nature of timeouts is to catch the exception from normal operation (TCP ack timed out, disk does not respond, etc.). For these kind of timeouts the accuracy is not really a concern. In case the timeout fires, performance is down the drain already.
The few timers which actually expire can be split into two categories:
1) Short expiry times which expect halfways accurate expiry
2) Long term expiry times are inaccurate today already due to the batching which is done for NOHZ.
So for long term expiry timers we can avoid the cascading property and just leave them in the less granular outer wheels until expiry or cancelation.
Contrary to the classic wheel the granularities of the next wheel is not the capacity of the first wheel. The granularities of the wheels are in the currently chosen setting 8 times the granularity of the previous wheel. So for HZ=250 we end up with the following granularity levels:
Level Offset Granularity Range
0 0 4 ms 0 ms - 252 ms
1 64 32 ms 256 ms - 2044 ms (256ms - ~2s)
2 128 256 ms 2048 ms - 16380 ms (~2s - ~16s)
3 192 2048 ms (~2s) 16384 ms - 131068 ms (~16s - ~2m)
4 256 16384 ms (~16s) 131072 ms - 1048572 ms (~2m - ~17m)
5 320 131072 ms (~2m) 1048576 ms - 8388604 ms (~17m - ~2h)
That's a worst case inaccuracy of 12.5% for the timers which are queued at the beginning of a level.
So the new wheel concept addresses the old issues:
1) Cascading is avoided (except for extreme long time timers)
2) By keeping the timers in the bucket until expiry/cancelation we can track the buckets which have timers enqueued in a bucket bitmap and therefor can lookup the next expiring timer fast and time bound.
A further benefit of the concept is, that the slack calculation which is done on every timer start is not longer necessary because the granularity levels provide natural batching already.
Our extensive testing with various loads did not show any performance degradation vs. the current wheel implementation.
This patch does not address the 'fast lookup' issue as we wanted to make sure that there is no regression introduced by the wheel redesign. The optimizations are in follow up patches.
[ Contains fixes from Anna-Maria Gleixner and Richard Cochran ]
---
v3: fix return value of __next_timer_interrupt()
v2: change HASH_SIZE to TOT_HASH_SIZE (as Richard mentioned)
include/linux/timer.h | 2 kernel/time/timer.c | 825 ++++++++++++++++++++++++++++---------------------- 2 files changed, 471 insertions(+), 356 deletions(-)
include/linux/timer.h +2 -0
@@ -64,6 +64,8 @@ struct timer_list {#define TIMER_DEFERRABLE 0x00100000#define TIMER_PINNED 0x00200000#define TIMER_IRQSAFE 0x00400000 +#define TIMER_ARRAYSHIFT 23 +#define TIMER_ARRAYMASK 0xFF800000#define __TIMER_INITIALIZER (_function, _expires, _data, _flags) { \ .entry = { .next = TIMER_ENTRY_STATIC }, \
kernel/time/timer.c +469 -356
1,037 lines not on this page · raw message
Peter Zijlstra
maintainerRe: [patch 13/20] timer: Switch to a non cascading wheel
Quoted 25 lines · Thomas Gleixner
+ * HZ 1000
+ * Level Offset Granularity Range+ * 0 0 1 ms 0 ms - 63 ms + * 1 64 8 ms 64 ms - 511 ms + * 2 128 64 ms 512 ms - 4095 ms (512ms - ~4s) + * 3 192 512 ms 4096 ms - 32767 ms (~4s - ~32s) + * 4 256 4096 ms (~4s) 32768 ms - 262143 ms (~32s - ~4m) + * 5 320 32768 ms (~32s) 262144 ms - 2097151 ms (~4m - ~34m)kernel/time/timer.c +15
-0+static int collect_expired_timers (struct timer_base *base, +struct hlist_head *heads) +{ +unsigned long clock = base->clk; +struct hlist_head tmp, *vec; +int i, levels =0 ; +unsigned int idx; + +/* Expire the regular buckets */ +for (i =0 ; i <LVL_DEPTH -1 ; i++) { + idx = (clock &LVL_MASK ) + i *LVL_SIZE ; + +if (__test_and_clear_bit (idx, base->pending_map)) { + vec = base->vectors + idx; +hlist_move_list (vec, heads++);
kernel/time/timer.c static void call_timer_fn(struct timer_l · line 1281
Quoted 10 lines
kernel/time/timer.c +10
-0+ levels++; + } +/* Is it time to look at the next level? */ +if (clock &LVL_CLK_MASK ) +return levels; +/* Shift clock for the next level granularity */ + clock >>=LVL_CLK_SHIFT ; + } + +/* Cascading, sigh... */
So given that userspace has no influence on timer period; can't we simply fail to support timers longer than 30 minutes?
In anything really arming timers _that_ long?
Quoted 19 lines
kernel/time/timer.c +19
-0+ idx = (clock &LVL_MASK ) + i *LVL_SIZE ; + +if (__test_and_clear_bit (idx, base->pending_map)) { + vec = base->vectors + idx; +hlist_move_list (vec, &tmp); +/* Make sure we queue them in the future */ + base->clk++; +while (!hlist_empty (&tmp)) { +struct timer_list *timer; + + timer = hlist_entry(tmp.first, struct timer_list, entry); +trace_timer_cascade (timer); +__hlist_del (&timer->entry); +__internal_add_timer (base, timer); + } + base->clk--; + } +return levels; +}
Re: [patch 13/20] timer: Switch to a non cascading wheel
Quoted 8 lines · Peter Zijlstra
Review
Quoted 2 lines · Thomas Gleixner
(quoted diff) +2
-0+ + /* Cascading, sigh... */So given that userspace has no influence on timer period; can't we simply fail to support timers longer than 30 minutes?
In anything really arming timers _that_ long?
Unfortunately yes. Networking being one of those. Real cascading happens once in a blue moon, but it happens.
Thanks,
tglx
Re: [patch 13/20] timer: Switch to a non cascading wheel
Quoted 5 lines · Thomas Gleixner
Quoted 4 lines · Peter Zijlstra
Quoted 2 lines · Thomas Gleixner
(quoted diff) +2
-0+ + /* Cascading, sigh... */
Quoted 8 lines
Quoted 4 lines
So given that userspace has no influence on timer period; can't we simply fail to support timers longer than 30 minutes?
In anything really arming timers _that_ long?
Unfortunately yes. Networking being one of those. Real cascading happens once in a blue moon, but it happens.
Anyway, for the more common HZ=1000 case, what would the 'limit' be ?
Re: [patch 13/20] timer: Switch to a non cascading wheel
Quoted 9 lines · Eric Dumazet
Quoted 8 lines · Thomas Gleixner
Quoted 7 lines · Peter Zijlstra
Review
Quoted 2 lines · Thomas Gleixner
(quoted diff) +2
-0+ + /* Cascading, sigh... */So given that userspace has no influence on timer period; can't we simply fail to support timers longer than 30 minutes?
Quoted 6 lines
Quoted 5 lines
Quoted 1 line
In anything really arming timers _that_ long?
Unfortunately yes. Networking being one of those. Real cascading happens once in a blue moon, but it happens.
Anyway, for the more common HZ=1000 case, what would the 'limit' be ?
34 minutes with the currently chosen parameters.
Ingo Molnar
maintainerRe: [patch 13/20] timer: Switch to a non cascading wheel
In reply to Thomas GleixnerQuoted 8 lines · Thomas Gleixner
Quoted 7 lines · Peter Zijlstra
Review
Quoted 2 lines · Thomas Gleixner
(quoted diff) +2
-0+ + /* Cascading, sigh... */So given that userspace has no influence on timer period; can't we simply fail to support timers longer than 30 minutes?
Quoted 4 lines
Quoted 1 line
In anything really arming timers _that_ long?
Unfortunately yes. Networking being one of those. Real cascading happens once in a blue moon, but it happens.
So I'd really prefer it if we added a few more levels, a hard limit and got rid of the cascading once and for all!
IMHO 'once in a blue moon' code is much worse than a bit more data overhead.
Thanks,
Ingo
Re: [patch 13/20] timer: Switch to a non cascading wheel
Quoted 11 lines · Ingo Molnar
Quoted 10 lines · Thomas Gleixner
Quoted 8 lines · Peter Zijlstra
Review
Quoted 2 lines · Thomas Gleixner
(quoted diff) +2
-0+ + /* Cascading, sigh... */So given that userspace has no influence on timer period; can't we simply fail to support timers longer than 30 minutes?
In anything really arming timers _that_ long?
Quoted 7 lines
Quoted 2 lines
Unfortunately yes. Networking being one of those. Real cascading happens once in a blue moon, but it happens.
So I'd really prefer it if we added a few more levels, a hard limit and got rid of the cascading once and for all!
IMHO 'once in a blue moon' code is much worse than a bit more data overhead.
I agree. If we add two wheel levels then we end up with:
HZ 1000: 134217727 ms ~= 37 hours
HZ 250: 536870908 ms ~= 149 hours
HZ 100: 1342177270 ms ~= 372 hours
Looking through all my data I found exactly one timeout which is insanely
large: 120 hours!
That's net/netfilter/nf_conntrack_core.c:
setup_timer(&ct->timeout, death_by_timeout, (unsigned long)ct);
Anything else is way below 37 hours.
Thanks,
tglx
Re: [patch 13/20] timer: Switch to a non cascading wheel
evaluating a 120 hours timer ever 37 hours to see if it should fire... not too horrid.
Quoted 37 lines · Thomas Gleixner
Quoted 11 lines · Ingo Molnar
Quoted 10 lines · Thomas Gleixner
Quoted 8 lines · Peter Zijlstra
Review
Quoted 2 lines · Thomas Gleixner
(quoted diff) +2
-0+ + /* Cascading, sigh... */So given that userspace has no influence on timer period; can't we simply fail to support timers longer than 30 minutes?
In anything really arming timers _that_ long?
Quoted 7 lines
Quoted 2 lines
Unfortunately yes. Networking being one of those. Real cascading happens once in a blue moon, but it happens.
So I'd really prefer it if we added a few more levels, a hard limit and got rid of the cascading once and for all!
IMHO 'once in a blue moon' code is much worse than a bit more data overhead.
I agree. If we add two wheel levels then we end up with:
HZ 1000: 134217727 ms ~= 37 hours
HZ 250: 536870908 ms ~= 149 hours
HZ 100: 1342177270 ms ~= 372 hoursLooking through all my data I found exactly one timeout which is insanely
large: 120 hours!That's
net/netfilter/nf_conntrack_core.c:setup_timer(&ct->timeout, death_by_timeout, (unsigned long)ct);Anything else is way below 37 hours.
Thanks,
tglx
Re: [patch 13/20] timer: Switch to a non cascading wheel
Quoted 2 lines · Arjan van de Ven
evaluating a 120 hours timer ever 37 hours to see if it should fire... not too horrid.
Well that thing is doing weird stuff anyway:
swapper 0 [001] 1789995.305532: timer:timer_start: timer=0xffff8800c8346920 function=death_by_timeout expires=4850639994 [timeout=108000000]
ssh 3870 [001] 1790025.284704: timer:timer_cancel: timer=0xffff8800c8346920
ssh 3870 [001] 1790025.284707: timer:timer_start: timer=0xffff8800c8346920 function=death_by_timeout expires=4742722493 [timeout=75000]
swapper 0 [001] 1790025.330514: timer:timer_cancel: timer=0xffff8800c8346920
swapper 0 [001] 1790025.330515: timer:timer_start: timer=0xffff8800c8346920 function=death_by_timeout expires=4850647504 [timeout=108000000]
ssh 3870 [001] 1790055.307058: timer:timer_cancel: timer=0xffff8800c8346920
ssh 3870 [001] 1790055.307060: timer:timer_start: timer=0xffff8800c8346920 function=death_by_timeout expires=4742730003 [timeout=75000]
swapper 0 [001] 1790055.352146: timer:timer_cancel: timer=0xffff8800c8346920And that goes on forever. 2834 such sequences for this particular timer instance in 4.5 hours. 90000 sequences total for all timers related to death_by_timeout in 4.5 hours
No idea what this is doing and why the heck it nees a 120 hour timeout ....
Thanks,
tglx
Peter Zijlstra
maintainerRe: [patch 13/20] timer: Switch to a non cascading wheel
Quoted 8 lines · Thomas Gleixner
Quoted 2 lines · Arjan van de Ven
evaluating a 120 hours timer ever 37 hours to see if it should fire... not too horrid.
Well that thing is doing weird stuff anyway:
swapper 0 [001] 1789995.305532: timer:timer_start: timer=0xffff8800c8346920 function=death_by_timeout expires=4850639994 [timeout=108000000]
You're running a HZ=250 kernel, right?
inet/netfilter/nf_conntrack_proto_tcp.c:
#define SECS * HZ
#define MINS * 60 SECS
#define HOURS * 60 MINS
#define DAYS * 24 HOURS
static unsigned int tcp_timeouts[TCP_CONNTRACK_TIMEOUT_MAX] __read_mostly = {
[TCP_CONNTRACK_SYN_SENT] = 2 MINS,
[TCP_CONNTRACK_SYN_RECV] = 60 SECS,
[TCP_CONNTRACK_ESTABLISHED] = 5 DAYS,
^^^^^^ thatends up being 108000000 for HZ == 250.
[TCP_CONNTRACK_FIN_WAIT] = 2 MINS, [TCP_CONNTRACK_CLOSE_WAIT] = 60 SECS, [TCP_CONNTRACK_LAST_ACK] = 30 SECS, [TCP_CONNTRACK_TIME_WAIT] = 2 MINS, [TCP_CONNTRACK_CLOSE] = 10 SECS, [TCP_CONNTRACK_SYN_SENT2] = 2 MINS, /* RFC1122 says the R2 limit should be at least 100 seconds. Linux uses 15 packets as limit, which corresponds to ~13-30min depending on RTO. */ [TCP_CONNTRACK_RETRANS] = 5 MINS, [TCP_CONNTRACK_UNACK] = 5 MINS, };
Quoted 2 lines
ssh 3870 [001] 1790025.284704: timer:timer_cancel: timer=0xffff8800c8346920
ssh 3870 [001] 1790025.284707: timer:timer_start: timer=0xffff8800c8346920 function=death_by_timeout expires=4742722493 [timeout=75000]And that one would then be one of the 5 MINS ones.
Peter Zijlstra
maintainerRe: [patch 13/20] timer: Switch to a non cascading wheel
In reply to Thomas GleixnerQuoted 16 lines · Thomas Gleixner
Quoted 2 lines · Arjan van de Ven
evaluating a 120 hours timer ever 37 hours to see if it should fire... not too horrid.
Well that thing is doing weird stuff anyway:
swapper 0 [001] 1789995.305532: timer:timer_start: timer=0xffff8800c8346920 function=death_by_timeout expires=4850639994 [timeout=108000000] ssh 3870 [001] 1790025.284704: timer:timer_cancel: timer=0xffff8800c8346920 ssh 3870 [001] 1790025.284707: timer:timer_start: timer=0xffff8800c8346920 function=death_by_timeout expires=4742722493 [timeout=75000] swapper 0 [001] 1790025.330514: timer:timer_cancel: timer=0xffff8800c8346920 swapper 0 [001] 1790025.330515: timer:timer_start: timer=0xffff8800c8346920 function=death_by_timeout expires=4850647504 [timeout=108000000] ssh 3870 [001] 1790055.307058: timer:timer_cancel: timer=0xffff8800c8346920 ssh 3870 [001] 1790055.307060: timer:timer_start: timer=0xffff8800c8346920 function=death_by_timeout expires=4742730003 [timeout=75000] swapper 0 [001] 1790055.352146: timer:timer_cancel: timer=0xffff8800c8346920
Quoted 5 lines
And that goes on forever. 2834 such sequences for this particular timer instance in 4.5 hours. 90000 sequences total for all timers related to
death_by_timeoutin 4.5 hoursNo idea what this is doing and why the heck it nees a 120 hour timeout ....
So it moves that timer on every packet for that TCP connection stream, provided the expiration is at least 1 second behind.
If the stream hasn't had a packet in 5 days (see previous email), then the connection state is destroyed.
Its been too long since I've read the TCP RFCs, but I can imagine changing this will upset people.
Re: [patch 13/20] timer: Switch to a non cascading wheel
Quoted 23 lines · Peter Zijlstra
Quoted 16 lines · Thomas Gleixner
Quoted 2 lines · Arjan van de Ven
evaluating a 120 hours timer ever 37 hours to see if it should fire... not too horrid.
Well that thing is doing weird stuff anyway:
swapper 0 [001] 1789995.305532: timer:timer_start: timer=0xffff8800c8346920 function=death_by_timeout expires=4850639994 [timeout=108000000] ssh 3870 [001] 1790025.284704: timer:timer_cancel: timer=0xffff8800c8346920 ssh 3870 [001] 1790025.284707: timer:timer_start: timer=0xffff8800c8346920 function=death_by_timeout expires=4742722493 [timeout=75000] swapper 0 [001] 1790025.330514: timer:timer_cancel: timer=0xffff8800c8346920 swapper 0 [001] 1790025.330515: timer:timer_start: timer=0xffff8800c8346920 function=death_by_timeout expires=4850647504 [timeout=108000000] ssh 3870 [001] 1790055.307058: timer:timer_cancel: timer=0xffff8800c8346920 ssh 3870 [001] 1790055.307060: timer:timer_start: timer=0xffff8800c8346920 function=death_by_timeout expires=4742730003 [timeout=75000] swapper 0 [001] 1790055.352146: timer:timer_cancel: timer=0xffff8800c8346920Quoted 5 lines
And that goes on forever. 2834 such sequences for this particular timer instance in 4.5 hours. 90000 sequences total for all timers related to
death_by_timeoutin 4.5 hoursNo idea what this is doing and why the heck it nees a 120 hour timeout ....
Quoted 9 lines
So it moves that timer on every packet for that TCP connection stream, provided the expiration is at least 1 second behind.
If the stream hasn't had a packet in 5 days (see previous email), then the connection state is destroyed.
Its been too long since I've read the TCP RFCs, but I can imagine changing this will upset people.
Original TCP RFCs tell timeout is infinite ;)
Practically, conntrack has a 5 days timeout, but I really doubt anyone expects an idle TCP flow to stay 'alive' when nothing is sent for 5 days.
Re: [patch 13/20] timer: Switch to a non cascading wheel
Quoted 5 lines · Eric Dumazet
Original TCP RFCs tell timeout is infinite ;)
Practically, conntrack has a 5 days timeout, but I really doubt anyone expects an idle TCP flow to stay 'alive' when nothing is sent for 5 days.
So would 37hrs ~= 1.5 days be a reasonable cutoff or will stuff fall apart and people be surprised?
Thanks,
tglx
Re: [patch 13/20] timer: Switch to a non cascading wheel
what would 1 more timer wheel do?
Quoted 14 lines · Thomas Gleixner
Quoted 5 lines · Eric Dumazet
Original TCP RFCs tell timeout is infinite ;)
Practically, conntrack has a 5 days timeout, but I really doubt anyone expects an idle TCP flow to stay 'alive' when nothing is sent for 5 days.
So would 37hrs ~= 1.5 days be a reasonable cutoff or will stuff fall apart and people be surprised?
Thanks,
tglx
Re: [patch 13/20] timer: Switch to a non cascading wheel
Quoted 1 line · Arjan van de Ven
what would 1 more timer wheel do?
Waste storage space and make the collection of expired timers more expensive.
The selection of the timer wheel properties is combination of:
1) Granularity
2) Storage space
3) Number of levels to collect
#1 Influences the accuracy of the wheel levels. The worst case error is the
ratio of the shortest timeout in a level and the granularity of that level.
Currently I've chosen 12.5%. If we go to 25% worst case, then we increase the per level capacity by factor (1 << lvl).
#2 It strikes me a bit silly to waste storage space for esoteric
outliers. According to the data I collected we have no timers at all between 2hrs and 5 days.
#3 The more levels we have the more steps we have when collecting expired
timers or searching the next expiring timer. It's all bound, but it still adds up.
Thanks,
tglx
Re: [patch 13/20] timer: Switch to a non cascading wheel
Quoted 5 lines · Thomas Gleixner
Quoted 1 line · Arjan van de Ven
what would 1 more timer wheel do?
Waste storage space and make the collection of expired timers more expensive.
Quoted 7 lines
The selection of the timer wheel properties is combination of:
1) Granularity
2) Storage space
3) Number of levels to collect
So I came up with a slightly different solution for this. The problem case is HZ=1000 and again looking at the data, there is no reason why we need actual 1ms granularity for timer wheel timers. That's independent of the desired ms based interfaces.
We can simply run the wheel internaly with 4ms base level resolution and degrade from there. That gives us 6 days+ and a simple cutoff at the capacity of the 7th level wheel.
0 0 4 ms 0 ms - 255 ms
1 64 32 ms 256 ms - 2047 ms (256ms - ~2s)
2 128 256 ms 2048 ms - 16383 ms (~2s - ~16s) 3 192 2048 ms (~2s) 16384 ms - 131071 ms (~16s - ~2m) 4 256 16384 ms (~16s) 131072 ms - 1048575 ms (~2m - ~17m) 5 320 131072 ms (~2m) 1048576 ms - 8388607 ms (~17m - ~2h) 6 384 1048576 ms (~17m) 8388608 ms - 67108863 ms (~2h - ~18h) 7 448 8388608 ms (~2h) 67108864 ms - 536870911 ms (~18h - ~6d)
That works really nice and has the interesting side effect that we batch in the first level wheel which helps networking. I'll repost the series with the other review points addressed later tonight.
Btw, I also thought a bit more about the milliseconds interfaces. I think we shouldn't invent new interfaces. The correct solution IMHO is to distangle the scheduler tick frequency and jiffies. If we have that completely seperated then we can do the following:
1) Force HZ=1000. That means jiffies and timer wheel units are 1ms. If the tick frequency is != 1000 we simply increment jiffies in the tick by the proper amount (4 @250 ticks/sec, 10 @100 ticks/sec).
So all msec_to_jiffies() invocations compile out into nothing magically and we can remove them gradually over time.
2) When we do that right, we can make the tick frequency a command line option and just have a compiled in default.
Thoughts?
Thanks,
tglx
Re: [patch 13/20] timer: Switch to a non cascading wheel
Quoted 41 lines · Thomas Gleixner
Quoted 5 lines · Thomas Gleixner
Quoted 1 line · Arjan van de Ven
what would 1 more timer wheel do?
Waste storage space and make the collection of expired timers more expensive.
Quoted 7 lines
The selection of the timer wheel properties is combination of:
1) Granularity
2) Storage space
3) Number of levels to collect
So I came up with a slightly different solution for this. The problem case is HZ=1000 and again looking at the data, there is no reason why we need actual 1ms granularity for timer wheel timers. That's independent of the desired ms based interfaces.
We can simply run the wheel internaly with 4ms base level resolution and degrade from there. That gives us 6 days+ and a simple cutoff at the capacity of the 7th level wheel.
0 0 4 ms 0 ms - 255 ms
1 64 32 ms 256 ms - 2047 ms (256ms - ~2s)2 128 256 ms 2048 ms - 16383 ms (~2s - ~16s) 3 192 2048 ms (~2s) 16384 ms - 131071 ms (~16s - ~2m) 4 256 16384 ms (~16s) 131072 ms - 1048575 ms (~2m - ~17m) 5 320 131072 ms (~2m) 1048576 ms - 8388607 ms (~17m - ~2h) 6 384 1048576 ms (~17m) 8388608 ms - 67108863 ms (~2h - ~18h) 7 448 8388608 ms (~2h) 67108864 ms - 536870911 ms (~18h - ~6d)That works really nice and has the interesting side effect that we batch in the first level wheel which helps networking. I'll repost the series with the other review points addressed later tonight.
Btw, I also thought a bit more about the milliseconds interfaces. I think we shouldn't invent new interfaces. The correct solution IMHO is to distangle the scheduler tick frequency and jiffies. If we have that completely seperated then we can do the following:
Quoted 6 lines
1) Force HZ=1000. That means jiffies and timer wheel units are 1ms. If the tick frequency is != 1000 we simply increment jiffies in the tick by the proper amount (4 @250 ticks/sec, 10 @100 ticks/sec).
So all
msec_to_jiffies()invocations compile out into nothing magically and we can remove them gradually over time.
Some of RCU's heuristics assume that if scheduling-clock ticks happen, they happen once per jiffy. These would need to be adjusted, which would not be a big deal, just a bit more use of HZ.
Quoted 2 lines
2) When we do that right, we can make the tick frequency a command line option and just have a compiled in default.
As long as there is something that tells RCU what the tick frequency actually is at runtime, this should not be a problem. For example, in rcu_implicit_dynticks_qs(), the following:
rdp->rsp->jiffies_resched += 5;
Would instead need to be something like:
rdp->rsp->jiffies_resched += 5 * jiffies_per_tick;
Changing tick frequency at runtime would be a bit more tricky, as it would be tough to avoid some oddball false positives during the transition. But setting it at boot time would be fine. ;-)
Thanx, Paul
Quoted 6 lines
Thoughts?
Thanks,
tglx
Peter Zijlstra
maintainerRe: [patch 13/20] timer: Switch to a non cascading wheel
Quoted 7 lines · Paul E. McKenney
Quoted 2 lines
2) When we do that right, we can make the tick frequency a command line option and just have a compiled in default.
As long as there is something that tells RCU what the tick frequency actually is at runtime, this should not be a problem. For example, in
rcu_implicit_dynticks_qs(), the following:
Quoted 8 lines
rdp->rsp->jiffies_resched += 5;Would instead need to be something like:
rdp->rsp->jiffies_resched += 5 * jiffies_per_tick;Changing tick frequency at runtime would be a bit more tricky, as it would be tough to avoid some oddball false positives during the transition.
So the 'fun' part will be frequencies with non integer factors of 1000. Like say HZ=300. For that we'll have to keep jiffies_remainder, and add an extra jiffy every time that rolls over.
That would make your case slightly more interesting than you really want I suspect.
Re: [patch 13/20] timer: Switch to a non cascading wheel
Quoted 17 lines · Peter Zijlstra
Quoted 7 lines · Paul E. McKenney
Quoted 2 lines
2) When we do that right, we can make the tick frequency a command line option and just have a compiled in default.
As long as there is something that tells RCU what the tick frequency actually is at runtime, this should not be a problem. For example, in
rcu_implicit_dynticks_qs(), the following:Quoted 8 lines
rdp->rsp->jiffies_resched += 5;Would instead need to be something like:
rdp->rsp->jiffies_resched += 5 * jiffies_per_tick;Changing tick frequency at runtime would be a bit more tricky, as it would be tough to avoid some oddball false positives during the transition.
Quoted 6 lines
So the 'fun' part will be frequencies with non integer factors of 1000. Like say HZ=300. For that we'll have to keep
jiffies_remainder, and add an extra jiffy every time that rolls over.That would make your case slightly more interesting than you really want I suspect.
My particular case is not that sensitive, so 1000/300 would be plenty accurate.
Thanx, Paul
Re: [patch 13/20] timer: Switch to a non cascading wheel
Quoted 22 lines · Paul E. McKenney
Quoted 21 lines · Peter Zijlstra
Quoted 7 lines · Paul E. McKenney
Quoted 2 lines
2) When we do that right, we can make the tick frequency a command line option and just have a compiled in default.
As long as there is something that tells RCU what the tick frequency actually is at runtime, this should not be a problem. For example, in
rcu_implicit_dynticks_qs(), the following:Quoted 8 lines
rdp->rsp->jiffies_resched += 5;Would instead need to be something like:
rdp->rsp->jiffies_resched += 5 * jiffies_per_tick;Changing tick frequency at runtime would be a bit more tricky, as it would be tough to avoid some oddball false positives during the transition.
So the 'fun' part will be frequencies with non integer factors of 1000. Like say HZ=300. For that we'll have to keep
jiffies_remainder, and add an extra jiffy every time that rolls over.
Quoted 5 lines
Quoted 2 lines
That would make your case slightly more interesting than you really want I suspect.
My particular case is not that sensitive, so 1000/300 would be plenty accurate.
That said... If someone were to set HZ=501, now -that- could be a bit problematic. :-/
Thanx, Paul
Re: [patch 13/20] timer: Switch to a non cascading wheel
In reply to Thomas GleixnerI think there's 2 elements on the interface.
1) having a relative interface to the current time (avoid use of absolute jiffies in drivers)
2) having wallclock units. Making HZ always be 1000 is effectively doing that as well (1 msec after all)
Quoted 56 lines · Thomas Gleixner
Quoted 5 lines · Thomas Gleixner
Quoted 1 line · Arjan van de Ven
what would 1 more timer wheel do?
Waste storage space and make the collection of expired timers more expensive.
Quoted 7 lines
The selection of the timer wheel properties is combination of:
1) Granularity
2) Storage space
3) Number of levels to collect
So I came up with a slightly different solution for this. The problem case is HZ=1000 and again looking at the data, there is no reason why we need actual 1ms granularity for timer wheel timers. That's independent of the desired ms based interfaces.
We can simply run the wheel internaly with 4ms base level resolution and degrade from there. That gives us 6 days+ and a simple cutoff at the capacity of the 7th level wheel.
0 0 4 ms 0 ms - 255 ms
1 64 32 ms 256 ms - 2047 ms (256ms - ~2s)2 128 256 ms 2048 ms - 16383 ms (~2s - ~16s) 3 192 2048 ms (~2s) 16384 ms - 131071 ms (~16s - ~2m) 4 256 16384 ms (~16s) 131072 ms - 1048575 ms (~2m - ~17m) 5 320 131072 ms (~2m) 1048576 ms - 8388607 ms (~17m - ~2h) 6 384 1048576 ms (~17m) 8388608 ms - 67108863 ms (~2h - ~18h) 7 448 8388608 ms (~2h) 67108864 ms - 536870911 ms (~18h - ~6d)That works really nice and has the interesting side effect that we batch in the first level wheel which helps networking. I'll repost the series with the other review points addressed later tonight.
Btw, I also thought a bit more about the milliseconds interfaces. I think we shouldn't invent new interfaces. The correct solution IMHO is to distangle the scheduler tick frequency and jiffies. If we have that completely seperated then we can do the following:
1) Force HZ=1000. That means jiffies and timer wheel units are 1ms. If the tick frequency is != 1000 we simply increment jiffies in the tick by the proper amount (4 @250 ticks/sec, 10 @100 ticks/sec).
So all
msec_to_jiffies()invocations compile out into nothing magically and we can remove them gradually over time.2) When we do that right, we can make the tick frequency a command line option and just have a compiled in default.
Thoughts?
Thanks,
tglx
Re: [patch 13/20] timer: Switch to a non cascading wheel
Quoted 4 lines · Arjan van de Ven
I think there's 2 elements on the interface.
1) having a relative interface to the current time (avoid use of absolute jiffies in drivers)
That's the easy part :)
Quoted 2 lines
2) having wallclock units. Making HZ always be 1000 is effectively doing that as well (1 msec after all)
Right. And it allows us to switch over the whole code base w/o fiddling with it in one go. Cleanups should obviously take place after that.
Thanks,
tglx
Re: [patch 13/20] timer: Switch to a non cascading wheel
In reply to Thomas GleixnerQuoted 10 lines · Thomas Gleixner
Quoted 5 lines · Eric Dumazet
Original TCP RFCs tell timeout is infinite ;)
Practically, conntrack has a 5 days timeout, but I really doubt anyone expects an idle TCP flow to stay 'alive' when nothing is sent for 5 days.
So would 37hrs ~= 1.5 days be a reasonable cutoff or will stuff fall apart and people be surprised?
It seems very reasonable to me at least.
Re: [patch 13/20] timer: Switch to a non cascading wheel
In reply to Thomas Gleixnerkernel/time/timer.c static inline void detach_timer(struct t · line 847
Quoted 4 lines · Thomas Gleixner
kernel/time/timer.c +4
-0+static inline struct timer_base *get_timer_base (u32 tflags) +{ +return get_timer_cpu_base (tflags, tflags &TIMER_BASEMASK ); +}
This should rather be (tflags & TIMER_CPUMASK) to avoid using per_cpu_ptr() with the TIMER_MIGRATING bit set in the CPU index.
The one caller in this patch is okay, since it already checks that TIMER_MIGRATING is clear:
kernel/time/timer.c static inline void detach_timer(struct t · line 881
Quoted 13 lines
static struct
timer_base*lock_timer_base(structtimer_list*timer,kernel/time/timer.c +3
-3- unsigned long *flags) + unsigned long *flags) __acquires(timer->base->lock) {for (;;) { -u32 tf = timer->flags;struct timer_base *base; +u32 tf = timer->flags;if (!(tf &TIMER_MIGRATING )) { - base =per_cpu_ptr (&timer_bases, tf &TIMER_CPUMASK ); + base =get_timer_base (tf);
However, in patch #20, we'll have this in __mod_timer();
/* * Take the current timer_jiffies of base, but without holding * the lock! */ base = get_timer_base(timer->flags);
Thanks,
Richard
Re: [patch 13/20] timer: Switch to a non cascading wheel
Quoted 11 lines · Richard Cochran
kernel/time/timer.c static inline void detach_timer(struct t · line 847
Quoted 4 lines · Thomas Gleixner
kernel/time/timer.c +4
-0+static inline struct timer_base *get_timer_base (u32 tflags) +{ +return get_timer_cpu_base (tflags, tflags &TIMER_BASEMASK ); +}This should rather be (tflags &
TIMER_CPUMASK) to avoid usingper_cpu_ptr()with theTIMER_MIGRATINGbit set in the CPU index.The one caller in this patch is okay, since it already checks that
TIMER_MIGRATINGis clear:
Good catch!
Thanks,
tglx
PATCH 14/20 timer: Remove slack leftovers
In reply to Thomas GleixnerWe now have implicit batching in the timer wheel. The slack is not longer used. Remove it.
--- block/genhd.c | 5 ----- drivers/mmc/host/jz4740_mmc.c | 2 -- drivers/power/bq27xxx_battery.c | 5 +---- drivers/usb/host/ohci-hcd.c | 1 - drivers/usb/host/xhci.c | 2 -- include/linux/timer.h | 4 ---- kernel/time/timer.c | 19 ------------------- lib/random32.c | 1 - 8 files changed, 1 insertion(+), 38 deletions(-)
block/genhd.c +0 -5
@@ -1523,12 +1523,7 @@ static void __disk_unblock_events(structif (--ev->block) goto out_unlock; -/* -* Not exactly a latency critical operation, set poll timer -* slack to 25% and kick event check. -*/ intv =disk_events_poll_jiffies (disk); -set_timer_slack (&ev->dwork.timer, intv /4 );if (check_now)queue_delayed_work (system_freezable_power_efficient_wq, &ev->dwork,0 );
drivers/mmc/host/jz4740_mmc.c +0 -2
@@ -1068,8 +1068,6 @@ static int jz4740_mmc_probe(struct platfjz4740_mmc_clock_disable (host);setup_timer (&host->timeout_timer, jz4740_mmc_timeout, (unsigned long )host); -/* It is not important when it times out, it just needs to timeout. */ -set_timer_slack (&host->timeout_timer,HZ ); host->use_dma = true;if (host->use_dma &&jz4740_mmc_acquire_dma_channels (host) !=0 )
drivers/power/bq27xxx_battery.c +1 -4
@@ -735,11 +735,8 @@ static void bq27xxx_battery_poll(structbq27xxx_battery_update (di); -if (poll_interval >0 ) { -/* The timer does not have to be accurate. */ -set_timer_slack (&di->work.timer, poll_interval *HZ /4 ); +if (poll_interval >0 )schedule_delayed_work (&di->work, poll_interval *HZ ); - } }/*
drivers/usb/host/ohci-hcd.c +0 -1
@@ -500,7 +500,6 @@ static int ohci_init (struct ohci_hcd *osetup_timer (&ohci->io_watchdog, io_watchdog_func, (unsigned long ) ohci); -set_timer_slack (&ohci->io_watchdog,msecs_to_jiffies (20 )); ohci->hcca =dma_alloc_coherent (hcd->self.controller,sizeof (*ohci->hcca), &ohci->hcca_dma,GFP_KERNEL );
drivers/usb/host/xhci.c +0 -2
@@ -490,8 +490,6 @@ static void compliance_mode_recovery_tim xhci->comp_mode_recovery_timer.expires = jiffies +msecs_to_jiffies (COMP_MODE_RCVRY_MSECS ); -set_timer_slack (&xhci->comp_mode_recovery_timer, -msecs_to_jiffies (COMP_MODE_RCVRY_MSECS ));add_timer (&xhci->comp_mode_recovery_timer);xhci_dbg_trace (xhci, trace_xhci_dbg_quirks,"Compliance mode recovery timer initialized" );
include/linux/timer.h +0 -4
@@ -19,7 +19,6 @@ struct timer_list {void (*function)(unsigned long );unsigned long data;u32 flags; -int slack;#ifdef CONFIG_TIMER_STATS int start_pid;
@@ -73,7 +72,6 @@ struct timer_list { .expires = (_expires), \ .data = (_data), \ .flags = (_flags), \ - .slack = -1, \__TIMER_LOCKDEP_MAP_INITIALIZER ( \ __FILE__":" __stringify(__LINE__)) \ }
@@ -193,8 +191,6 @@ extern int del_timer(struct timer_list *extern int mod_timer (struct timer_list *timer,unsigned long expires);extern int mod_timer_pending (struct timer_list *timer,unsigned long expires); -extern void set_timer_slack (struct timer_list *time,int slack_hz); -/* * The jiffies value which is added to now, when there is no timer * in the timer wheel:
kernel/time/timer.c +0 -19
@@ -432,24 +432,6 @@ unsigned long round_jiffies_up_relative( }EXPORT_SYMBOL_GPL (round_jiffies_up_relative); -/** -* set_timer_slack - set the allowed slack for a timer -* @timer: the timer to be modified -* @slack_hz: the amount of time (in jiffies) allowed for rounding -* -* Set the amount of time, in jiffies, that a certain timer has -* in terms of slack. By setting this value, the timer subsystem -* will schedule the actual timer somewhere between -* the time mod_timer() asks for, and that time plus the slack. -* -* By setting the slack to -1, a percentage of the delay is used -* instead. -*/ -void set_timer_slack (struct timer_list *timer,int slack_hz) -{ - timer->slack = slack_hz; -} -EXPORT_SYMBOL_GPL (set_timer_slack);static inline unsigned int timer_get_idx (struct timer_list *timer) {
@@ -753,7 +735,6 @@ static void do_init_timer(struct timer_l { timer->entry.pprev =NULL ; timer->flags = flags |raw_smp_processor_id (); - timer->slack =-1 ;#ifdef CONFIG_TIMER_STATS timer->start_site =NULL ; timer->start_pid =-1 ;
lib/random32.c +0 -1
@@ -233,7 +233,6 @@ static void __prandom_timer(unsigned lonstatic void __init __prandom_start_seed_timer (void ) { -set_timer_slack (&seed_timer,HZ ); seed_timer.expires = jiffies +msecs_to_jiffies (40 *MSEC_PER_SEC );add_timer (&seed_timer); }
PATCH 15/20 timer: Move __run_timers() function
In reply to Thomas GleixnerFrom: Anna-Maria Gleixner <[email protected]>
Move __run_timers() below __next_timer_interrupt() and next_pending_bucket() in preparation for __run_timers() NOHZ optimization.
No functional change.
--- kernel/time/timer.c | 46 +++++++++++++++++++++++----------------------- 1 file changed, 23 insertions(+), 23 deletions(-)
kernel/time/timer.c +23 -23
@@ -1280,29 +1280,6 @@ static int collect_expired_timers(structreturn levels; } -/** -* __run_timers - run all expired timers (if any) on this CPU. -* @base: the timer vector to be processed. -*/ -static inline void __run_timers (struct timer_base *base) -{ -struct hlist_head heads[LVL_DEPTH ]; -int levels; - -spin_lock_irq (&base->lock); - -while (time_after_eq (jiffies, base->clk)) { - - levels =collect_expired_timers (base, heads); - base->clk++; - -while (levels--) -expire_timers (base, heads + levels); - } - base->running_timer =NULL ; -spin_unlock_irq (&base->lock); -} -#ifdef CONFIG_NO_HZ_COMMON /* * Find the next pending bucket of a level. Search from @offset + @clk upwards
@@ -1478,6 +1455,29 @@ void update_process_times(int user_tick)run_posix_cpu_timers (p); } +/** +* __run_timers - run all expired timers (if any) on this CPU. +* @base: the timer vector to be processed. +*/ +static inline void __run_timers (struct timer_base *base) +{ +struct hlist_head heads[LVL_DEPTH ]; +int levels; + +spin_lock_irq (&base->lock); + +while (time_after_eq (jiffies, base->clk)) { + + levels =collect_expired_timers (base, heads); + base->clk++; + +while (levels--) +expire_timers (base, heads + levels); + } + base->running_timer =NULL ; +spin_unlock_irq (&base->lock); +} +/* * This function runs timers and the timer-tq in bottom half context. */
PATCH 16/20 timer: Optimize collect timers for NOHZ
In reply to Thomas GleixnerFrom: Anna-Maria Gleixner <[email protected]>
After a NOHZ idle sleep the wheel must be forwarded to current jiffies. There might be expired timers so the current code loops and checks the epxired buckets for timers. This can take quite some time for long NOHZ idle periods.
The pending bitmask in the timer base allows us to do a quick search for the next expiring timer and therefor a fast forward of the base time which prevents pointless long lasting loops.
For a 3 second idle sleep this reduces the catchup time from ~1ms to 5us.
--- kernel/time/timer.c | 48 ++++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 40 insertions(+), 8 deletions(-)
kernel/time/timer.c +40 -8
@@ -1235,8 +1235,8 @@ static void expire_timers(struct timer_b } } -static int collect_expired_timers (struct timer_base *base, -struct hlist_head *heads) +static int __collect_expired_timers (struct timer_base *base, +struct hlist_head *heads) {unsigned long clock = base->clk;struct hlist_head tmp, *vec;
@@ -1282,9 +1282,9 @@ static int collect_expired_timers(struct#ifdef CONFIG_NO_HZ_COMMON /* -* Find the next pending bucket of a level. Search from @offset + @clk upwards -* and if nothing there, search from start of the level (@offset) up to -* @offset + clk. +* Find the next pending bucket of a level. Search from level start (@offset) +* + @clk upwards and if nothing there, search from start of the level +* (@offset) up to @offset + clk. */ static int next_pending_bucket (struct timer_base *base,unsigned offset,unsigned clk)
@@ -1301,7 +1301,8 @@ static int next_pending_bucket(struct ti }/* -* Search the first expiring timer in the various clock levels. +* Search the first expiring timer in the various clock levels. Caller must +* hold base->lock. * * Note: This implementation might be suboptimal vs. timers enqueued in the * cascade level because we do not look at the timers to figure out when
@@ -1314,7 +1315,6 @@ static unsigned long __next_timer_interrunsigned long clk, next, adj;unsigned lvl, offset =0 ; -spin_lock (&base->lock); clk = base->clk; next = clk +NEXT_TIMER_MAX_DELTA ;for (lvl =0 ; lvl <LVL_DEPTH ; lvl++, offset +=LVL_SIZE ) {
@@ -1367,7 +1367,6 @@ static unsigned long __next_timer_interr clk >>=LVL_CLK_SHIFT ; clk += adj; } -spin_unlock (&base->lock);return next; }
@@ -1425,7 +1424,10 @@ u64 get_next_timer_interrupt(unsigned loif (cpu_is_offline (smp_processor_id ()))return expires; +spin_lock (&base->lock); nextevt =__next_timer_interrupt (base); +spin_unlock (&base->lock); +if (time_before_eq (nextevt, basej)) expires = basem;else
@@ -1433,6 +1435,36 @@ u64 get_next_timer_interrupt(unsigned lo return cmp_next_hrtimer_event(basem, expires); } + +static int collect_expired_timers (struct timer_base *base, +struct hlist_head *heads) +{ +/* +* NOHZ optimization. After a long idle sleep we need to forward the +* base to current jiffies. Avoid a loop by searching the bitfield for +* the next expiring timer. +*/ +if ((jiffies - base->clk) >1 ) { +unsigned long next =__next_timer_interrupt (base); + +/* +* If the next timer is ahead of time forward to current +* jiffies, otherwise forward to the next expiry time. +*/ +if (time_after (next, jiffies)) { + base->clk = jiffies; +return 0 ; + } + base->clk = next; + } +return __collect_expired_timers (base, heads); +} +#else +static inline int collect_expired_timers (struct timer_base *base, +struct hlist_head *heads) +{ +return __collect_expired_timers (base, heads); +}#endif /*
PATCH 17/20 tick/sched: Remove pointless empty function
In reply to Thomas GleixnerThis was a failed attempt to optimize the timer expiry in idle, which was disabled and never revisited. Remove the cruft.
--- kernel/time/tick-sched.c | 33 +-------------------------------- 1 file changed, 1 insertion(+), 32 deletions(-)
kernel/time/tick-sched.c +1 -32
@@ -1092,35 +1092,6 @@ static void tick_nohz_switch_to_nohz(voitick_nohz_activate (ts,NOHZ_MODE_LOWRES ); } -/* -* When NOHZ is enabled and the tick is stopped, we need to kick the -* tick timer from irq_enter() so that the jiffies update is kept -* alive during long running softirqs. That's ugly as hell, but -* correctness is key even if we need to fix the offending softirq in -* the first place. -* -* Note, this is different to tick_nohz_restart. We just kick the -* timer and do not touch the other magic bits which need to be done -* when idle is left. -*/ -static void tick_nohz_kick_tick (struct tick_sched *ts,ktime_t now) -{ -#if 0 -/* Switch back to 2.6.27 behaviour */ -ktime_t delta; - -/* -* Do not touch the tick device, when the next expiry is either -* already reached or less/equal than the tick period. -*/ - delta =ktime_sub (hrtimer_get_expires (&ts->sched_timer), now); -if (delta.tv64 <= tick_period.tv64) -return ; - -tick_nohz_restart (ts, now); -#endif -} -static inline void tick_nohz_irq_enter (void ) {struct tick_sched *ts =this_cpu_ptr (&tick_cpu_sched);
@@ -1131,10 +1102,8 @@ static inline void tick_nohz_irq_enter(v now =ktime_get ();if (ts->idle_active)tick_nohz_stop_idle (ts, now); -if (ts->tick_stopped) { +if (ts->tick_stopped) tick_nohz_update_jiffies(now); -tick_nohz_kick_tick (ts, now); - } }#else
PATCH 18/20 timer: Forward wheel clock whenever possible
In reply to Thomas GleixnerThe wheel clock is stale when a cpu goes into a long idle sleep. This has the side effect, that timers which are queued end up in the outer wheel levels. That results in coarser granularity.
To solve this, we keep track of the idle state and forward the wheel clock whenever it's possible.
--- kernel/time/tick-internal.h | 1 kernel/time/tick-sched.c | 13 ++++ kernel/time/timer.c | 120 +++++++++++++++++++++++++++++++++++--------- 3 files changed, 111 insertions(+), 23 deletions(-)
kernel/time/tick-internal.h +1 -0
@@ -164,3 +164,4 @@ static inline void timers_update_migrati DECLARE_PER_CPU(struct hrtimer_cpu_base, hrtimer_bases);extern u64 get_next_timer_interrupt (unsigned long basej,u64 basem); +void timer_clear_idle (void );
kernel/time/tick-sched.c +13 -0
@@ -700,6 +700,12 @@ static ktime_t tick_nohz_stop_sched_tick delta = next_tick - basemono;if (delta <= (u64 )TICK_NSEC ) { tick.tv64 =0 ; + +/* +* Tell the timer code that the base is not idle, i.e. undo +* the effect of get_next_timer_interrupt(). +*/ +timer_clear_idle ();/* * We've not stopped the tick yet, and there's a timer in the * next period, so no point in stopping it either, bail.
@@ -809,6 +815,12 @@ static void tick_nohz_restart_sched_ticktick_do_update_jiffies64 (now);cpu_load_update_nohz_stop (); +/* +* Clear the timer idle flag, so we avoid IPIs on remote queueing and +* the clock forward checks in the enqueue path. +*/ +timer_clear_idle (); +calc_load_exit_idle ();touch_softlockup_watchdog_sched ();/*
@@ -1025,6 +1037,7 @@ void tick_nohz_idle_exit(void)tick_nohz_stop_idle (ts, now);if (ts->tick_stopped) { +timer_clear_idle ();tick_nohz_restart_sched_tick (ts, now);tick_nohz_account_idle_ticks (ts); }
kernel/time/timer.c +97 -23
167 lines not on this page · raw message
Re: [patch 18/20] timer: Forward wheel clock whenever possible
Thomas,
Quoted 18 lines · Thomas Gleixner
kernel/time/timer.c +0
-14@@ -498,23 +500,27 @@ static void internal_add_timer(struct ti__internal_add_timer (base, timer);/* -* Check whether the other CPU is in dynticks mode and needs -* to be triggered to reevaluate the timer wheel. We are -* protected against the other CPU fiddling with the timer by -* holding the timer base lock. This also makes sure that a -* CPU on the way to stop its tick can not evaluate the timer -* wheel. -* -* Spare the IPI for deferrable timers on idle targets though. -* The next busy ticks will take care of it. Except full dynticks -* require special care against races with idle_cpu(), lets deal -* with that later. -*/ -if (IS_ENABLED (CONFIG_NO_HZ_COMMON ) && base->nohz_active) { - if (!(timer->flags & TIMER_DEFERRABLE) ||
kernel/time/timer.c static void internal_add_timer(struct ti · line 509
Quoted 10 lines
kernel/time/timer.c +7
-3- tick_nohz_full_cpu(base->cpu)) -wake_up_nohz_cpu (base->cpu); - } +* 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 base lock. +*/ +if (!IS_ENABLED (CONFIG_NO_HZ_COMMON ) || !base->is_idle || + (timer->flags &TIMER_DEFERRABLE )) +return ;
The tests for is_idle and TIMER_DEFERRABLE are actually checking the same thing. I was looking at the usage of base.is_idle, and it is only ever set for BASE_STD.
So the TIMER_DEFERRABLE test is redundant, but maybe this is on purpose? Anyhow, it did leave me scratching my head.
Thanks,
Richard
Re: [patch 18/20] timer: Forward wheel clock whenever possible
Quoted 34 lines · Richard Cochran
Quoted 18 lines · Thomas Gleixner
kernel/time/timer.c +0
-14@@ -498,23 +500,27 @@ static void internal_add_timer(struct ti__internal_add_timer (base, timer);/* -* Check whether the other CPU is in dynticks mode and needs -* to be triggered to reevaluate the timer wheel. We are -* protected against the other CPU fiddling with the timer by -* holding the timer base lock. This also makes sure that a -* CPU on the way to stop its tick can not evaluate the timer -* wheel. -* -* Spare the IPI for deferrable timers on idle targets though. -* The next busy ticks will take care of it. Except full dynticks -* require special care against races with idle_cpu(), lets deal -* with that later. -*/ -if (IS_ENABLED (CONFIG_NO_HZ_COMMON ) && base->nohz_active) { - if (!(timer->flags & TIMER_DEFERRABLE) ||kernel/time/timer.c static void internal_add_timer(struct ti · line 509
Quoted 10 lines
kernel/time/timer.c +7
-3- tick_nohz_full_cpu(base->cpu)) -wake_up_nohz_cpu (base->cpu); - } +* 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 base lock. +*/ +if (!IS_ENABLED (CONFIG_NO_HZ_COMMON ) || !base->is_idle || + (timer->flags &TIMER_DEFERRABLE )) +return ;The tests for
is_idleandTIMER_DEFERRABLEare actually checking the same thing. I was looking at the usage of base.is_idle, and it is only ever set forBASE_STD.
Quoted 2 lines
So the
TIMER_DEFERRABLEtest is redundant, but maybe this is on purpose? Anyhow, it did leave me scratching my head.
You're right. is_idle makes the deferrable check obsolete. We can remove it, but it needs a big fat comment ....
Thanks
tglx
PATCH 19/20 timer: Split out index calculation
In reply to Thomas GleixnerFrom: Anna-Maria Gleixner <[email protected]>
For further optimizations we need to seperate index calculation and queueing. No functional change.
--- kernel/time/timer.c | 41 +++++++++++++++++++++++++++++++++-------- 1 file changed, 33 insertions(+), 8 deletions(-)
kernel/time/timer.c +33 -8
@@ -456,12 +456,9 @@ static inline unsigned calc_index(unsignreturn base + (((expires + gran) >> sft) &LVL_MASK ); } -static void -__internal_add_timer (struct timer_base *base,struct timer_list *timer) +static int calc_wheel_index (unsigned long expires,unsigned long clk) { -unsigned long expires = timer->expires; -unsigned long delta = expires - base->clk; -struct hlist_head *vec; +unsigned long delta = expires - clk;unsigned int idx;if (delta <LVL1_TSTART ) {
@@ -475,7 +472,7 @@ static void }else if (delta <LVL5_TSTART ) { idx =calc_index (expires,LVL4_GRAN ,LVL4_SHIFT ,LVL4_OFFS ); }else if ((long ) delta <0 ) { - idx = base->clk &LVL_MASK ; + idx = clk &LVL_MASK ; }else {/* * The long timeouts go into the last array level. They
@@ -485,6 +482,18 @@ static void idx =calc_index (expires,LVL5_GRAN ,LVL5_SHIFT ,LVL5_OFFS ); } +return idx; +} + +/* +* Enqueue the timer into the hash bucket, mark it pending in +* the bitmap and store the index in the timer flags. +*/ +static void enqueue_timer (struct timer_base *base,struct timer_list *timer, +unsigned int idx) +{ +struct hlist_head *vec; +/* * Enqueue the timer into the array bucket, mark it pending in * the bitmap and store the index in the timer flags.
@@ -495,10 +504,19 @@ static voidtimer_set_idx (timer, idx); } -static void internal_add_timer (struct timer_base *base,struct timer_list *timer) +static void +__internal_add_timer (struct timer_base *base,struct timer_list *timer) { -__internal_add_timer (base, timer); +unsigned long expires = timer->expires; +unsigned int idx; + + idx =calc_wheel_index (expires, base->clk); +enqueue_timer (base, timer, idx); +} +static void +trigger_dyntick_cpu (struct timer_base *base,struct timer_list *timer) +{/* * 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
@@ -523,6 +541,13 @@ static void internal_add_timer(struct tiwake_up_nohz_cpu (base->cpu); } +static void +internal_add_timer (struct timer_base *base,struct timer_list *timer) +{ +__internal_add_timer (base, timer); +trigger_dyntick_cpu (base, timer); +} +#ifdef CONFIG_TIMER_STATS void __timer_stats_timer_set_start_info (struct timer_list *timer,void *addr) {
PATCH 20/20 timer: Optimization for same expiry time in mod_timer()
In reply to Thomas GleixnerFrom: Anna-Maria Gleixner <[email protected]>
The existing optimization for same expiry time in mod_timer() checks whether the timer expiry time is the same as the new requested expiry time. In the old timer wheel implementation this does not take the slack batching into account, neither does the new implementation evaluate whether the new expiry time will requeue the timer to the same bucket.
To optimize that, we can calculate the resulting bucket and check if the new expiry time is different from the current expiry time. This calculation happens outside the base lock held region. If the resulting bucket is the same we can avoid taking the base lock and requeueing the timer.
If the timer needs to be requeued then we have to check under the base lock whether the base time has changed between the lockless calculation and taking the lock. If it has changed we need to recalculate under the lock.
This optimization takes effect for timers which are enqueued into the less granular wheel levels (1 and above). With a simple test case the functionality has been verified:
Before After
Match: 5.5% 86.6%
Requeue: 94.5% 13.4% Recalc: <0.01%
In the non optimized case the timer is requeued in 94.5% of the cases. With the index optimization in place the requeue rate drops to 13.4%. The case where the lockless index calculation has to be redone is less than 0.01%.
With a real world test case (networking) we observed the following changes:
Before After
Match: 97.8% 99.7%
Requeue: 2.2% 0.3% Recalc: <0.001%
That means two percent less lock/requeue/unlock operations in one of the hot path use cases of timers.
--- kernel/time/timer.c | 51 +++++++++++++++++++++++++++++++++++---------------- 1 file changed, 35 insertions(+), 16 deletions(-)
kernel/time/timer.c +35 -16
@@ -938,28 +938,36 @@ static inline int __mod_timer(struct timer_list *timer, unsigned long expires, bool pending_only) { struct timer_base *base, *new_base; -unsigned long flags; +unsigned int idx =UINT_MAX ; +unsigned long clk =0 , flags;int ret =0 ;/* -* TODO: Calculate the array bucket of the timer right here w/o -* holding the base lock. This allows to check not only -* timer->expires == expires below, but also whether the timer -* ends up in the same bucket. If we really need to requeue -* the timer then we check whether base->clk have -* advanced between here and locking the timer base. If -* jiffies advanced we have to recalc the array bucket with the -* lock held. -*/ - -/* -* This is a common optimization triggered by the -* networking code - if the timer is re-modified -* to be the same thing then just return: +* This is a common optimization triggered by the networking code - if +* the timer is re-modified to be the same thing or ends up in the +* same array bucket then just return: */ if (timer_pending (timer)) {if (timer->expires == expires)return 1 ; +/* +* Take the current timer_jiffies of base, but without holding +* the lock! +*/ + base =get_timer_base (timer->flags); + clk = base->clk; + + idx =calc_wheel_index (expires, clk); + +/* +* Retrieve and compare the array index of the pending +* timer. If it matches set the expiry to the new value so a +* subsequent call will exit in the expires check above. +*/ +if (idx ==timer_get_idx (timer)) { + timer->expires = expires; +return 1 ; + } }timer_stats_timer_set_start_info (timer);
@@ -996,7 +1004,18 @@ static inline int } timer->expires = expires; -internal_add_timer (base, timer); +/* +* If idx was calculated above and the base time did not advance +* between calculating idx and taking the lock, only enqueue_timer() +* and trigger_dyntick_cpu() is required. Otherwise we need to +* (re)calculate the wheel index via internal_add_timer(). +*/ +if (idx !=UINT_MAX && clk == base->clk) { +enqueue_timer (base, timer, idx); +trigger_dyntick_cpu (base, timer); + }else { +internal_add_timer (base, timer); + } out_unlock:spin_unlock_irqrestore (&base->lock, flags);
Quoted 1 line · Thomas Gleixner
The current timer wheel has some drawbacks:
...
Quoted 8 lines
This series has also preparatory patches for changing the NOHZ timer handling from the current push to a pull model. Currently we decide at timer enqueue time on which cpu we queue the timer. This is exceptionally silly because there is no way to predict at enqueue time which cpu will be idle when the timer expires. Given the fact that most timers are canceled or rearmed before expiry this is even more silly. We trade a expensive decision and cross cpu access for a very doubtful benefit.
Also many TCP timers are armed from softirq handler interrupting idle task, so scheduler believes current cpu is idle .
This patch series is really nice Thomas.
Quoted 10 lines · Thomas Gleixner
The current timer wheel has some drawbacks:
1) Cascading
Cascading can be an unbound operation and is completely pointless in most cases because the vast majority of the timer wheel timers are canceled or rearmed before expiration.
2) No fast lookup of the next expiring timer
Quoted 8 lines
In NOHZ scenarios the first timer soft interrupt after a long NOHZ period must fast forward the base time to current jiffies. As we have no way to find the next expiring timer fast, the code loops and increments the base time by one and checks for expired timers in each step. I've observed loops lasting 1 ms!
There are some other issues caused by the above, but they are minor compare to those.
For SMP configurations, this passes light rcutorture testing. For UP builds, it complains about undefined symbols. Builds succeed with the following kneejerk patch. Am retesting rcutorture.
Thanx, Paul
------------------------------------------------------------------------
commit 87dbd35cf4034f2b664b6e9d60decd2bdbfc416f
Author: Paul E. McKenney <[email protected]> Date: Mon Jun 13 09:11:41 2016 -0700
Fix undefined get_nohz_timer_target() in non-SMP builds
include/linux/sched.h +4 -0
@@ -348,6 +348,10 @@ extern int get_nohz_timer_target(void);#else static inline void nohz_balance_enter_idle (int cpu) { }static inline void set_cpu_sd_state_idle (void ) { } +static inline int get_nohz_timer_target (void ) +{ +return raw_smp_processor_id (); +}#endif /*
Paul E. McKenney
Tested-byQuoted 17 lines · Paul E. McKenney
Quoted 16 lines · Thomas Gleixner
The current timer wheel has some drawbacks:
1) Cascading
Cascading can be an unbound operation and is completely pointless in most cases because the vast majority of the timer wheel timers are canceled or rearmed before expiration.
2) No fast lookup of the next expiring timer
In NOHZ scenarios the first timer soft interrupt after a long NOHZ period must fast forward the base time to current jiffies. As we have no way to find the next expiring timer fast, the code loops and increments the base time by one and checks for expired timers in each step. I've observed loops lasting 1 ms!
Quoted 6 lines
Quoted 2 lines
There are some other issues caused by the above, but they are minor compare to those.
For SMP configurations, this passes light rcutorture testing. For UP builds, it complains about undefined symbols. Builds succeed with the following kneejerk patch. Am retesting rcutorture.
And with the patch below, testing goes as well with your patch stack as it does without it. So, with that patch (or equivalent):
There were some complaints about increasing the size of the tiny configuration, FYI.
So, just out of curiosity, does anyone still run -rt on single-CPU systems?
Thanx, Paul
Quoted 25 lines
------------------------------------------------------------------------
commit
87dbd35cf4034f2b664b6e9d60decd2bdbfc416fAuthor: Paul E. McKenney <[email protected]> Date: Mon Jun 13 09:11:41 2016 -0700Fix undefined
get_nohz_timer_target()in non-SMP buildsSigned-off-by Paul E. McKenney <[email protected]>include/linux/sched.h +4
-0@@ -348,6 +348,10 @@ extern int get_nohz_timer_target(void);#else static inline void nohz_balance_enter_idle (int cpu) { }static inline void set_cpu_sd_state_idle (void ) { } +static inline int get_nohz_timer_target (void ) +{ +return raw_smp_processor_id (); +}#endif /*
Quoted 25 lines · Paul E. McKenney
Quoted 17 lines · Paul E. McKenney
Quoted 16 lines · Thomas Gleixner
The current timer wheel has some drawbacks:
1) Cascading
Cascading can be an unbound operation and is completely pointless in most cases because the vast majority of the timer wheel timers are canceled or rearmed before expiration.
2) No fast lookup of the next expiring timer
In NOHZ scenarios the first timer soft interrupt after a long NOHZ period must fast forward the base time to current jiffies. As we have no way to find the next expiring timer fast, the code loops and increments the base time by one and checks for expired timers in each step. I've observed loops lasting 1 ms!
Quoted 6 lines
Quoted 2 lines
There are some other issues caused by the above, but they are minor compare to those.
For SMP configurations, this passes light rcutorture testing. For UP builds, it complains about undefined symbols. Builds succeed with the following kneejerk patch. Am retesting rcutorture.
Quoted 7 lines
And with the patch below, testing goes as well with your patch stack as it does without it. So, with that patch (or equivalent):
Tested-by Paul E. McKenney <[email protected]>There were some complaints about increasing the size of the tiny configuration, FYI.
I know. The extra storage space for the deferrable stuff makes it larger along with the extra code for avoiding all the crap which the current wheel suffers from :) Do the tiny people need NOHZ?
Quoted 1 line
So, just out of curiosity, does anyone still run -rt on single-CPU systems?
Of course :)
Thanks,
tglx
Quoted 29 lines · Thomas Gleixner
Quoted 28 lines · Paul E. McKenney
Quoted 17 lines · Paul E. McKenney
Quoted 16 lines · Thomas Gleixner
The current timer wheel has some drawbacks:
1) Cascading
Cascading can be an unbound operation and is completely pointless in most cases because the vast majority of the timer wheel timers are canceled or rearmed before expiration.
2) No fast lookup of the next expiring timer
In NOHZ scenarios the first timer soft interrupt after a long NOHZ period must fast forward the base time to current jiffies. As we have no way to find the next expiring timer fast, the code loops and increments the base time by one and checks for expired timers in each step. I've observed loops lasting 1 ms!
Quoted 6 lines
Quoted 2 lines
There are some other issues caused by the above, but they are minor compare to those.
For SMP configurations, this passes light rcutorture testing. For UP builds, it complains about undefined symbols. Builds succeed with the following kneejerk patch. Am retesting rcutorture.
And with the patch below, testing goes as well with your patch stack as it does without it. So, with that patch (or equivalent):
Quoted 8 lines
Quoted 4 lines
Tested-by Paul E. McKenney <[email protected]>There were some complaints about increasing the size of the tiny configuration, FYI.
I know. The extra storage space for the deferrable stuff makes it larger along with the extra code for avoiding all the crap which the current wheel suffers from :) Do the tiny people need NOHZ?
If it makes the code bigger, I would hope that they don't need it.
Quoted 3 lines
Quoted 1 line
So, just out of curiosity, does anyone still run -rt on single-CPU systems?
Of course :)
Hey, had to ask! ;-)
Thanx, Paul