qrwlock && read-after-read

qrwlock && read-after-read

[Oleg Nesterov]

I am working on the (off-topic) bug report which motivated me to
look at locking/qrwlock.c and it seems to me there is a problem
with the queued rwlocks.

Unless I am totally confused read-after-read is no longer valid,
write_lock() stops the new readers. And lockdep doesn't know this,
read_lock()->rwlock_acquire_read() doesn't match the reality. The
code doing

	read_lock(X);
	read_lock(X);

can deadlock if another CPU does write_lock(X) in between. This
was fine before rwlock_t was changed to use qrwlock.

A nested read_lock() in interrupt should be fine though, and this
is because queue_read_lock_slowpath() "ignores" _QW_WAITING if
in_interrupt().

This means that rwlock_t has the really strange semantics imho,
and again, it is not lockdep-friendly.

What do you think we can/should do? Or did I misread this code?

Oleg.

Re: qrwlock && read-after-read

[Peter Zijlstra]

On Tue, Aug 04, 2015 at 03:00:53PM +0200, Oleg Nesterov wrote:
> I am working on the (off-topic) bug report which motivated me to
> look at locking/qrwlock.c and it seems to me there is a problem
> with the queued rwlocks.
> 
> Unless I am totally confused read-after-read is no longer valid,
> write_lock() stops the new readers. And lockdep doesn't know this,
> read_lock()->rwlock_acquire_read() doesn't match the reality. The
> code doing
> 
> 	read_lock(X);
> 	read_lock(X);
> 
> can deadlock if another CPU does write_lock(X) in between. This
> was fine before rwlock_t was changed to use qrwlock.
> 
> A nested read_lock() in interrupt should be fine though, and this
> is because queue_read_lock_slowpath() "ignores" _QW_WAITING if
> in_interrupt().
> 
> This means that rwlock_t has the really strange semantics imho,
> and again, it is not lockdep-friendly.
> 
> What do you think we can/should do? Or did I misread this code?

Fix lockdep, although that's non trivial from what I remember.

These (new) semantics were very much on purpose and suggested by Linus
IIRC.

Re: qrwlock && read-after-read

[Oleg Nesterov]

On 08/04, Peter Zijlstra wrote:
>
> On Tue, Aug 04, 2015 at 03:00:53PM +0200, Oleg Nesterov wrote:
> > I am working on the (off-topic) bug report which motivated me to
> > look at locking/qrwlock.c and it seems to me there is a problem
> > with the queued rwlocks.
> >
> > Unless I am totally confused read-after-read is no longer valid,
> > write_lock() stops the new readers. And lockdep doesn't know this,
> > read_lock()->rwlock_acquire_read() doesn't match the reality. The
> > code doing
> >
> > 	read_lock(X);
> > 	read_lock(X);
> >
> > can deadlock if another CPU does write_lock(X) in between. This
> > was fine before rwlock_t was changed to use qrwlock.
> >
> > A nested read_lock() in interrupt should be fine though, and this
> > is because queue_read_lock_slowpath() "ignores" _QW_WAITING if
> > in_interrupt().
> >
> > This means that rwlock_t has the really strange semantics imho,
> > and again, it is not lockdep-friendly.
> >
> > What do you think we can/should do? Or did I misread this code?
>
> Fix lockdep, although that's non trivial from what I remember.
>
> These (new) semantics were very much on purpose and suggested by Linus
> IIRC.

Hmm, OK.

Lets fix the lockdep annotaions?

Oleg.

--- x/include/linux/rwlock_api_smp.h
+++ x/include/linux/rwlock_api_smp.h
@@ -146,7 +146,7 @@ static inline int __raw_write_trylock(rw
 static inline void __raw_read_lock(rwlock_t *lock)
 {
 	preempt_disable();
-	rwlock_acquire_read(&lock->dep_map, 0, 0, _RET_IP_);
+	lock_acquire(..., /* read */ in_interrupt() 2 : 1, ...);
 	LOCK_CONTENDED(lock, do_raw_read_trylock, do_raw_read_lock);
 }
 

Re: qrwlock && read-after-read

[Peter Zijlstra]

On Tue, Aug 04, 2015 at 03:40:43PM +0200, Oleg Nesterov wrote:
> Lets fix the lockdep annotaions?
> 
> Oleg.
> 
> --- x/include/linux/rwlock_api_smp.h
> +++ x/include/linux/rwlock_api_smp.h
> @@ -146,7 +146,7 @@ static inline int __raw_write_trylock(rw
>  static inline void __raw_read_lock(rwlock_t *lock)
>  {
>  	preempt_disable();
> -	rwlock_acquire_read(&lock->dep_map, 0, 0, _RET_IP_);
> +	lock_acquire(..., /* read */ in_interrupt() 2 : 1, ...);
>  	LOCK_CONTENDED(lock, do_raw_read_trylock, do_raw_read_lock);
>  }

I think that suffers the same problems we had before; see:

  8acd91e86208 ("locking/lockdep: Revert qrwlock recusive stuff")

Re: qrwlock && read-after-read

[Linus Torvalds]

On Tue, Aug 4, 2015 at 6:10 AM, Peter Zijlstra <peterz@infradead.org> wrote:
>
> These (new) semantics were very much on purpose and suggested by Linus
> IIRC.

Well, I'll not take all the blame.

I refused to have something that broke the tasklist lock, so the "irq
users nest" was a requirement.

I also refused to have the original version that made this a per-lock
explicit and confusing choice, which in turn required changes to
existing users, and that made the interface more complex in ways that
didn't actually help anybody.

So it's not like I love the current semantics, but at least they are
realistic and can work. I agree that teaching lockdep to check for
this would be a good idea, because the semantics _are_ subtle.

(And I'm not 100% convinced we needed the fair model at all, but
fairness does end up being a good thing _if_ it works).

                   Linus

Re: qrwlock && read-after-read

[Oleg Nesterov]

On 08/04, Linus Torvalds wrote:
>
> I refused to have something that broke the tasklist lock, so the "irq
> users nest" was a requirement.

And I was going to reply that this breaks tasklist lock anyway but
failed to find anything wrong after the quick grep.

> So it's not like I love the current semantics, but at least they are
> realistic and can work. I agree that teaching lockdep to check for
> this would be a good idea, because the semantics _are_ subtle.

Yes... Just for example, the comment above task_lock(),

	Nests both inside and outside of read_lock(&tasklist_lock).

is no longer correct. Fortunately task_lock() is not irq-safe, and
iirc nobody does task_lock() + read_lock(&tasklist_lock) in process
context, so we are probably fine. Still, qrwlock changed the rules
and now it can only nest inside of read_lock(tasklist_lock).

Hmm. And afaics this in turn means that the next sentence

	It must not be nested with write_lock_irq(&tasklist_lock),
	neither inside nor outside.

also becomes wrong. So task_lock() can nest inside tasklist_lock,
write-or-read doesn't matter.


So it would be really nice to fix lockdep, but as Peter explains
(thanks Peter!) this is not simple.


> (And I'm not 100% convinced we needed the fair model at all, but
> fairness does end up being a good thing _if_ it works).

Yes. At least this automatically fixes the easy-to-trigger problems
with write_lock(tasklist) starvation/lockup.

Oleg.