Date:   Tue, 21 Mar 2023 17:11:40 +0100
From:   Sebastian Andrzej Siewior <bigeasy@linutronix.de>
To:     Thomas Gleixner <tglx@linutronix.de>
Cc:     Mel Gorman <mgorman@techsingularity.net>,
        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: [PATCH v6] locking/rwbase: Mitigate indefinite writer starvation.
Message-ID: <20230321161140.HMcQEhHb@linutronix.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Precedence: bulk

The rw_semaphore and rwlock_t locks are unfair to writers. Readers can
indefinitely acquire the lock unless the writer fully acquired the lock.
This can never happen if there is always a reader in the critical
section owning the lock.

Mel Gorman reported that since LTP-20220121 the dio_truncate test case
went from having 1 reader to having 16 reader and the number of readers
is sufficient to prevent the down_write ever succeeding while readers
exist. Eventually the test is killed after 30 minutes as a failure.

Mel proposed a timeout to limit how long a writer can be blocked until
the reader is forced into the slowpath.
Thomas argued that there is no added value by providing this timeout.
From PREEMPT_RT point of view, there are no critical rw_semaphore or
rwlock_t locks left where the reader must be prefer.

Mitigate indefinite writer starvation by forcing the READER into the
slowpath once the WRITER attempts to acquire the lock.

Reported-by: Mel Gorman <mgorman@techsingularity.net>
Link: https://lore.kernel.org/877cwbq4cq.ffs@tglx
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
- v5..v6: https://lore.kernel.org/all/Y+0W0wgyaJqYHKoj@linutronix.de/
  - dropped the timeout and forcing reader into the slowpath once a
    writer is waiting.

- v4..v5 https://lore.kernel.org/20230120140847.4pjqf3oinemokcyp@techsingularity.net
  - Reworded last paragraph of the commit message as Mel's suggestion
  - RT/DL tasks are no longer excluded from the waiter timeout. There
    is no reason why this should be done since no RT user relies on
    rwsem (and would need this kind of behaviour). The critical user
    from RT perspective replaced rwsem with RCU.
    Avoiding special treatment avoids this kind of bug with RT
    readers.
  - Update comments accordingly.

 kernel/locking/rwbase_rt.c | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/kernel/locking/rwbase_rt.c b/kernel/locking/rwbase_rt.c
index c201aadb93017..25ec0239477c2 100644
--- a/kernel/locking/rwbase_rt.c
+++ b/kernel/locking/rwbase_rt.c
@@ -72,15 +72,6 @@ static int __sched __rwbase_read_lock(struct rwbase_rt *rwb,
 	int ret;
 
 	raw_spin_lock_irq(&rtm->wait_lock);
-	/*
-	 * Allow readers, as long as the writer has not completely
-	 * acquired the semaphore for write.
-	 */
-	if (atomic_read(&rwb->readers) != WRITER_BIAS) {
-		atomic_inc(&rwb->readers);
-		raw_spin_unlock_irq(&rtm->wait_lock);
-		return 0;
-	}
 
 	/*
 	 * Call into the slow lock path with the rtmutex->wait_lock
-- 
2.40.0