On Sun, May 08, 2022 at 10:54:01PM +0200, Alexandr Nedvedicky wrote:
> what bothers me is the situation where there are
> more than one reader. The line 350 is executed by
> the first reader which drops the lock. So the process
> woken up by wakeup(rwl) are going to find out the
> lock is still occupied by remaining readers.
wakeup() activates all sleepers. They should check and sleep again.
Maybe a little bit of wasted resources, but I don't see a severe
problem.
I did a little digging in history. In rev 1.3 of kern_rwlock.c
we had case RW_READ: need_wait = RWLOCK_WRLOCK | RWLOCK_WRWANT;
and the commet "Let writers through before obtaining read lock."
The comment
* RW_READ RWLOCK_WRLOCK|RWLOCK_WRWANT may not be set. We increment
* with RWLOCK_READ_INCR. RWLOCK_WAIT while waiting.
is still there, just the RWLOCK_WRWANT got lost from the condition.
So I think we should get back the original reader writer priority.
Regarding the race in rw_do_exit() that sashan@ found I also found
a comment in rev 1.7.
/*
* Potential MP race here. If the owner had WRWANT set we cleared
* it and a reader can sneak in before a writer. Do we care?
*/
I do not want to change anything to that behavior now. There is
no easy fix and I did not see the problem during testing. But we
can put the comment back to clarify the situation.
ok?
bluhm
Index: kern/kern_rwlock.c
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/kern/kern_rwlock.c,v
retrieving revision 1.47
diff -u -p -r1.47 kern_rwlock.c
--- kern/kern_rwlock.c 8 Feb 2021 08:18:45 -0000 1.47
+++ kern/kern_rwlock.c 9 May 2022 07:36:35 -0000
@@ -81,7 +81,7 @@ static const struct rwlock_op {
},
{ /* RW_READ */
RWLOCK_READ_INCR,
- RWLOCK_WRLOCK,
+ RWLOCK_WRLOCK | RWLOCK_WRWANT,
RWLOCK_WAIT,
0,
PLOCK
@@ -103,7 +103,7 @@ rw_enter_read(struct rwlock *rwl)
{
unsigned long owner = rwl->rwl_owner;
- if (__predict_false((owner & RWLOCK_WRLOCK) ||
+ if (__predict_false((owner & (RWLOCK_WRLOCK | RWLOCK_WRWANT)) ||
rw_cas(&rwl->rwl_owner, owner, owner + RWLOCK_READ_INCR)))
rw_enter(rwl, RW_READ);
else {
@@ -340,6 +340,11 @@ rw_do_exit(struct rwlock *rwl, unsigned
do {
owner = rwl->rwl_owner;
+ /*
+ * Potential MP race here. If the owner had WRWANT set we
+ * cleared it and a reader can sneak in before a writer.
+ * Do we care?
+ */
if (wrlock)
set = 0;
else
