On Fri, Feb 21, 2020 at 9:36 AM Jann Horn [off-list ref] wrote:

adding some locking folks to the thread...

On Fri, Feb 21, 2020 at 6:06 PM David Howells [off-list ref] wrote:

quoted

Jann Horn [off-list ref] wrote:

quoted

On Fri, Feb 21, 2020 at 1:24 PM David Howells [off-list ref] wrote:

quoted

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