On Wed, Aug 21, 2019 at 10:21 PM Rick Payne <ri...@rossfell.co.uk> wrote:
> On Wed, 2019-08-21 at 12:22 +0300, Nadav Har'El wrote:
> > I am guessing (need to verify...) that our rwlock implementation is
> > not recursive - a thread already holding the write lock needs to wait
> > (forever) for the read lock. If this is true, this is an rwlock bug.
>
> So I was puzzled a bit by the rwlock code. It handles recursive write
> lock acquisitions by incrementing a counter:
>
> // recursive write lock
> if (_wowner == sched::thread::current()) {
> _wrecurse++;
> }
>
Indeed - it appears that although write lock is recursive (the same thread
can get the write lock again), a thread holding the write lock cannot get a
read lock.
It's not completely obvious whether this is a feature, or a bug, though.
It's a bug in the sense it (a thread holding a write lock asking for a read
lock) causes a certain deadlock, which is always bad - it would have been
even better to assert() than just silently hang. But it's "feature" in the
sense that if the reader is expecting to see stable data, not in the middle
of someone reading it, he is bound to be disappointed because we are in the
middle of writing (even if on this thread).
I think it's clear here that we don't need to fix rwlock now: the problem
we have is why we got the page fault in the first place - not why it
deadlocked while handling it.
>
> On the unlock side though, it does decrement the counter but then goes
> on to wake a write_waiter - which seems wrong. Probably I am missing
> something - but why is the second part not inside the else clause where
> _wowner is set to nullptr?
>
You're right, it seems there's should be a "return" in the recursive case!
That being said, I think the spurious wakeup doesn't cause any harm,
because the wait code rwlock::writer_wait_lockable() loops, and if a thread
is woken while the lock is still taken, it just goes to sleep again. It
will just lose it's good spot on the queue :-(
>
> void rwlock::wunlock()
> {
> WITH_LOCK(_mtx) {
> assert(_wowner == sched::thread::current());
>
> if (_wrecurse > 0) {
> _wrecurse--;
> } else {
> _wowner = nullptr;
> }
>
> if (!_write_waiters.empty()) {
> _write_waiters.wake_one(_mtx);
> } else {
> _read_waiters.wake_all(_mtx);
> }
> }
> }
>
> I think you're right that if you hold the write lock and then try and
> readlock it will fail:
>
> bool rwlock::read_lockable()
> {
> return ((!_wowner) && (_write_waiters.empty()));
> }
>
> Rick
>
> --
> You received this message because you are subscribed to the Google Groups
> "OSv Development" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to osv-dev+unsubscr...@googlegroups.com.
> To view this discussion on the web visit
> https://groups.google.com/d/msgid/osv-dev/d75b970c54f9fe25cda79f2cc0aa0e7eaee3ec2f.camel%40rossfell.co.uk
> .
>
--
You received this message because you are subscribed to the Google Groups "OSv
Development" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to osv-dev+unsubscr...@googlegroups.com.
To view this discussion on the web visit
https://groups.google.com/d/msgid/osv-dev/CANEVyjttzni5VrgmFd08gVGfd-hDHANzgcB8dOK%2BZgCEcKJTmg%40mail.gmail.com.
