[PATCH] locking/rwsem: don't let a read lock steal from a queued writer

[PATCH] locking/rwsem: don't let a read lock steal from a queued writer

[Lorenzo Stoakes (ARM)]

Since commit 1a728dff855a ("locking/rwsem: Enable reader optimistic lock
stealing"), a racing read lock can steal an rwsem from under a queued
writer whose handoff timeout hasn't expired yet.

This is most problematic when an rwsem read lock is released then
reacquired in order to relieve lock contention, as instead it will simply
have the lock handed right back to it ahead of the intended beneficiary.

Naturally, this behaviour rather defeats the object of doing this kind of
thing, so this is less than ideal.

Fix this by updating the optimistic read lock stealing code in
rwsem_down_read_slowpath() to explicitly check whether the first queued
lock is a waiting writer, and to not allow the theft if so.

When a steal is disallowed, keep the existing sem->wait_lock held when
jumping to the queue code to avoid useless lock acquisition/release.

In order to perform this check, sem->wait_lock is acquired whenever there
are queued waiters in the steal code, rather than the previous behaviour of
only doing so when the reader count is 1.

Since failing to steal a read lock unconditionally results in acquisition
of the sem->wait_lock, this doesn't seem likely to be a significant
overhead.

Also update the comment describing optimistic read lock acquisition
conditions to include this newly introduced behaviour.

Care is taken to ensure correct ordering of lock acquisition, lock waking
and marking the lock reader-owned.

Signed-off-by: Lorenzo Stoakes (ARM) <ljs@kernel.org>
---
 kernel/locking/rwsem.c | 30 ++++++++++++++++++------------
 1 file changed, 18 insertions(+), 12 deletions(-)

diff --git a/kernel/locking/rwsem.c b/kernel/locking/rwsem.c
index b9c180ac1eee..cdb3dd6260bf 100644
--- a/kernel/locking/rwsem.c
+++ b/kernel/locking/rwsem.c
@@ -59,7 +59,8 @@
  * A fast path reader optimistic lock stealing is supported when the rwsem
  * is previously owned by a writer and the following conditions are met:
  *  - rwsem is not currently writer owned
- *  - the handoff isn't set.
+ *  - the handoff isn't set
+ *  - a writer isn't at the head of the wait queue.
  */
 #define RWSEM_READER_OWNED	(1UL << 0)
 #define RWSEM_NONSPINNABLE	(1UL << 1)
@@ -1034,31 +1035,36 @@ rwsem_down_read_slowpath(struct rw_semaphore *sem, long count, unsigned int stat
 	 * Reader optimistic lock stealing.
 	 */
 	if (!(count & (RWSEM_WRITER_LOCKED | RWSEM_FLAG_HANDOFF))) {
-		rwsem_set_reader_owned(sem);
-		lockevent_inc(rwsem_rlock_steal);
-
-		/*
-		 * Wake up other readers in the wait queue if it is
-		 * the first reader.
-		 */
-		if ((rcnt == 1) && (count & RWSEM_FLAG_WAITERS)) {
+		if (count & RWSEM_FLAG_WAITERS) {
 			raw_spin_lock_irq(&sem->wait_lock);
-			if (sem->first_waiter)
+			first = sem->first_waiter;
+			/* A writer at the head of queue takes precedence. */
+			if (first && first->type == RWSEM_WAITING_FOR_WRITE)
+				goto queue_locked;
+			rwsem_set_reader_owned(sem);
+			/* If first reader, wake other readers in queue. */
+			if (first && rcnt == 1)
 				rwsem_mark_wake(sem, RWSEM_WAKE_READ_OWNED,
 						&wake_q);
 			raw_spin_unlock_irq(&sem->wait_lock);
-			wake_up_q(&wake_q);
+		} else {
+			rwsem_set_reader_owned(sem);
 		}
+
+		lockevent_inc(rwsem_rlock_steal);
+		if (!wake_q_empty(&wake_q))
+			wake_up_q(&wake_q);
 		return sem;
 	}
 
 queue:
+	raw_spin_lock_irq(&sem->wait_lock);
+queue_locked:
 	waiter.task = current;
 	waiter.type = RWSEM_WAITING_FOR_READ;
 	waiter.timeout = jiffies + RWSEM_WAIT_TIMEOUT;
 	waiter.handoff_set = false;
 
-	raw_spin_lock_irq(&sem->wait_lock);
 	first = sem->first_waiter;
 	if (!first) {
 		/*

---
base-commit: 05441d697f7f9a927264f4058cbbc6a03dfcac9d
change-id: 20261105-rwsem-dont-steal-from-queued-writer-04b11d9d406b

Cheers,
-- 
Lorenzo Stoakes (ARM) <ljs@kernel.org>

Re: [PATCH] locking/rwsem: don't let a read lock steal from a queued writer

[Waiman Long]

On 11/5/26 3:24 PM, Lorenzo Stoakes (ARM) wrote:
> Since commit 1a728dff855a ("locking/rwsem: Enable reader optimistic lock
> stealing"), a racing read lock can steal an rwsem from under a queued
> writer whose handoff timeout hasn't expired yet.
>
> This is most problematic when an rwsem read lock is released then
> reacquired in order to relieve lock contention, as instead it will simply
> have the lock handed right back to it ahead of the intended beneficiary.
>
> Naturally, this behaviour rather defeats the object of doing this kind of
> thing, so this is less than ideal.

If there is only one reader doing this release and then acquire 
operation, it will leave its task_struct value in the owner field. So we 
can check if the owner field  to determine if we should skip the lock 
stealing operation.

Something like

diff --git a/kernel/locking/rwsem.c b/kernel/locking/rwsem.c
index b9c180ac1eee..1f0fffd75c18 100644
--- a/kernel/locking/rwsem.c
+++ b/kernel/locking/rwsem.c
@@ -1019,15 +1019,17 @@ rwsem_down_read_slowpath(struct rw_semaphore 
*sem, long count, unsigned int stat
         long adjustment = -RWSEM_READER_BIAS;
         long rcnt = (count >> RWSEM_READER_SHIFT);
         struct rwsem_waiter waiter, *first;
+       long owner = atomic_long_read(&sem->owner);
         DEFINE_WAKE_Q(wake_q);

         /*
          * To prevent a constant stream of readers from starving a sleeping
          * writer, don't attempt optimistic lock stealing if the lock is
-        * very likely owned by readers.
+        * very likely owned by readers or when the current task is the
+        * previous owning reader.
          */
-       if ((atomic_long_read(&sem->owner) & RWSEM_READER_OWNED) &&
-           (rcnt > 1) && !(count & RWSEM_WRITER_LOCKED))
+       if ((owner & RWSEM_READER_OWNED) && ((rcnt > 1) ||
+           (current == (struct task_struct *)(owner & 
~RWSEM_OWNER_FLAGS_MASK))))
                 goto queue;

         /*

------------------------------------------------------------------------------------------------------------------------

Will this help?

Cheers,
Longman

Re: [PATCH] locking/rwsem: don't let a read lock steal from a queued writer

[Lorenzo Stoakes (ARM)]

On Thu, Nov 05, 2026 at 11:55:37PM -0500, Waiman Long wrote:
>
> On 11/5/26 3:24 PM, Lorenzo Stoakes (ARM) wrote:
> > Since commit 1a728dff855a ("locking/rwsem: Enable reader optimistic lock
> > stealing"), a racing read lock can steal an rwsem from under a queued
> > writer whose handoff timeout hasn't expired yet.
> >
> > This is most problematic when an rwsem read lock is released then
> > reacquired in order to relieve lock contention, as instead it will simply
> > have the lock handed right back to it ahead of the intended beneficiary.
> >
> > Naturally, this behaviour rather defeats the object of doing this kind of
> > thing, so this is less than ideal.
>
> If there is only one reader doing this release and then acquire operation,
> it will leave its task_struct value in the owner field. So we can check if
> the owner field  to determine if we should skip the lock stealing operation.

That seems like it's overfitted the problem though? And would cede to read locks
also, no?

The intent is to prevent starvation of write locks, which seems like a
worthwhile goal not only in the case of dropping an rwsem to relieve contention.

>
> Something like
>
> diff --git a/kernel/locking/rwsem.c b/kernel/locking/rwsem.c
> index b9c180ac1eee..1f0fffd75c18 100644
> --- a/kernel/locking/rwsem.c
> +++ b/kernel/locking/rwsem.c
> @@ -1019,15 +1019,17 @@ rwsem_down_read_slowpath(struct rw_semaphore *sem,
> long count, unsigned int stat
>         long adjustment = -RWSEM_READER_BIAS;
>         long rcnt = (count >> RWSEM_READER_SHIFT);
>         struct rwsem_waiter waiter, *first;
> +       long owner = atomic_long_read(&sem->owner);
>         DEFINE_WAKE_Q(wake_q);
>
>         /*
>          * To prevent a constant stream of readers from starving a sleeping
>          * writer, don't attempt optimistic lock stealing if the lock is
> -        * very likely owned by readers.
> +        * very likely owned by readers or when the current task is the
> +        * previous owning reader.
>          */
> -       if ((atomic_long_read(&sem->owner) & RWSEM_READER_OWNED) &&
> -           (rcnt > 1) && !(count & RWSEM_WRITER_LOCKED))
> +       if ((owner & RWSEM_READER_OWNED) && ((rcnt > 1) ||
> +           (current == (struct task_struct *)(owner &
> ~RWSEM_OWNER_FLAGS_MASK))))

Actually the RWSEM_WRITER_LOCKED here is redundant given the block below, I
did think about removing it...

And could probably avoid that open-coded expression with
rwsem_owner_flags() I think?

Anyway, it seems this code would be redundant with CONFIG_DEBUG_RWSEMS or
CONFIG_DETECT_HUNG_TASK_BLOCKER, both of which implement code for
rwsem_clear_reader_owned() which is called when a read lock is dropped.

That would mean any kernel with those set would not get the required
behaviour, which is unexpected behaviour, so I think this is out anyway.

>                 goto queue;
>
>         /*
>
> ------------------------------------------------------------------------------------------------------------------------
>
> Will this help?

I think in general the original patch is more useful in that it expressly
prefers a first-in-the-queue writer to an incoming reader.

>
> Cheers,
> Longman
>

--
Cheers, Lorenzo