From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52755) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b38X7-0006Se-Ja for qemu-devel@nongnu.org; Wed, 18 May 2016 16:57:34 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1b38X4-0006pi-Fe for qemu-devel@nongnu.org; Wed, 18 May 2016 16:57:33 -0400 Received: from mail-lf0-x241.google.com ([2a00:1450:4010:c07::241]:36403) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b38X4-0006pT-3s for qemu-devel@nongnu.org; Wed, 18 May 2016 16:57:30 -0400 Received: by mail-lf0-x241.google.com with SMTP id y84so3991945lfc.3 for ; Wed, 18 May 2016 13:57:29 -0700 (PDT) References: <1463196873-17737-1-git-send-email-cota@braap.org> <1463196873-17737-8-git-send-email-cota@braap.org> <573CC7AD.2040108@gmail.com> <20160518205232.GA24279@flamenco> From: Sergey Fedorov Message-ID: <573CD737.3020303@gmail.com> Date: Wed, 18 May 2016 23:57:27 +0300 MIME-Version: 1.0 In-Reply-To: <20160518205232.GA24279@flamenco> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v5 07/18] qemu-thread: add simple test-and-set spinlock List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Emilio G. Cota" Cc: QEMU Developers , MTTCG Devel , =?UTF-8?Q?Alex_Benn=c3=a9e?= , Paolo Bonzini , Peter Crosthwaite , Richard Henderson On 18/05/16 23:52, Emilio G. Cota wrote: > 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(). Great! > >>> +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. It's "static inline" anyway, so that shouldn't matter much if we have that "if". But you're right, let's save likely/unlikely for the user of the function. Kind regards, Sergey