From:   Maria Yu <quic_aiquny@quicinc.com>
To:     <peterz@infradead.org>
CC:     <boqun.feng@gmail.com>, <hdanton@sina.com>,
        <linux-kernel@vger.kernel.org>, <longman@redhat.com>,
        <mazhenhua@xiaomi.com>, <mingo@redhat.com>, <will@kernel.org>,
        <quic_aiquny@quicinc.com>
Subject: Re: [BUG]locking/rwsem: only clean RWSEM_FLAG_HANDOFF when already set
Date:   Thu, 11 Nov 2021 10:42:10 +0800
Message-ID: <20211111024210.11775-1-quic_aiquny@quicinc.com>
In-Reply-To: <20211110213854.GE174703@worktop.programming.kicks-ass.net>
References: <20211110213854.GE174703@worktop.programming.kicks-ass.net>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

From: quic_aiquny@quicinc.com

There is an important information that when the issue reported, the first waiter can be changed. This is via other hook changes which can not always add to the tail of the waitlist which is not the same of the current upstream code.

The current original Xiaomi's issue when we checked the log, we can find out that the scenario is that write waiter set the sem->count with RWSEM_FLAG_HANDOFF bit and then another reader champ in and got the first waiter.
So When the reader go through rwsem_down_read_slowpath if it have RWSEM_FLAG_HANDOFF bit set, it will clear the RWSEM_FLAG_HANDOFF bit.

So it left the wstate of the local variable as WRITER_HANDOFF, but the reader thought it was a Reader Handoff and cleared it.
While the writer only checkes the wstate local variable of WRITER_HANDOFF, and do the add(-RWSEM_FLAG_HANDOFF) action. If it do a addnot(RWSEM_FLAG_HANDOFF), then the writer will not make the sem->count underflow.

The current scenario looked like this:
--------------------------------
	wstate = WRITER_HANDOFF;
	raw_spin_lock_irq(&sem->wait_lock);
	if (rwsem_try_write_lock(sem, wstate))
	raw_spin_unlock_irq(&sem->wait_lock);
	   :
	   if (signal_pending_state(state, current))       //other *important* reader rwsem_down_read_slowpath add to the first of the waitlist, and clear the RWSEM_FLAG_HANDOFF
	     goto out_nolock
	     out_nolock:
	     add(-RWSEM_FLAG_HANDOFF, sem->count)                     // make the count to be negative.
--------------------------------

So if we do a andnot it will be much more safer, and working in this scenario:
--------------------------------
	wstate = WRITER_HANDOFF;
	raw_spin_lock_irq(&sem->wait_lock);
	if (rwsem_try_write_lock(sem, wstate))
	raw_spin_unlock_irq(&sem->wait_lock);
	   :
	   if (signal_pending_state(state, current))       //other *important* reader rwsem_down_read_slowpath add to the first of the waitlist, and clear the RWSEM_FLAG_HANDOFF
	     goto out_nolock
	     out_nolock:
	     addnot(RWSEM_FLAG_HANDOFF, sem->count)                // make the count unchanged.
--------------------------------


So if we do a andnot it will be much more safer, and working in the not killed normal senario:
--------------------------------
	wstate = WRITER_HANDOFF;
	raw_spin_lock_irq(&sem->wait_lock);
	if (rwsem_try_write_lock(sem, wstate))
	raw_spin_unlock_irq(&sem->wait_lock);
	   :
	   if (signal_pending_state(state, current))       //other *important* reader rwsem_down_read_slowpath add to the first of the waitlist, and clear the RWSEM_FLAG_HANDOFF
		if (wstate == WRITER_HANDOFF)
			trylock_again:
			raw_spin_lock_irq(&sem->wait_lock);
			if (rwsem_try_write_lock(sem, wstate))		//writer will set the RWSEM_FLAG_HANDOFF flag again.
	raw_spin_unlock_irq(&sem->wait_lock);
--------------------------------


Thx and BRs,
Aiqun Yu (Maria)