Re: seq_lock and lockdep_is_held() assertions

From: John Stultz <john.stultz@linaro.org>
To: Jann Horn <jannh@google.com>
Cc: David Howells <dhowells@redhat.com>,
	Peter Zijlstra <peterz@infradead.org>,
	Ingo Molnar <mingo@redhat.com>, Will Deacon <will@kernel.org>,
	Al Viro <viro@zeniv.linux.org.uk>,
	raven@themaw.net, Miklos Szeredi <mszeredi@redhat.com>,
	Christian Brauner <christian@brauner.io>,
	Linux API <linux-api@vger.kernel.org>,
	linux-fsdevel <linux-fsdevel@vger.kernel.org>,
	kernel list <linux-kernel@vger.kernel.org>
Subject: Re: seq_lock and lockdep_is_held() assertions
Date: Fri, 21 Feb 2020 10:02:25 -0800	[thread overview]
Message-ID: <CALAqxLWxpkZMrJEHgYjtZnRtYtT7zY8=igKQ3rNPpqNV8FmLhg@mail.gmail.com> (raw)
In-Reply-To: <CAG48ez14CHMjZS8vCp6h6FnLvcFQq8oy_9JPCd=5qZ52X6w12Q@mail.gmail.com>

On Fri, Feb 21, 2020 at 9:36 AM Jann Horn <jannh@google.com> wrote:
>
> adding some locking folks to the thread...
>
> On Fri, Feb 21, 2020 at 6:06 PM David Howells <dhowells@redhat.com> wrote:
> > Jann Horn <jannh@google.com> wrote:
> > > On Fri, Feb 21, 2020 at 1:24 PM David Howells <dhowells@redhat.com> wrote:
> > > > What's the best way to write a lockdep assertion?
> > > >
> > > >         BUG_ON(!lockdep_is_held(lock));
> > >
> > > lockdep_assert_held(lock) is the normal way, I think - that will
> > > WARN() if lockdep is enabled and the lock is not held.
> >
> > Okay.  But what's the best way with a seqlock_t?  It has two dep maps in it.
> > Do I just ignore the one attached to the spinlock?
>
> Uuuh... very good question. Looking at how the seqlock_t helpers use
> the dep map of the seqlock, I don't think lockdep asserts work for
> asserting that you're in the read side of a seqlock?
>
> read_seqbegin_or_lock() -> read_seqbegin() -> read_seqcount_begin() ->
> seqcount_lockdep_reader_access() does seqcount_acquire_read() (which
> maps to lock_acquire_shared_recursive()), but immediately following
> that calls seqcount_release() (which maps to lock_release())?
>
> So I think lockdep won't consider you to be holding any locks after
> read_seqbegin_or_lock() if the lock wasn't taken?

Yea. It's a bit foggy now, but the main concern at the time was
wanting to catch seqlock readers that happened under a writer which
was a common cause of deadlocks between the timekeeping core and stuff
like printks (or anything we called out that might try to read the
time).

I think it was because writers can properly interrupt readers, we
couldn't hold the depmap across the read critical section? That's why
we just take and release the depmap, since that will still catch any
reads made while holding the write, which would deadlock.

But take that with a grain of salt, as its been awhile.

thanks
-john