On 17/03/21 11:40, David Edmondson wrote:
Isn't this...
* ... Also, @qemu_co_rwlock_upgrade
* only overrides CoRwlock fairness if there are no concurrent readers, so
* another writer might run while @qemu_co_rwlock_upgrade blocks.
...now incorrect?
Maybe, but for sure the comment was too hard to parse.
+ if (lock->owner == 1 && QSIMPLEQ_EMPTY(&lock->tickets)) {
+ lock->owner = -1;
+ qemu_co_mutex_unlock(&lock->mutex);
+ } else {
+ lock->owner--;
+ qemu_co_rwlock_sleep(false, lock);
Doesn't this need something for the case where lock->owner hits 0?
You're right, we need to call qemu_co_rwlock_maybe_wake_one if lock goes
to 0. The "else" branch would have to be
if (--lock->owner == 0) {
qemu_co_rwlock_maybe_wake_one(lock);
qemu_co_mutex_lock(&lock->mutex);
}
qemu_co_rwlock_sleep(false, lock);
In the end it's actually simpler to just inline qemu_co_rwlock_sleep,
which also leads to the following slightly more optimized code for the
"else" branch:
CoRwTicket my_ticket = { false, qemu_coroutine_self() };
lock->owner--;
QSIMPLEQ_INSERT_TAIL(&lock->tickets, &my_ticket, next);
qemu_co_rwlock_maybe_wake_one(lock);
qemu_coroutine_yield();
assert(lock->owner == -1);
I'll add a testcase, too. You don't even need two upgraders, for example:
rdlock
yield
wrlock
upgrade
<queued> <dequeued>
unlock
<dequeued>
unlock
In fact even for this simple case, the old implementation got it wrong!
(The new one also did, but the fix is easy).
There are a couple other improvements that can be made:
qemu_co_rwlock_unlock can also call qemu_co_rwlock_maybe_wake_one
unconditionally, the "if" around the call is unnecessary; and I'll
rename "owner" to "owners".
Anyway, there is nothing that really made you scream, so that's good.
Paolo
