Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1199807 > unrolled thread

qrwlock && read-after-read

Back to article view | Back to linux.kernel

Contents

  qrwlock && read-after-read Oleg Nesterov <oleg@redhat.com> - 2015-08-04 15:10 +0200
    Re: qrwlock && read-after-read Peter Zijlstra <peterz@infradead.org> - 2015-08-04 15:20 +0200
      Re: qrwlock && read-after-read Oleg Nesterov <oleg@redhat.com> - 2015-08-04 15:50 +0200
        Re: qrwlock && read-after-read Peter Zijlstra <peterz@infradead.org> - 2015-08-04 19:40 +0200
      Re: qrwlock && read-after-read Linus Torvalds <torvalds@linux-foundation.org> - 2015-08-04 16:00 +0200
        Re: qrwlock && read-after-read Oleg Nesterov <oleg@redhat.com> - 2015-08-05 15:20 +0200

#1199807 — qrwlock && read-after-read

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.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1199819

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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1199846

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);
 }
 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1200178

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")
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1199858

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1200718

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.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web