From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754780AbbBRTOI (ORCPT ); Wed, 18 Feb 2015 14:14:08 -0500 Received: from mail-we0-f182.google.com ([74.125.82.182]:37667 "EHLO mail-we0-f182.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751715AbbBRTOG (ORCPT ); Wed, 18 Feb 2015 14:14:06 -0500 Message-ID: <54E4E479.4050003@colorfullife.com> Date: Wed, 18 Feb 2015 20:14:01 +0100 From: Manfred Spraul User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.4.0 MIME-Version: 1.0 To: Oleg Nesterov , "Paul E. McKenney" CC: Peter Zijlstra , Kirill Tkhai , linux-kernel@vger.kernel.org, Ingo Molnar , Josh Poimboeuf Subject: Re: [PATCH 2/2] [PATCH] sched: Add smp_rmb() in task rq locking cycles References: <20150217104516.12144.85911.stgit@tkhai> <1424170021.5749.22.camel@tkhai> <20150217121258.GM5029@twins.programming.kicks-ass.net> <20150217130523.GV24151@twins.programming.kicks-ass.net> <20150217160532.GW4166@linux.vnet.ibm.com> <20150217183636.GR5029@twins.programming.kicks-ass.net> <20150217215231.GK4166@linux.vnet.ibm.com> <20150218155904.GA27687@redhat.com> In-Reply-To: <20150218155904.GA27687@redhat.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Oleg, On 02/18/2015 04:59 PM, Oleg Nesterov wrote: > Let's look at sem_lock(). I never looked at this code before, I can be > easily wrong. Manfred will correct me. But at first glance we can write > the oversimplified pseudo-code: > > spinlock_t local, global; > > bool my_lock(bool try_local) > { > if (try_local) { > spin_lock(&local); > if (!spin_is_locked(&global)) > return true; > spin_unlock(&local); > } > > spin_lock(&global); > spin_unlock_wait(&local); > return false; > } > > void my_unlock(bool drop_local) > { > if (drop_local) > spin_unlock(&local); > else > spin_unlock(&global); > } > > it assumes that the "local" lock is cheaper than "global", the usage is > > bool xxx = my_lock(condition); > /* CRITICAL SECTION */ > my_unlock(xxx); > > Now. Unless I missed something, my_lock() does NOT need a barrier BEFORE > spin_unlock_wait() (or spin_is_locked()). Either my_lock(true) should see > spin_is_locked(global) == T, or my_lock(false)->spin_unlock_wait() should > see that "local" is locked and wait. I would agree: There is no need for a barrier. spin_unlock_read() is just a read, the barriers are from spin_lock() and spin_unlock(). The barrier exist to protect something like a "force_global" flag (complex_count) > spinlock_t local, global; > bool force_global; > bool my_lock(bool try_local) > { > if (try_local) { > spin_lock(&local); > if (!spin_is_locked(&global)) { > if (!force_global) { > return true; > } > } > spin_unlock(&local); > > > spin_lock(&global); > spin_unlock_wait(&local); > return false; > } > > void my_unlock(bool drop_local) > { > if (drop_local) > spin_unlock(&local); > else > spin_unlock(&global); > } > } force_global can only be set by the owner of &global. > Another question is do we need a barrier AFTER spin_unlock_wait(). I do not > know what ipc/sem.c actually needs, but in general (I think) this does need > mb(). Otherwise my_lock / my_unlock itself does not have the proper acq/rel > semantics. For example, my_lock(false) can miss the changes which were done > under my_lock(true). How could that happen? I thought that thread A: protected_var = 1234; spin_unlock(&lock_a) thread B: spin_lock(&lock_b) if (protected_var) is safe. i.e, there is no need that acquire and releases is done on the same pointer. -- Manfred