From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:51063)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <cota@braap.org>) id 1b38SX-00046h-VR
	for qemu-devel@nongnu.org; Wed, 18 May 2016 16:52:51 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <cota@braap.org>) id 1b38ST-0005Wu-PC
	for qemu-devel@nongnu.org; Wed, 18 May 2016 16:52:48 -0400
Received: from out5-smtp.messagingengine.com ([66.111.4.29]:40043)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <cota@braap.org>) id 1b38SR-0005SW-Ex
	for qemu-devel@nongnu.org; Wed, 18 May 2016 16:52:45 -0400
Received: from compute2.internal (compute2.nyi.internal [10.202.2.42])
	by mailout.nyi.internal (Postfix) with ESMTP id 1EAA8209CE
	for <qemu-devel@nongnu.org>; Wed, 18 May 2016 16:52:33 -0400 (EDT)
Date: Wed, 18 May 2016 16:52:32 -0400
From: "Emilio G. Cota" <cota@braap.org>
Message-ID: <20160518205232.GA24279@flamenco>
References: <1463196873-17737-1-git-send-email-cota@braap.org>
	<1463196873-17737-8-git-send-email-cota@braap.org>
	<573CC7AD.2040108@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <573CC7AD.2040108@gmail.com>
Subject: Re: [Qemu-devel] [PATCH v5 07/18] qemu-thread: add simple
 test-and-set spinlock
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel/>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Sergey Fedorov <serge.fdrv@gmail.com>
Cc: QEMU Developers <qemu-devel@nongnu.org>, MTTCG Devel <mttcg@listserver.greensocs.com>, Alex =?iso-8859-1?Q?Benn=E9e?= <alex.bennee@linaro.org>, Paolo Bonzini <pbonzini@redhat.com>, Peter Crosthwaite <crosthwaite.peter@gmail.com>, Richard Henderson <rth@twiddle.net>

On Wed, May 18, 2016 at 22:51:09 +0300, Sergey Fedorov wrote:
> On 14/05/16 06:34, Emilio G. Cota wrote:
> > +static inline void qemu_spin_lock(QemuSpin *spin)
> > +{
> > +    while (atomic_test_and_set_acquire(&spin->value)) {
> 
> A possible optimization might be using unlikely() here, copmare:

Testing with a spinlock-heavy workload reveals a little improvement:

taskset -c 0 tests/qht-bench \
	-d 5 -n 1 -u 100 -k 4096 -K 4096 -l 4096 -r 4096 -s 4096

I'm running this 10 times. Results in Mops/s:
Head			31.283 +- 0.190557661148069
while (unlikely)	31.397 +- 0.107501937967028
if (likely) + while	31.524 +- 0.219605707272527 

The last case does:
    if (likely(__sync_lock_test_and_set(&spin->value, true) == false)) {
        return;
    }
    while (__sync_lock_test_and_set(&spin->value, true)) {
        while (atomic_read(&spin->value)) {
            cpu_relax();
        }
    }

Although I don't like how this will do the TAS twice if the lock is
contended.

I'll just add the unlikely() to while().

> > +static inline int qemu_spin_trylock(QemuSpin *spin)
> > +{
> > +    if (atomic_test_and_set_acquire(&spin->value)) {
> > +        return -EBUSY;
> > +    }
> > +    return 0;
> > +}
> 
> Here we could also benefit from unlikely(), I think.

I never liked this branch in _trylock, because there will
be a branch anyway around the function. How about:

static inline bool qemu_spin_trylock(QemuSpin *spin)
{
    return __sync_lock_test_and_set(&spin->value, true);
}

We don't return EBUSY, which nobody cares about anyway; callers
will still do if (!trylock). With this we save a branch,
and let callers sprinkle likely/unlikely based on how contented
they expect the lock to be.

		Emilio