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]

Message-ID: <877cwbq4cq.ffs@tglx>
Date:   Tue, 21 Feb 2023 00:55:33 +0100
From:   Thomas Gleixner <tglx@...utronix.de>
To:     Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
        Mel Gorman <mgorman@...hsingularity.net>
Cc:     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 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