linux-kernel - Re: [PATCH v5] locking/rwbase: Mitigate indefinite writer starvation

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 21 Feb 2023 09:47:20 +0100
From:   Sebastian Andrzej Siewior <bigeasy@...utronix.de>
To:     Thomas Gleixner <tglx@...utronix.de>
Cc:     Mel Gorman <mgorman@...hsingularity.net>,
        Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...nel.org>,
        Davidlohr Bueso <dave@...olabs.net>,
        Linux-RT <linux-rt-users@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v5] locking/rwbase: Mitigate indefinite writer starvation

On 2023-02-21 00:55:33 [+0100], Thomas Gleixner wrote:
> 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?

Once the writer got the lock, then all further reader and writer queue
up on rtmutex and will be processed one after the other. It only becomes
a problem once a writer with elevated priority acquires the lock
repeatedly to the point that tasks with lower priority starve.

> Can we finally get a proper justification for this?

Avoid writer starvation caused by having at least one reader in the
critical section all the time blocking the writer to make any progress.
Reader starvation is not an issue because all tasks line up on the
rtmutex and will be processed in FIFO order.
Tasks with elevated priority will be preferred and can lead starvation
of the tasks with lower priority. This is by design and can happen with
other lock types, too.

> > @@ -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:

Sure. Let me look at this once we agreed on the justification.

…
> Thanks,
> 
>         tglx
Sebastian