Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752322AbcCBHNd (ORCPT ); Wed, 2 Mar 2016 02:13:33 -0500 Received: from mail-oi0-f53.google.com ([209.85.218.53]:35507 "EHLO mail-oi0-f53.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750992AbcCBHNc (ORCPT ); Wed, 2 Mar 2016 02:13:32 -0500 MIME-Version: 1.0 In-Reply-To: <20160302063914.GA19608@dvhart-mobl5.amr.corp.intel.com> References: <1456746093-10578-1-git-send-email-nasa4836@gmail.com> <20160229222209.GD7499@dvhart-mobl5.amr.corp.intel.com> <20160302013759.GA362@dvhart-mobl5.amr.corp.intel.com> <20160302063914.GA19608@dvhart-mobl5.amr.corp.intel.com> From: Jianyu Zhan Date: Wed, 2 Mar 2016 15:12:51 +0800 Message-ID: Subject: Re: [PATCH] futex: replace bare barrier() with a READ_ONCE() To: Darren Hart Cc: LKML , Thomas Gleixner , dave@stgolabs.net, Peter Zijlstra , Andrew Morton , Ingo Molnar , dvhart@linux.intel.com, bigeasy@linutronix.de, Paul McKenney Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2496 Lines: 75 On Wed, Mar 2, 2016 at 2:39 PM, Darren Hart wrote: > I believe you are correct with respect to the retry and while condition being an > appropriate place for the application of READ_ONCE. The question is why is this > preferred to the existing barrier()? I suggest: > > While barrier() is a fairly brute force general application of a compiler > barrier, READ_ONCE() is very specific, it targets only operations dealing with > the specified variable. As such, its application both clearly identifies the > volatile variable and frees the compiler to make optimizations a more general > barrier would forbid. > > Yep, beside the informative point, the more specifics of READ_ONCE over barrier is what I meant "lightweight", I missed emphasizing this point. Thanks for pointing it. I will respin this patch to reflect this. >> >> >> > As for #2... >> > >> >> 2. For the second problem I memtioned, yes, it is theoretical, and >> >> it is also due to q->lock_ptr can change between >> >> the test of nullness of q->lock_ptr and the lock on q->lock_ptr. >> >> >> >> the code is >> >> >> >> retry: >> >> lock_ptr = q->lock_ptr; >> >> if (lock_ptr != 0) { >> >> spin_lock(lock_ptr) >> >> if (unlikely(lock_ptr != q->lock_ptr)) { >> >> spin_unlock(lock_ptr); >> >> goto retry; >> >> } >> >> ... >> >> } >> >> >> >> which is effectively the same as : >> >> >> >> while (lock_ptr = q->lock_ptr) { >> >> spin_lock(lock_ptr) >> >> if (unlikely(lock_ptr != q->lock_ptr)) { >> >> spin_unlock(lock_ptr); >> >> goto retry; >> >> } >> >> ... >> >> } >> >> >> >> This might cause the compiler load the q->lock_ptr once and use it >> >> many times, quoted from > > Which is already covered by the barrier() in place today as a more general > compiler barrier. > > Your argument is then simply that READ_ONCE is a more specific solution to the > problem. > Yep. And after re-thinking, I am now less convinced in this second argument, since it involves a comparison of q->lock_ptr in the loop body, so this may defeat any attempts that compilers try to optimize the load out of the loop, even without a READ_ONCE(). But I will also incorporate this in the second submission for review . Regards, Jianyu Zhan