Messages in this thread | | | Date | Tue, 12 May 2015 09:52:09 -0400 | From | Sasha Levin <> | Subject | Re: [tip:timers/core] hrtimer: Allow concurrent hrtimer_start() for self restarting timers |
| |
On 04/22/2015 03:15 PM, tip-bot for Peter Zijlstra wrote: > Commit-ID: 5de2755c8c8b3a6b8414870e2c284914a2b42e4d > Gitweb: http://git.kernel.org/tip/5de2755c8c8b3a6b8414870e2c284914a2b42e4d > Author: Peter Zijlstra <peterz@infradead.org> > AuthorDate: Tue, 20 May 2014 15:49:48 +0200 > Committer: Thomas Gleixner <tglx@linutronix.de> > CommitDate: Wed, 22 Apr 2015 17:06:52 +0200 > > hrtimer: Allow concurrent hrtimer_start() for self restarting timers > > Because we drop cpu_base->lock around calling hrtimer::function, it is > possible for hrtimer_start() to come in between and enqueue the timer. > > If hrtimer::function then returns HRTIMER_RESTART we'll hit the BUG_ON > because HRTIMER_STATE_ENQUEUED will be set. > > Since the above is a perfectly valid scenario, remove the BUG_ON and > make the enqueue_hrtimer() call conditional on the timer not being > enqueued already. > > NOTE: in that concurrent scenario its entirely common for both sites > to want to modify the hrtimer, since hrtimers don't provide > serialization themselves be sure to provide some such that the > hrtimer::function and the hrtimer_start() caller don't both try and > fudge the expiration state at the same time. > > To that effect, add a WARN when someone tries to forward an already > enqueued timer, the most common way to change the expiry of self > restarting timers. Ideally we'd put the WARN in everything modifying > the expiry but most of that is inlines and we don't need the bloat. > > Fixes: 2d44ae4d7135 ("hrtimer: clean up cpu->base locking tricks") > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> > Cc: Ben Segall <bsegall@google.com> > Cc: Roman Gushchin <klamm@yandex-team.ru> > Cc: Paul Turner <pjt@google.com> > Link: http://lkml.kernel.org/r/20150415113105.GT5029@twins.programming.kicks-ass.net > Signed-off-by: Thomas Gleixner <tglx@linutronix.de> > --- > kernel/time/hrtimer.c | 12 +++++++++--- > 1 file changed, 9 insertions(+), 3 deletions(-) > > diff --git a/kernel/time/hrtimer.c b/kernel/time/hrtimer.c > index 3bac942..4adf320 100644 > --- a/kernel/time/hrtimer.c > +++ b/kernel/time/hrtimer.c > @@ -799,6 +799,9 @@ u64 hrtimer_forward(struct hrtimer *timer, ktime_t now, ktime_t interval) > if (delta.tv64 < 0) > return 0; > > + if (WARN_ON(timer->state & HRTIMER_STATE_ENQUEUED)) > + return 0; > + > if (interval.tv64 < hrtimer_resolution) > interval.tv64 = hrtimer_resolution;
Hey Peter,
I seem to be hitting this warning with the latest -next (2015-05-11):
[3344291.055800] WARNING: CPU: 0 PID: 9422 at kernel/time/hrtimer.c:802 hrtimer_forward+0x1f9/0x330() [3344291.057883] Modules linked in: [3344291.058734] CPU: 0 PID: 9422 Comm: trinity-main Tainted: G W 4.1.0-rc3-next-20150511-sa sha-00037-g87c65d4-dirty #2199 [3344291.061763] ffff880003a88000 00000000d4a58de9 ffff880021007c88 ffffffffabc833ac [3344291.063726] 0000000000000000 0000000000000000 ffff880021007cd8 ffffffffa21f0a86 [3344291.065656] ffffffffae6936c8 ffffffffa2386319 ffff880021007cb8 ffff8800211f5f90 [3344291.067590] Call Trace: [3344291.068085] <IRQ> [<ffffffffabc833ac>] dump_stack+0x4f/0x7b [3344291.068949] [<ffffffffa21f0a86>] warn_slowpath_common+0xc6/0x120 [3344291.070382] [<ffffffffa21f0cca>] warn_slowpath_null+0x1a/0x20 [3344291.071078] [<ffffffffa2386319>] hrtimer_forward+0x1f9/0x330 [3344291.071834] [<ffffffffa24fc7a0>] perf_mux_hrtimer_handler+0x340/0x750 [3344291.072616] [<ffffffffa238830c>] __hrtimer_run_queues+0x30c/0x10d0 [3344291.075592] [<ffffffffa238a7ac>] hrtimer_interrupt+0x19c/0x480 [3344291.076340] [<ffffffffa20c1bef>] local_apic_timer_interrupt+0x6f/0xc0 [3344291.077156] [<ffffffffabcf3af3>] smp_trace_apic_timer_interrupt+0xe3/0x859 [3344291.077968] [<ffffffffabcf1df0>] trace_apic_timer_interrupt+0x70/0x80 [3344291.078729] <EOI> [<ffffffffa2620890>] ? arch_local_irq_restore+0x10/0x30 [3344291.079582] [<ffffffffa2626ff4>] __slab_alloc+0x704/0x790 [3344291.082482] [<ffffffffa2627324>] kmem_cache_alloc+0x2a4/0x3a0 [3344291.083944] [<ffffffffa27cdf6d>] proc_alloc_inode+0x1d/0x270 [3344291.084631] [<ffffffffa26d1a7b>] alloc_inode+0x5b/0x170 [3344291.085278] [<ffffffffa26d7501>] new_inode_pseudo+0x11/0xd0 [3344291.085950] [<ffffffffa27ce698>] proc_get_inode+0x18/0x580 [3344291.086615] [<ffffffffa27dd946>] proc_lookup_de+0xc6/0x160 [3344291.088717] [<ffffffffa27f168c>] proc_tgid_net_lookup+0x5c/0xa0 [3344291.089435] [<ffffffffa269f470>] lookup_real+0x90/0x120 [3344291.090071] [<ffffffffa26a0183>] __lookup_hash+0xe3/0x100 [3344291.092954] [<ffffffffa26a96d7>] walk_component+0x697/0xc10 [3344291.096353] [<ffffffffa26ae0cf>] path_lookupat+0x13f/0x380 [3344291.097727] [<ffffffffa26ae441>] filename_lookup+0x131/0x1f0 [3344291.098411] [<ffffffffa26b4d11>] user_path_at_empty+0xc1/0x160 [3344291.101545] [<ffffffffa26b4dc1>] user_path_at+0x11/0x20 [3344291.102274] [<ffffffffa268f131>] vfs_fstatat+0xb1/0x140 [3344291.105842] [<ffffffffa2690299>] SYSC_newfstatat+0x79/0xd0 [3344291.108737] [<ffffffffa269041e>] SyS_newfstatat+0xe/0x10 [3344291.109377] [<ffffffffabcf0ee2>] tracesys_phase2+0x88/0x8d
Thanks, Sasha
| |