[RFC 0/1] Shrinking rwsem

[RFC 0/1] Shrinking rwsem

[Matthew Wilcox (Oracle)]

Some of Christoph's recent work has pointed out that we're achingly
close to being able to squeeze one more inode into each slab [1].
There are currently three rwsems embedded in struct inode --
invalidate_lock, i_mmap_rwsem and i_rwsem, so this saves us 24 bytes.
Of course, it'll shrink a lot of other data structures that embed
an rwsem (anon_vma and mm_struct come to mind).

We can do the same trick to struct mutex, but I thought I'd send this
out to see how people feel about the extra code complexity to make these
savings before investing any effort in doing that.

Maybe the performance bots will come back with some numbers, although
they've been weirdly sensitive to rwsem alignment in the past, so I'm
not sure I'd believe their numbers.

[1] https://lore.kernel.org/linux-fsdevel/20260202060754.270269-1-hch@lst.de/

Matthew Wilcox (Oracle) (1):
  rwsem: Shrink rwsem by one pointer

 include/linux/rwsem.h  |  8 ++---
 kernel/locking/rwsem.c | 74 +++++++++++++++++++++++++++++++-----------
 2 files changed, 59 insertions(+), 23 deletions(-)

-- 
2.47.3

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

[Matthew Wilcox (Oracle)]

Instead of embedding a list_head in struct rw_semaphore, store a pointer
to the first waiter.  The list of waiters remains a doubly linked list
so we can efficiently add to the tail of the list, remove from the front
(or middle) of the list.

Some of the list manipulation becomes more complicated, but it's a
reasonable tradeoff on the slow paths to shrink some core data structures
like struct inode.

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 include/linux/rwsem.h  |  8 ++---
 kernel/locking/rwsem.c | 74 +++++++++++++++++++++++++++++++-----------
 2 files changed, 59 insertions(+), 23 deletions(-)

diff --git a/include/linux/rwsem.h b/include/linux/rwsem.h
index f1aaf676a874..1771c96a01d2 100644
--- a/include/linux/rwsem.h
+++ b/include/linux/rwsem.h
@@ -57,7 +57,7 @@ struct rw_semaphore {
 	struct optimistic_spin_queue osq; /* spinner MCS lock */
 #endif
 	raw_spinlock_t wait_lock;
-	struct list_head wait_list;
+	struct rwsem_waiter *first_waiter;
 #ifdef CONFIG_DEBUG_RWSEMS
 	void *magic;
 #endif
@@ -104,7 +104,7 @@ static inline void rwsem_assert_held_write_nolockdep(const struct rw_semaphore *
 	  .owner = ATOMIC_LONG_INIT(0),				\
 	  __RWSEM_OPT_INIT(name)				\
 	  .wait_lock = __RAW_SPIN_LOCK_UNLOCKED(name.wait_lock),\
-	  .wait_list = LIST_HEAD_INIT((name).wait_list),	\
+	  .first_waiter = NULL,					\
 	  __RWSEM_DEBUG_INIT(name)				\
 	  __RWSEM_DEP_MAP_INIT(name) }
 
@@ -127,9 +127,9 @@ do {								\
  * rwsem to see if somebody from an incompatible type is wanting access to the
  * lock.
  */
-static inline int rwsem_is_contended(struct rw_semaphore *sem)
+static inline bool rwsem_is_contended(struct rw_semaphore *sem)
 {
-	return !list_empty(&sem->wait_list);
+	return sem->first_waiter != NULL;
 }
 
 #if defined(CONFIG_DEBUG_RWSEMS) || defined(CONFIG_DETECT_HUNG_TASK_BLOCKER)
diff --git a/kernel/locking/rwsem.c b/kernel/locking/rwsem.c
index 24df4d98f7d2..4226eb0ec5da 100644
--- a/kernel/locking/rwsem.c
+++ b/kernel/locking/rwsem.c
@@ -72,7 +72,7 @@
 		#c, atomic_long_read(&(sem)->count),		\
 		(unsigned long) sem->magic,			\
 		atomic_long_read(&(sem)->owner), (long)current,	\
-		list_empty(&(sem)->wait_list) ? "" : "not "))	\
+		(sem)->first_waiter ? "" : "not "))		\
 			debug_locks_off();			\
 	} while (0)
 #else
@@ -321,7 +321,7 @@ void __init_rwsem(struct rw_semaphore *sem, const char *name,
 #endif
 	atomic_long_set(&sem->count, RWSEM_UNLOCKED_VALUE);
 	raw_spin_lock_init(&sem->wait_lock);
-	INIT_LIST_HEAD(&sem->wait_list);
+	sem->first_waiter = NULL;
 	atomic_long_set(&sem->owner, 0L);
 #ifdef CONFIG_RWSEM_SPIN_ON_OWNER
 	osq_lock_init(&sem->osq);
@@ -341,8 +341,7 @@ struct rwsem_waiter {
 	unsigned long timeout;
 	bool handoff_set;
 };
-#define rwsem_first_waiter(sem) \
-	list_first_entry(&sem->wait_list, struct rwsem_waiter, list)
+#define rwsem_first_waiter(sem)	sem->first_waiter
 
 enum rwsem_wake_type {
 	RWSEM_WAKE_ANY,		/* Wake whatever's at head of wait list */
@@ -368,11 +367,36 @@ enum rwsem_wake_type {
 static inline void
 rwsem_add_waiter(struct rw_semaphore *sem, struct rwsem_waiter *waiter)
 {
+	struct rwsem_waiter *first = sem->first_waiter;
 	lockdep_assert_held(&sem->wait_lock);
-	list_add_tail(&waiter->list, &sem->wait_list);
+	if (first) {
+		list_add_tail(&waiter->list, &first->list);
+	} else {
+		INIT_LIST_HEAD(&waiter->list);
+		sem->first_waiter = waiter;
+	}
 	/* caller will set RWSEM_FLAG_WAITERS */
 }
 
+static inline
+bool __rwsem_del_waiter(struct rw_semaphore *sem, struct rwsem_waiter *waiter)
+{
+	if (sem->first_waiter == waiter) {
+		if (list_empty(&waiter->list)) {
+			sem->first_waiter = NULL;
+			return true;
+		} else {
+			sem->first_waiter = list_first_entry(&waiter->list,
+					struct rwsem_waiter, list);
+			list_del(&waiter->list);
+		}
+	} else {
+		list_del(&waiter->list);
+	}
+
+	return false;
+}
+
 /*
  * Remove a waiter from the wait_list and clear flags.
  *
@@ -385,14 +409,22 @@ static inline bool
 rwsem_del_waiter(struct rw_semaphore *sem, struct rwsem_waiter *waiter)
 {
 	lockdep_assert_held(&sem->wait_lock);
-	list_del(&waiter->list);
-	if (likely(!list_empty(&sem->wait_list)))
+	if (__rwsem_del_waiter(sem, waiter))
 		return true;
-
 	atomic_long_andnot(RWSEM_FLAG_HANDOFF | RWSEM_FLAG_WAITERS, &sem->count);
 	return false;
 }
 
+static inline struct rwsem_waiter *next_waiter(const struct rw_semaphore *sem,
+		const struct rwsem_waiter *waiter)
+{
+	struct rwsem_waiter *next = list_first_entry(&waiter->list,
+			struct rwsem_waiter, list);
+	if (next == sem->first_waiter)
+		return NULL;
+	return next;
+}
+
 /*
  * handle the lock release when processes blocked on it that can now run
  * - if we come here from up_xxxx(), then the RWSEM_FLAG_WAITERS bit must
@@ -411,7 +443,7 @@ static void rwsem_mark_wake(struct rw_semaphore *sem,
 			    enum rwsem_wake_type wake_type,
 			    struct wake_q_head *wake_q)
 {
-	struct rwsem_waiter *waiter, *tmp;
+	struct rwsem_waiter *waiter, *next;
 	long oldcount, woken = 0, adjustment = 0;
 	struct list_head wlist;
 
@@ -506,25 +538,28 @@ static void rwsem_mark_wake(struct rw_semaphore *sem,
 	 *    put them into wake_q to be woken up later.
 	 */
 	INIT_LIST_HEAD(&wlist);
-	list_for_each_entry_safe(waiter, tmp, &sem->wait_list, list) {
+	do {
+		next = next_waiter(sem, waiter);
 		if (waiter->type == RWSEM_WAITING_FOR_WRITE)
 			continue;
 
 		woken++;
 		list_move_tail(&waiter->list, &wlist);
+		if (sem->first_waiter == waiter)
+			sem->first_waiter = next;
 
 		/*
 		 * Limit # of readers that can be woken up per wakeup call.
 		 */
 		if (unlikely(woken >= MAX_READERS_WAKEUP))
 			break;
-	}
+	} while ((waiter = next) != NULL);
 
 	adjustment = woken * RWSEM_READER_BIAS - adjustment;
 	lockevent_cond_inc(rwsem_wake_reader, woken);
 
 	oldcount = atomic_long_read(&sem->count);
-	if (list_empty(&sem->wait_list)) {
+	if (!sem->first_waiter) {
 		/*
 		 * Combined with list_move_tail() above, this implies
 		 * rwsem_del_waiter().
@@ -545,7 +580,7 @@ static void rwsem_mark_wake(struct rw_semaphore *sem,
 		atomic_long_add(adjustment, &sem->count);
 
 	/* 2nd pass */
-	list_for_each_entry_safe(waiter, tmp, &wlist, list) {
+	list_for_each_entry_safe(waiter, next, &wlist, list) {
 		struct task_struct *tsk;
 
 		tsk = waiter->task;
@@ -639,7 +674,7 @@ static inline bool rwsem_try_write_lock(struct rw_semaphore *sem,
 			new |= RWSEM_WRITER_LOCKED;
 			new &= ~RWSEM_FLAG_HANDOFF;
 
-			if (list_is_singular(&sem->wait_list))
+			if (list_empty(&first->list))
 				new &= ~RWSEM_FLAG_WAITERS;
 		}
 	} while (!atomic_long_try_cmpxchg_acquire(&sem->count, &count, new));
@@ -659,7 +694,8 @@ static inline bool rwsem_try_write_lock(struct rw_semaphore *sem,
 	 * Have rwsem_try_write_lock() fully imply rwsem_del_waiter() on
 	 * success.
 	 */
-	list_del(&waiter->list);
+	__rwsem_del_waiter(sem, waiter);
+
 	rwsem_set_owner(sem);
 	return true;
 }
@@ -1019,7 +1055,7 @@ rwsem_down_read_slowpath(struct rw_semaphore *sem, long count, unsigned int stat
 		 */
 		if ((rcnt == 1) && (count & RWSEM_FLAG_WAITERS)) {
 			raw_spin_lock_irq(&sem->wait_lock);
-			if (!list_empty(&sem->wait_list))
+			if (sem->first_waiter)
 				rwsem_mark_wake(sem, RWSEM_WAKE_READ_OWNED,
 						&wake_q);
 			raw_spin_unlock_irq(&sem->wait_lock);
@@ -1035,7 +1071,7 @@ rwsem_down_read_slowpath(struct rw_semaphore *sem, long count, unsigned int stat
 	waiter.handoff_set = false;
 
 	raw_spin_lock_irq(&sem->wait_lock);
-	if (list_empty(&sem->wait_list)) {
+	if (!sem->first_waiter) {
 		/*
 		 * In case the wait queue is empty and the lock isn't owned
 		 * by a writer, this reader can exit the slowpath and return
@@ -1218,7 +1254,7 @@ static struct rw_semaphore *rwsem_wake(struct rw_semaphore *sem)
 
 	raw_spin_lock_irqsave(&sem->wait_lock, flags);
 
-	if (!list_empty(&sem->wait_list))
+	if (sem->first_waiter)
 		rwsem_mark_wake(sem, RWSEM_WAKE_ANY, &wake_q);
 
 	raw_spin_unlock_irqrestore(&sem->wait_lock, flags);
@@ -1239,7 +1275,7 @@ static struct rw_semaphore *rwsem_downgrade_wake(struct rw_semaphore *sem)
 
 	raw_spin_lock_irqsave(&sem->wait_lock, flags);
 
-	if (!list_empty(&sem->wait_list))
+	if (sem->first_waiter)
 		rwsem_mark_wake(sem, RWSEM_WAKE_READ_OWNED, &wake_q);
 
 	raw_spin_unlock_irqrestore(&sem->wait_lock, flags);
-- 
2.47.3

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

[Linus Torvalds]

On Tue, 17 Feb 2026 at 11:08, Matthew Wilcox (Oracle)
<willy@infradead.org> wrote:
>
> Instead of embedding a list_head in struct rw_semaphore, store a pointer
> to the first waiter.  The list of waiters remains a doubly linked list
> so we can efficiently add to the tail of the list, remove from the front
> (or middle) of the list.
>
> Some of the list manipulation becomes more complicated, but it's a
> reasonable tradeoff on the slow paths to shrink some core data structures
> like struct inode.

I like this, but I have to say that I dislike how rwsem_add_waiter()
in particular ends up looking.

Not because it's horrible on its own, but when you look at the
call-sites, that function ends up being entirely pointless.

It does two things:

 - it asserts that the wait_lock is held

 - it now checks whether the new waiter is the first one and does two
different things depending on that

And lookie here: there are exactly two call sites, and both of them
are immediately preceded by a

        raw_spin_lock_irq(&sem->wait_lock);

and one of them then goes on to check whether there are no waiters
before it calls rwsem_add_waiter().

And the other? Immediately *after* the call, it does

        /* we're now waiting on the lock */
        if (rwsem_first_waiter(sem) != &waiter) {

so that other call-site *ALSO* basically ends up having special code
for "am I the first writer"?

Put another way: this helper function seems all kinds of pointless and
it got worse in this iteration, because the pointlessness is now in
some of that added complexity.

So considering that there iareonly two callers. and both of them
*already* fundamentally know about this whole "first waiter is
special" situation, that helper is actually the opposite of a helper.
It's actually hiding what is going on, and just making code generation
worse.

If we want helper functions, can we please just make them be
"add_first_waiter()" and "add_to_waiter_list()", and make them
actually be what the callers need and want?

Somewhat similarly, I also reacted to this part:

    -#define rwsem_first_waiter(sem) \
    -       list_first_entry(&sem->wait_list, struct rwsem_waiter, list)
    +#define rwsem_first_waiter(sem)        sem->first_waiter

that rwsem_first_waiter() macro used to make sense as a syntactic
helper function. But now it really doesn't. It is literally more
typing and *less* legible than just accessing that new
"sem->first_waiter" field.

In other words, I like the direction you're taking, and I think the
code already is set up to have that whole "first waiter" thing that
your patch just makes much more explicit and obvious. That all argues
that yes, having a "first waiter" pointer instead of a "struct
list_head" is a good change.

But I think some of those existing helpers were because we did *not*
use to do it that better way, and they were designed for the old world
order, and they no longer make any sense with your changes.

Hmm?

               Linus

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

[Matthew Wilcox]

On Tue, Feb 17, 2026 at 12:27:29PM -0800, Linus Torvalds wrote:
> I like this, but I have to say that I dislike how rwsem_add_waiter()
> in particular ends up looking.
> 
> Not because it's horrible on its own, but when you look at the
> call-sites, that function ends up being entirely pointless.

I confess, I didn't look at the callers.  Good catch; I've integrated
your suggestion and it looks better.

I was most concerned with just how ugly __rwsem_del_waiter() looked.
I had a good think about it and have an improved version.

> Somewhat similarly, I also reacted to this part:
> 
>     -#define rwsem_first_waiter(sem) \
>     -       list_first_entry(&sem->wait_list, struct rwsem_waiter, list)
>     +#define rwsem_first_waiter(sem)        sem->first_waiter
> 
> that rwsem_first_waiter() macro used to make sense as a syntactic
> helper function. But now it really doesn't. It is literally more
> typing and *less* legible than just accessing that new
> "sem->first_waiter" field.

Yep, I did notice that too, just decided not to fix it.  Also taken
care of in the next version.

Here's all the changes I made, and I'll post a rolled-up version
next.

diff --git a/kernel/locking/rwsem.c b/kernel/locking/rwsem.c
index 4226eb0ec5da..16f3db35652a 100644
--- a/kernel/locking/rwsem.c
+++ b/kernel/locking/rwsem.c
@@ -341,7 +341,6 @@ struct rwsem_waiter {
 	unsigned long timeout;
 	bool handoff_set;
 };
-#define rwsem_first_waiter(sem)	sem->first_waiter
 
 enum rwsem_wake_type {
 	RWSEM_WAKE_ANY,		/* Wake whatever's at head of wait list */
@@ -364,36 +363,19 @@ enum rwsem_wake_type {
  */
 #define MAX_READERS_WAKEUP	0x100
 
-static inline void
-rwsem_add_waiter(struct rw_semaphore *sem, struct rwsem_waiter *waiter)
-{
-	struct rwsem_waiter *first = sem->first_waiter;
-	lockdep_assert_held(&sem->wait_lock);
-	if (first) {
-		list_add_tail(&waiter->list, &first->list);
-	} else {
-		INIT_LIST_HEAD(&waiter->list);
-		sem->first_waiter = waiter;
-	}
-	/* caller will set RWSEM_FLAG_WAITERS */
-}
-
 static inline
 bool __rwsem_del_waiter(struct rw_semaphore *sem, struct rwsem_waiter *waiter)
 {
-	if (sem->first_waiter == waiter) {
-		if (list_empty(&waiter->list)) {
-			sem->first_waiter = NULL;
-			return true;
-		} else {
-			sem->first_waiter = list_first_entry(&waiter->list,
-					struct rwsem_waiter, list);
-			list_del(&waiter->list);
-		}
-	} else {
-		list_del(&waiter->list);
+	if (list_empty(&waiter->list)) {
+		sem->first_waiter = NULL;
+		return true;
 	}
 
+	if (sem->first_waiter == waiter)
+		sem->first_waiter = list_first_entry(&waiter->list,
+				struct rwsem_waiter, list);
+	list_del(&waiter->list);
+
 	return false;
 }
 
@@ -453,7 +435,7 @@ static void rwsem_mark_wake(struct rw_semaphore *sem,
 	 * Take a peek at the queue head waiter such that we can determine
 	 * the wakeup(s) to perform.
 	 */
-	waiter = rwsem_first_waiter(sem);
+	waiter = sem->first_waiter;
 
 	if (waiter->type == RWSEM_WAITING_FOR_WRITE) {
 		if (wake_type == RWSEM_WAKE_ANY) {
@@ -612,8 +594,6 @@ rwsem_del_wake_waiter(struct rw_semaphore *sem, struct rwsem_waiter *waiter,
 		      struct wake_q_head *wake_q)
 		      __releases(&sem->wait_lock)
 {
-	bool first = rwsem_first_waiter(sem) == waiter;
-
 	wake_q_init(wake_q);
 
 	/*
@@ -621,7 +601,7 @@ rwsem_del_wake_waiter(struct rw_semaphore *sem, struct rwsem_waiter *waiter,
 	 * the first waiter, we wake up the remaining waiters 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) && sem->first_waiter == waiter)
 		rwsem_mark_wake(sem, RWSEM_WAKE_ANY, wake_q);
 	raw_spin_unlock_irq(&sem->wait_lock);
 	if (!wake_q_empty(wake_q))
@@ -638,7 +618,7 @@ rwsem_del_wake_waiter(struct rw_semaphore *sem, struct rwsem_waiter *waiter,
 static inline bool rwsem_try_write_lock(struct rw_semaphore *sem,
 					struct rwsem_waiter *waiter)
 {
-	struct rwsem_waiter *first = rwsem_first_waiter(sem);
+	struct rwsem_waiter *first = sem->first_waiter;
 	long count, new;
 
 	lockdep_assert_held(&sem->wait_lock);
@@ -1030,7 +1010,7 @@ 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;
+	struct rwsem_waiter waiter, *first;
 	DEFINE_WAKE_Q(wake_q);
 
 	/*
@@ -1071,7 +1051,8 @@ rwsem_down_read_slowpath(struct rw_semaphore *sem, long count, unsigned int stat
 	waiter.handoff_set = false;
 
 	raw_spin_lock_irq(&sem->wait_lock);
-	if (!sem->first_waiter) {
+	first = sem->first_waiter;
+	if (!first) {
 		/*
 		 * In case the wait queue is empty and the lock isn't owned
 		 * by a writer, this reader can exit the slowpath and return
@@ -1087,8 +1068,11 @@ rwsem_down_read_slowpath(struct rw_semaphore *sem, long count, unsigned int stat
 			return sem;
 		}
 		adjustment += RWSEM_FLAG_WAITERS;
+		INIT_LIST_HEAD(&waiter.list);
+		sem->first_waiter = &waiter;
+	} else {
+		list_add_tail(&waiter.list, &first->list);
 	}
-	rwsem_add_waiter(sem, &waiter);
 
 	/* we're now waiting on the lock, but no longer actively locking */
 	count = atomic_long_add_return(adjustment, &sem->count);
@@ -1146,7 +1130,7 @@ rwsem_down_read_slowpath(struct rw_semaphore *sem, long count, unsigned int stat
 static struct rw_semaphore __sched *
 rwsem_down_write_slowpath(struct rw_semaphore *sem, int state)
 {
-	struct rwsem_waiter waiter;
+	struct rwsem_waiter waiter, *first;
 	DEFINE_WAKE_Q(wake_q);
 
 	/* do optimistic spinning and steal lock if possible */
@@ -1165,10 +1149,10 @@ rwsem_down_write_slowpath(struct rw_semaphore *sem, int state)
 	waiter.handoff_set = false;
 
 	raw_spin_lock_irq(&sem->wait_lock);
-	rwsem_add_waiter(sem, &waiter);
 
-	/* we're now waiting on the lock */
-	if (rwsem_first_waiter(sem) != &waiter) {
+	first = sem->first_waiter;
+	if (first) {
+		list_add_tail(&waiter.list, &first->list);
 		rwsem_cond_wake_waiter(sem, atomic_long_read(&sem->count),
 				       &wake_q);
 		if (!wake_q_empty(&wake_q)) {
@@ -1181,6 +1165,8 @@ rwsem_down_write_slowpath(struct rw_semaphore *sem, int state)
 			raw_spin_lock_irq(&sem->wait_lock);
 		}
 	} else {
+		INIT_LIST_HEAD(&waiter.list);
+		sem->first_waiter = &waiter;
 		atomic_long_or(RWSEM_FLAG_WAITERS, &sem->count);
 	}
 

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

[Linus Torvalds]

On Wed, 18 Feb 2026 at 13:00, Matthew Wilcox <willy@infradead.org> wrote:
>
> Here's all the changes I made, and I'll post a rolled-up version
> next.

Thanks, this all looks good to me.

                  Linus

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

[Peter Zijlstra]

On Wed, Feb 18, 2026 at 09:00:14PM +0000, Matthew Wilcox wrote:

> Here's all the changes I made, and I'll post a rolled-up version
> next.

That all seems reasonable.

One small nit:

> +	if (list_empty(&waiter->list)) {
> +		sem->first_waiter = NULL;
> +		return true;
>  	}
>  
> +	if (sem->first_waiter == waiter)
> +		sem->first_waiter = list_first_entry(&waiter->list,
> +				struct rwsem_waiter, list);

Since that's multiple lines, could you wrap in {}, also I tend to prefer
cino=(0:0 style wrapping. That is, something like so:

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


> +	list_del(&waiter->list);
> +
>  	return false;
>  }

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

[Linus Torvalds]

Oh, I already said "this looks good", but I take it back. Looking a
bit closer, I noticed a problem:

On Wed, 18 Feb 2026 at 13:00, Matthew Wilcox <willy@infradead.org> wrote:
>
> @@ -612,8 +594,6 @@ rwsem_del_wake_waiter(struct rw_semaphore *sem, struct rwsem_waiter *waiter,
>                       struct wake_q_head *wake_q)
>                       __releases(&sem->wait_lock)
>  {
> -       bool first = rwsem_first_waiter(sem) == waiter;
> -
>         wake_q_init(wake_q);
>
>         /*
> @@ -621,7 +601,7 @@ rwsem_del_wake_waiter(struct rw_semaphore *sem, struct rwsem_waiter *waiter,
>          * the first waiter, we wake up the remaining waiters 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) && sem->first_waiter == waiter)
>                 rwsem_mark_wake(sem, RWSEM_WAKE_ANY, wake_q);

This looks wrong.

Notice how "first" used to be done before deletion.

How it's doing that

     sem->first_waiter == waiter

test *after* the deletion, which makes no sense at all, and will never
trigger, afaik.

I think that "&& first" was always pointless, because
rwsem_del_waiter() would only return true if it was the only waiter,
and if it was the only waiter then it was first by definition. But now
it's gone from "pointless" to "actively wrong", because now that
"first" test will be false.

So I think the whole test should just be deleted. The return value of
rwsem_del_waiter() is already the right value.

Anyway, this is all from just looking at the patch, so maybe I missed
something, but it does look very wrong.

                Linus

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

[Linus Torvalds]

On Wed, 18 Feb 2026 at 14:45, Linus Torvalds
<torvalds@linux-foundation.org> 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".

              Linus

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@linux-foundation.org> 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.

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

[Waiman Long]

On 2/17/26 2:08 PM, Matthew Wilcox (Oracle) wrote:
> Instead of embedding a list_head in struct rw_semaphore, store a pointer
> to the first waiter.  The list of waiters remains a doubly linked list
> so we can efficiently add to the tail of the list, remove from the front
> (or middle) of the list.
>
> Some of the list manipulation becomes more complicated, but it's a
> reasonable tradeoff on the slow paths to shrink some core data structures
> like struct inode.

If the goal is to use only one pointer for the rwsem structure, would it 
make sense to change list_head to hlist_head for instance? At least we 
have existing helpers that can be used instead of making our own coding 
convention here.

Cheers,
Longman

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

[Matthew Wilcox]

On Wed, Feb 18, 2026 at 05:47:52PM -0500, Waiman Long wrote:
> On 2/17/26 2:08 PM, Matthew Wilcox (Oracle) wrote:
> > Instead of embedding a list_head in struct rw_semaphore, store a pointer
> > to the first waiter.  The list of waiters remains a doubly linked list
> > so we can efficiently add to the tail of the list, remove from the front
> > (or middle) of the list.
> > 
> > Some of the list manipulation becomes more complicated, but it's a
> > reasonable tradeoff on the slow paths to shrink some core data structures
> > like struct inode.
> 
> If the goal is to use only one pointer for the rwsem structure, would it
> make sense to change list_head to hlist_head for instance? At least we have
> existing helpers that can be used instead of making our own coding
> convention here.

There's no hlist_add_tail(), and obviously there can't be.

hlist_head.first = node1
node1.pprev = hlist_head.first
node1.next = node2
node2.pprev = node1
node2.next = NULL

now we want to add node3 to the tail.  there's no pointer to it, we have
to walk the entire chain to find out where to put it.

Whereas with this scheme, we can put it at ->first_waiter.prev.  If you
want to generalise this way to use list_head, be my guest, but I don't
want to do that work (and I don't want this patch to get held up behind
a "boil the ocean" approach).