Re: [tip:timers/core] hrtimer: Allow concurrent hrtimer_start() for self restarting timers

[Sasha Levin]

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@xxxxxxxxxxxxx>
> AuthorDate: Tue, 20 May 2014 15:49:48 +0200
> Committer:  Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> 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@xxxxxxxxxxxxx>
> Cc: Ben Segall <bsegall@xxxxxxxxxx>
> Cc: Roman Gushchin <klamm@xxxxxxxxxxxxxx>
> Cc: Paul Turner <pjt@xxxxxxxxxx>
> Link: http://lkml.kernel.org/r/20150415113105.GT5029@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
> Signed-off-by: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> ---
>  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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/