Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751778AbdG0PN6 (ORCPT ); Thu, 27 Jul 2017 11:13:58 -0400 Received: from foss.arm.com ([217.140.101.70]:48256 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751655AbdG0PN4 (ORCPT ); Thu, 27 Jul 2017 11:13:56 -0400 Date: Thu, 27 Jul 2017 16:14:03 +0100 From: Will Deacon To: qiaozhou Cc: Thomas Gleixner , John Stultz , sboyd@codeaurora.org, LKML , Wang Wilbur , Marc Zyngier , Peter Zijlstra Subject: Re: [Question]: try to fix contention between expire_timers and try_to_del_timer_sync Message-ID: <20170727151400.GE20746@arm.com> References: <3d2459c7-defd-a47e-6cea-007c10cecaac@asrmicro.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 1183 Lines: 29 On Thu, Jul 27, 2017 at 09:29:20AM +0800, qiaozhou wrote: > On 2017年07月26日 22:16, Thomas Gleixner wrote: > >--- a/kernel/time/timer.c > >+++ b/kernel/time/timer.c > >@@ -1301,10 +1301,12 @@ static void expire_timers(struct timer_b > > if (timer->flags & TIMER_IRQSAFE) { > > raw_spin_unlock(&base->lock); > > call_timer_fn(timer, fn, data); > >+ base->running_timer = NULL; > > raw_spin_lock(&base->lock); > > } else { > > raw_spin_unlock_irq(&base->lock); > > call_timer_fn(timer, fn, data); > >+ base->running_timer = NULL; > > raw_spin_lock_irq(&base->lock); > > } > > } > It should work for this particular issue and I'll test it. Previously I > thought it was unsafe to touch base->running_timer without holding lock. I think it works out in practice because base->lock and base->running_timer share a cacheline, so end up being ordered correctly. We should probably be using READ_ONCE/WRITE_ONCE for accessing the running_time field though. One thing I don't get though, is why try_to_del_timer_sync needs to check base->running_timer at all. Given that it holds the base->lock, can't it be the person that sets it to NULL? Will