Messages in this thread | | | From | Thomas Gleixner <> | Subject | Re: [PATCH v5] locking/rwbase: Mitigate indefinite writer starvation | Date | Tue, 21 Feb 2023 00:55:33 +0100 |
| |
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
| |