On Wed, Feb 15 2023 at 18:30, Sebastian Andrzej Siewior wrote:
> diff --git a/include/linux/rwbase_rt.h b/include/linux/rwbase_rt.h
> index 1d264dd086250..b969b1d9bb85c 100644
> --- a/include/linux/rwbase_rt.h
> +++ b/include/linux/rwbase_rt.h
> @@ -10,12 +10,14 @@
>
> struct rwbase_rt {
> atomic_t readers;
> + unsigned long waiter_timeout;
I'm still not convinced that this timeout has any value and if it has
then it should be clearly named writer_timeout because that's what it is
about.
The only reason for this timeout I saw so far is:
> +/*
> + * Allow reader bias with a pending writer for a minimum of 4ms or 1 tick. This
> + * matches RWSEM_WAIT_TIMEOUT for the generic RWSEM implementation.
Clearly RT and !RT have completely different implementations and
behaviour vs. rwsems and rwlocks. Just because !RT has a timeout does
not make a good argument.
Just for the record: !RT has the timeout applicable in both directions
to prevent writer bias via lock stealing. That's not a problem for RT
because?
Can we finally get a proper justification for this?
> @@ -264,12 +285,20 @@ static int __sched rwbase_write_lock(struct rwbase_rt *rwb,
> if (__rwbase_write_trylock(rwb))
> break;
>
> + /*
> + * Record timeout when reader bias is ignored. Ensure timeout
> + * is at least 1 in case of overflow.
> + */
> + rwb->waiter_timeout = (jiffies + RWBASE_RT_WAIT_TIMEOUT) | 1;
> +
So this has two sillies:
1) It resets the timeout once per loop which is plain wrong
2) The "| 1" is really a sloppy hack
Why not doing the obvious:
static bool __sched rwbase_allow_reader_bias(struct rwbase_rt *rwb)
{
int r = atomic_read(&rwb->readers);
if (likely(r < 0))
return true;
if (r == WRITER_BIAS)
return false;
/* Allow reader bias unless the writer timeout has expired. */
return time_before(jiffies, rwb->writer_timeout);
}
and with that the "| 1" and all the rwb->timeout = 0 nonsense simply
goes away and rwbase_read_lock() becomes:
if (rwbase_allow_reader_bias(rwb)) {
// fastpath
atomic_inc(&rwb->readers);
raw_spin_unlock_irq(&rtm->wait_lock);
return 0;
}
// slowpath
and the writelock slowpath has:
rwb->writer_timeout = jiffies + RWBASE_RT_WAIT_TIMEOUT;
for (;;) {
....
No?
Thanks,
tglx
