Re: [PATCH v2] locking/rwsem: add acquire barrier to read_slowpath exit when queue is empty

[Will Deacon]

Hi Jan, Waiman, [+Jade and Paul for the litmus test at the end]

On Wed, Jul 17, 2019 at 09:22:00PM +0200, Jan Stancek wrote:
> On Wed, Jul 17, 2019 at 10:19:04AM -0400, Waiman Long wrote:
> > > If you add a comment to the code outlining the issue (preferably as a litmus
> > > test involving sem->count and some shared data which happens to be
> > > vmacache_seqnum in your test)), then:
> > > 
> > > Reviewed-by: Will Deacon <will@xxxxxxxxxx>
> > > 
> > > Thanks,
> > > 
> > > Will
> > 
> > Agreed. A comment just above smp_acquire__after_ctrl_dep() on why this
> > is needed will be great.
> > 
> > Other than that,
> > 
> > Acked-by: Waiman Long <longman@xxxxxxxxxx>
> > 
> 
> litmus test looks a bit long, would following be acceptable?
> 
> diff --git a/kernel/locking/rwsem.c b/kernel/locking/rwsem.c
> index 37524a47f002..d9c96651bfc7 100644
> --- a/kernel/locking/rwsem.c
> +++ b/kernel/locking/rwsem.c
> @@ -1032,6 +1032,13 @@ static inline bool rwsem_reader_phase_trylock(struct rw_semaphore *sem,
>  		 */
>  		if (adjustment && !(atomic_long_read(&sem->count) &
>  		     (RWSEM_WRITER_MASK | RWSEM_FLAG_HANDOFF))) {
> +			/*
> +			 * down_read() issued ACQUIRE on enter, but we can race
> +			 * with writer who did RELEASE only after us.
> +			 * ACQUIRE here makes sure reader operations happen only
> +			 * after all writer ones.
> +			 */

How about an abridged form of the litmus test here, just to show the cod
flow? e.g.:

/*
 * We need to ensure ACQUIRE semantics when reading sem->count so that
 * we pair with the RELEASE store performed by an unlocking/downgrading
 * writer.
 *
 * P0 (writer)			P1 (reader)
 *
 * down_write(sem);
 * <write shared data>
 * downgrade_write(sem);
 * -> fetch_add_release(&sem->count)
 *
 *				down_read_slowpath(sem);
 *				-> atomic_read(&sem->count)
 *				   <ctrl dep>
 *				   smp_acquire__after_ctrl_dep()
 *				<read shared data>
 */

In writing this, I also noticed that we don't have any explicit ordering
at the end of the reader slowpath when we wait on the queue but get woken
immediately:

	if (!waiter.task)
		break;

Am I missing something?

> +			smp_acquire__after_ctrl_dep();
>  			raw_spin_unlock_irq(&sem->wait_lock);
>  			rwsem_set_reader_owned(sem);
>  			lockevent_inc(rwsem_rlock_fast);
> 
> 
> with litmus test in commit log:
> ----------------------------------- 8< ------------------------------------
> C rwsem
> 
> {
> 	atomic_t rwsem_count = ATOMIC_INIT(1);
> 	int vmacache_seqnum = 10;
> }
> 
> P0(int *vmacache_seqnum, atomic_t *rwsem_count)
> {
> 	r0 = READ_ONCE(*vmacache_seqnum);
> 	WRITE_ONCE(*vmacache_seqnum, r0 + 1);
> 	/* downgrade_write */
> 	r1 = atomic_fetch_add_release(-1+256, rwsem_count);
> }
> 
> P1(int *vmacache_seqnum, atomic_t *rwsem_count, spinlock_t *sem_wait_lock)
> {
> 	/* rwsem_read_trylock */
> 	r0 = atomic_add_return_acquire(256, rwsem_count);
> 	/* rwsem_down_read_slowpath */
> 	spin_lock(sem_wait_lock);
> 	r0 = atomic_read(rwsem_count);
> 	if ((r0 & 1) == 0) {
> 		// BUG: needs barrier
> 		spin_unlock(sem_wait_lock);
> 		r1 = READ_ONCE(*vmacache_seqnum);
> 	}
> }
> exists (1:r1=10)
> ----------------------------------- 8< ------------------------------------

Thanks for writing this! It's definitely worth sticking it in the commit
log, but Paul and Jade might also like to include it as part of their litmus
test repository too.

Will