From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755484AbcANTer (ORCPT ); Thu, 14 Jan 2016 14:34:47 -0500 Received: from e36.co.us.ibm.com ([32.97.110.154]:60542 "EHLO e36.co.us.ibm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754103AbcANTed (ORCPT ); Thu, 14 Jan 2016 14:34:33 -0500 X-IBM-Helo: d03dlp02.boulder.ibm.com X-IBM-MailFrom: paulmck@linux.vnet.ibm.com X-IBM-RcptTo: linux-kernel@vger.kernel.org Date: Thu, 14 Jan 2016 10:18:46 -0800 From: "Paul E. McKenney" To: Thomas Gleixner Cc: Sasha Levin , LKML , Peter Zijlstra Subject: Re: timers: HARDIRQ-safe -> HARDIRQ-unsafe lock order detected Message-ID: <20160114181846.GZ3818@linux.vnet.ibm.com> Reply-To: paulmck@linux.vnet.ibm.com References: <56955C0F.1090005@oracle.com> <20160113161608.GN3818@linux.vnet.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) X-TM-AS-MML: disable X-Content-Scanned: Fidelis XPS MAILER x-cbid: 16011419-0021-0000-0000-0000161B5E9E Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Jan 14, 2016 at 06:43:16PM +0100, Thomas Gleixner wrote: > On Wed, 13 Jan 2016, Paul E. McKenney wrote: > > On Wed, Jan 13, 2016 at 10:05:49AM +0100, Thomas Gleixner wrote: > > > We can fix that particular issue in the posix-timer code by making the > > > locking symetric: > > > > > > rcu_read_lock(); > > > spin_lock_irq(timer->lock); > > > > > > ... > > > > > > spin_unlock_irq(timer->lock); > > > rcu_read_unlock(); > > > > > > instead of: > > > > > > rcu_read_lock(); > > > spin_lock_irq(timer->lock); > > > rcu_read_unlock(); > > > > > > ... > > > > > > spin_unlock_irq(timer->lock); > > > > > > But the question is, whether this is the only offending code path in tree. We > > > can avoid the hassle by making rtmutex->wait_lock irq safe. > > > > > > Thoughts? > > > > Given that the lock is disabling irq, I don't see a problem with > > extending the RCU read-side critical section to cover the entire > > irq-disabled region. > > I cannot follow here. What would be different if the lock would not disable > irqs? I mean you can get preempted right after rcu_read_lock() before > acquiring the spinlock. I was thinking in terms of the fact that disabling irqs would block the grace period for the current implementation of RCU (but -not- SRCU, just for the record). You are right that the new version can be preempted just after the rcu_read_lock() but the same is true of the old pattern as well. To avoid this possibility of preemption, the code would need to look something like this: local_irq_disable(); rcu_read_lock(); spin_lock(timer->lock); ... spin_unlock(timer->lock); rcu_read_unlock(); local_irq_enable(); > > Your point about the hassle of finding and fixing all the other instances of > > this sort is well taken, however. > > Right. We have the pattern > > rcu_read_lock(); > x = lookup(); > if (x) > keep_hold(x) > rcu_read_unlock(); > return x; > > all over the place. Now that keep_hold() can be everything from a refcount to > a spinlock and I'm not sure that we can force stuff depending on the mechanism > to be completely symetric. So we are probably better off by making that rcu > unlock machinery more robust. OK. If I read the lockdep reports correctly, the issue occurs when rcu_read_unlock_special() finds that it needs to unboost, which means doing an rt_mutex_unlock(). This is done outside of rcu_read_unlock_special()'s irq-disabled region, but of course the caller might have disabled irqs. If I remember correctly, disabling irqs across rt_mutex_unlock() gets me lockdep splats. I could imagine having a per-CPU pointer to rt_mutex that rcu_read_unlock() sets, and that is checked at every point that irqs are enabled, with a call to rt_mutex_unlock() if that pointer is non-NULL. But perhaps you had something else in mind? Thanx, Paul