From:   Thomas Gleixner <tglx@linutronix.de>
To:     Sebastian Andrzej Siewior <bigeasy@linutronix.de>,
        Mel Gorman <mgorman@techsingularity.net>
Cc:     Peter Zijlstra <peterz@infradead.org>,
        Ingo Molnar <mingo@kernel.org>,
        Davidlohr Bueso <dave@stgolabs.net>,
        Linux-RT <linux-rt-users@vger.kernel.org>,
        LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v5] locking/rwbase: Mitigate indefinite writer starvation
In-Reply-To: <Y+0W0wgyaJqYHKoj@linutronix.de>
References: <Y+0W0wgyaJqYHKoj@linutronix.de>
Date:   Tue, 21 Feb 2023 00:55:33 +0100
Message-ID: <877cwbq4cq.ffs@tglx>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

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