Re: [RFC 1/1] rwsem: Shrink rwsem by one pointer

[Matthew Wilcox]

On Wed, Feb 18, 2026 at 02:52:29PM -0800, Linus Torvalds wrote:
> On Wed, 18 Feb 2026 at 14:45, Linus Torvalds
> <torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
> >
> > Anyway, this is all from just looking at the patch, so maybe I missed
> > something, but it does look very wrong.
> 
> Bah. And immediately after sending, I went "maybe I should look at the
> code" more closely.
> 
> I think my suggestion to just remove the check was right, but the
> return value of rwsem_del_waiter() needs to be fixed to be the "I used
> to be the first waiter, but there are other waiters and I updated the
> first waiter pointer".

I tried to make this work, but it's a bit ugly.  rwsem_del_waiter()
needs to know whether there are remaining waiters, and
rwsem_del_wake_waiter() needs to know whether we deleted the first
waiter _and_ there are remaining ones.  So we end up returning a
tristate from __rwsem_del_waiter() and I find it less clear.

 static inline
-bool __rwsem_del_waiter(struct rw_semaphore *sem, struct rwsem_waiter *waiter)
+int __rwsem_del_waiter(struct rw_semaphore *sem, struct rwsem_waiter *waiter)
 {
+       int ret = 1;
+
        if (list_empty(&waiter->list)) {
                sem->first_waiter = NULL;
-               return true;
+               return 0;
        }

-       if (sem->first_waiter == waiter)
+       if (sem->first_waiter == waiter) {
                sem->first_waiter = list_first_entry(&waiter->list,
                                struct rwsem_waiter, list);
+               ret = 2;
+       }
        list_del(&waiter->list);

-       return false;
+       return ret;
 }
[...]
-static inline bool
-rwsem_del_waiter(struct rw_semaphore *sem, struct rwsem_waiter *waiter)
+static inline
+bool rwsem_del_waiter(struct rw_semaphore *sem, struct rwsem_waiter *waiter)
 {
+       int del_case;
+
        lockdep_assert_held(&sem->wait_lock);
-       if (__rwsem_del_waiter(sem, waiter))
-               return true;
-       atomic_long_andnot(RWSEM_FLAG_HANDOFF | RWSEM_FLAG_WAITERS, &sem->count);
-       return false;
+       del_case = __rwsem_del_waiter(sem, waiter);
+       if (del_case > 0)
+               atomic_long_andnot(RWSEM_FLAG_HANDOFF | RWSEM_FLAG_WAITERS,
+                               &sem->count);
+       return del_case == 2;
 }
[...]
 {
-       bool first = sem->first_waiter == waiter;
-
        wake_q_init(wake_q);

        /*
-        * If the wait_list isn't empty and the waiter to be deleted is
-        * the first waiter, we wake up the remaining waiters as they may
-        * be eligible to acquire or spin on the lock.
+        * If the deleted waiter was the first one and there are other
+        * waiters, we wake them up as they may be eligible to acquire
+        * or spin on the lock.
         */
-       if (rwsem_del_waiter(sem, waiter) && first)
+       if (rwsem_del_waiter(sem, waiter))
                rwsem_mark_wake(sem, RWSEM_WAKE_ANY, wake_q);


Even if we use a nice enum instead of 0/1/2 for the return value, I
don't think this is an improvement.  I played around with a couple of
other ways to refactor this and didn't come up with anything pretty.