Date: Tue, 28 Apr 2015 18:52:26 +0200
From: Peter Zijlstra <peterz@infradead.org>
To: Waiman Long <Waiman.Long@hp.com>
Cc: Ingo Molnar <mingo@kernel.org>, linux-kernel@vger.kernel.org,
        Jason Low <jason.low2@hp.com>, Davidlohr Bueso <dave@stgolabs.net>,
        Scott J Norton <scott.norton@hp.com>,
        Douglas Hatch <doug.hatch@hp.com>
Subject: Re: [PATCH v3] locking/rwsem: reduce spinlock contention in wakeup
 after up_read/up_write
Message-ID: <20150428165226.GG23123@twins.programming.kicks-ass.net>
References: <1429898069-28907-1-git-send-email-Waiman.Long@hp.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1429898069-28907-1-git-send-email-Waiman.Long@hp.com>
User-Agent: Mutt/1.5.21 (2012-12-30)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2437
Lines: 63

On Fri, Apr 24, 2015 at 01:54:29PM -0400, Waiman Long wrote:
> +static inline bool rwsem_has_active_writer(struct rw_semaphore *sem)
> +{
> +	return READ_ONCE(sem->owner) != NULL;
> +}

> +static inline bool rwsem_has_spinner(struct rw_semaphore *sem)
> +{
> +	return osq_is_locked(&sem->osq);
> +}

> +	/*
> +	 * If a spinner is present, it is not necessary to do the wakeup.
> +	 * Try to do wakeup only if the trylock succeeds to minimize
> +	 * spinlock contention which may introduce too much delay in the
> +	 * unlock operation.
> +	 *
> +	 *    spinning writer		up_write/up_read caller
> +	 *    ---------------		-----------------------
> +	 * [S]   osq_unlock()		[L]   osq
> +	 *	 MB			      MB
> +	 * [RmW] rwsem_try_write_lock() [RmW] spin_trylock(wait_lock)
> +	 *
> +	 * Here, it is important to make sure that there won't be a missed
> +	 * wakeup while the rwsem is free and the only spinning writer goes
> +	 * to sleep without taking the rwsem. In case the spinning writer is
> +	 * just going to break out of the waiting loop, it will still do a
> +	 * trylock in rwsem_down_write_failed() before sleeping. IOW, if
> +	 * rwsem_has_spinner() is true, it will  guarantee at least one
> +	 * trylock attempt on the rwsem.
> +	 */
> +	if (!rwsem_has_spinner(sem)) {
> +		raw_spin_lock_irqsave(&sem->wait_lock, flags);
> +	} else {
> +		/*
> +		 * rwsem_has_spinner() is an atomic read while spin_trylock
> +		 * does not guarantee a full memory barrier. Insert a memory
> +		 * barrier here to make sure that wait_lock isn't read until
> +		 * after osq.
> +		 * Note: smp_rmb__after_atomic() should be used if available.
> +		 */
> +		smp_mb__after_atomic();

Sorry, that's wrong. the smp_mb__{before,after}_atomic() are for all
atomic (RmW) ops that do not return a value.

They end up as whatever barrier is required to make real atomic (RmW)
ops (LOCK on x86, LL/SC on risc etc) ordered. And all atomic (RmW) ops
that return a value are already guaranteed to imply full ordering
semantics.

Note, the (RmW) part is important here, atomic_{set,read}() are _NOT_
read-modify-write ops.

> +		if (!raw_spin_trylock_irqsave(&sem->wait_lock, flags))
> +			return sem;
> +	}
>  
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/