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

["Lorenzo Stoakes (ARM)" <ljs@kernel.org>]

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