Date:   Mon, 27 Feb 2023 11:31:02 +0100
From:   Peter Zijlstra <peterz@infradead.org>
To:     Waiman Long <longman@redhat.com>
Cc:     mingo@redhat.com, will@kernel.org, linux-kernel@vger.kernel.org,
        boqun.feng@gmail.com
Subject: Re: [PATCH 3/6] locking/rwsem: Rework writer wakeup
Message-ID: <Y/yGZgz1cJ1+pTt5@hirez.programming.kicks-ass.net>
References: <20230223122642.491637862@infradead.org>
 <20230223123319.487908155@infradead.org>
 <Y/t1AwGC9OoN/lFc@hirez.programming.kicks-ass.net>
 <Y/uN+89FlTw45uiA@hirez.programming.kicks-ass.net>
 <943686ee-975d-a463-46d1-04b200ac19b1@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <943686ee-975d-a463-46d1-04b200ac19b1@redhat.com>
Precedence: bulk

On Sun, Feb 26, 2023 at 07:22:47PM -0500, Waiman Long wrote:

> @@ -1151,55 +1154,39 @@ rwsem_down_write_slowpath(struct rw_semaphore *sem,
> int state)
> ??????????????? }
> ??????? } else {
> ??????????????? atomic_long_or(RWSEM_FLAG_WAITERS, &sem->count);
> +?????????????? if (rwsem_try_write_lock(sem, &waiter))
> +?????????????????????? waiter.task = NULL;
> ??????? }
> +?????? raw_spin_unlock_irq(&sem->wait_lock);
> 
> ??????? /* wait until we successfully acquire the lock */
> -?????? set_current_state(state);
> ??????? trace_contention_begin(sem, LCB_F_WRITE);
> 
> ??????? for (;;) {
> -?????????????? if (rwsem_try_write_lock(sem, &waiter)) {
> -?????????????????????? /* rwsem_try_write_lock() implies ACQUIRE on success
> */
> +?????????????? set_current_state(state);
> +?????????????? if (!smp_load_acquire(&waiter.task)) {
> +?????????????????????? /* Matches rwsem_waiter_wake()'s
> smp_store_release(). */
> ??????????????????????? break;
> ??????????????? }
> -
> 
> The additional rwsem_try_write_lock() call seems to address the missed
> wakeup problem AFAICT.

Indeed, prior to this I could readily reproduce the lockup.

So when thinking about missing wakeups I noticed this race on WAITERS.
If we queue but the unlock does not yet observe WAITERS the unlock does
not go into the slow path and wakeup gets lost.

Reader side fixes this with rwsem_cond_wake_waiter(), but I could not
convince myself that is correct for writer side -- perhaps it is, will
need to think more on that.

> I do have some concern that early lock transfer to a lock owner that has not
> been woken up yet may suppress writer lock stealing from optimistic spinning
> causing some performance regression in some cases. Let's see if the test
> robot report anything.

Ah yes, I suppose that is indeed a possibility. Given this is all under
wait_lock and the spinner is not, I was hoping it would still have
sufficient time to win. But yes, robots will tell us.