From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pd0-f177.google.com (mail-pd0-f177.google.com [209.85.192.177]) by kanga.kvack.org (Postfix) with ESMTP id E06756B0044 for ; Wed, 2 Oct 2013 15:37:43 -0400 (EDT) Received: by mail-pd0-f177.google.com with SMTP id y10so1355068pdj.22 for ; Wed, 02 Oct 2013 12:37:43 -0700 (PDT) Message-ID: <524C75F6.1080701@hp.com> Date: Wed, 02 Oct 2013 15:37:26 -0400 From: Waiman Long MIME-Version: 1.0 Subject: Re: [PATCH v6 5/6] MCS Lock: Restructure the MCS lock defines and locking code into its own file References: <1380147049.3467.67.camel@schen9-DESK> <1380226007.2170.2.camel@buesod1.americas.hpqcorp.net> <1380226997.2602.11.camel@j-VirtualBox> <1380228059.2170.10.camel@buesod1.americas.hpqcorp.net> <1380229794.2602.36.camel@j-VirtualBox> <1380231702.3467.85.camel@schen9-DESK> <1380235333.3229.39.camel@j-VirtualBox> <524C71C1.9060408@hp.com> In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Sender: owner-linux-mm@kvack.org List-ID: To: Jason Low Cc: Tim Chen , Davidlohr Bueso , Ingo Molnar , Andrew Morton , Andrea Arcangeli , Alex Shi , Andi Kleen , Michel Lespinasse , Davidlohr Bueso , Matthew R Wilcox , Dave Hansen , Peter Zijlstra , Rik van Riel , Peter Hurley , linux-kernel@vger.kernel.org, linux-mm On 10/02/2013 03:30 PM, Jason Low wrote: > On Wed, Oct 2, 2013 at 12:19 PM, Waiman Long wrote: >> On 09/26/2013 06:42 PM, Jason Low wrote: >>> On Thu, 2013-09-26 at 14:41 -0700, Tim Chen wrote: >>>> Okay, that would makes sense for consistency because we always >>>> first set node->lock = 0 at the top of the function. >>>> >>>> If we prefer to optimize this a bit though, perhaps we can >>>> first move the node->lock = 0 so that it gets executed after the >>>> "if (likely(prev == NULL)) {}" code block and then delete >>>> "node->lock = 1" inside the code block. >>>> >>>> static noinline >>>> void mcs_spin_lock(struct mcs_spin_node **lock, struct mcs_spin_node >>>> *node) >>>> { >>>> struct mcs_spin_node *prev; >>>> >>>> /* Init node */ >>>> node->next = NULL; >>>> >>>> prev = xchg(lock, node); >>>> if (likely(prev == NULL)) { >>>> /* Lock acquired */ >>>> return; >>>> } >>>> node->locked = 0; >> >> You can remove the locked flag setting statement inside if (prev == NULL), >> but you can't clear the locked flag after xchg(). In the interval between >> xchg() and locked=0, the previous lock owner may come in and set the flag. >> Now if your clear it, the thread will loop forever. You have to clear it >> before xchg(). > Yes, in my most recent version, I left locked = 0 in its original > place so that the xchg() can act as a barrier for it. > > The other option would have been to put another barrier after locked = > 0. I went with leaving locked = 0 in its original place so that we > don't need that extra barrier. I don't think putting another barrier after locked=0 will work. Chronologically, the flag must be cleared before the node address is saved in the lock field. There is no way to guarantee that except by putting the locked=0 before xchg(). -Longman -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: email@kvack.org