From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tim Chen Subject: Re: [PATCH v7 2/6] MCS Lock: optimizations and extra comments Date: Mon, 20 Jan 2014 11:11:25 -0800 Message-ID: <1390245085.3138.24.camel@schen9-DESK> References: <1389917300.3138.12.camel@schen9-DESK> <20140120135803.GF31570@twins.programming.kicks-ass.net> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 7bit Return-path: Received: from mga09.intel.com ([134.134.136.24]:18681 "EHLO mga09.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752210AbaATTL2 (ORCPT ); Mon, 20 Jan 2014 14:11:28 -0500 In-Reply-To: <20140120135803.GF31570@twins.programming.kicks-ass.net> Sender: linux-arch-owner@vger.kernel.org List-ID: To: Peter Zijlstra Cc: Ingo Molnar , Andrew Morton , Thomas Gleixner , "Paul E.McKenney" , Will Deacon , linux-kernel@vger.kernel.org, linux-mm , linux-arch@vger.kernel.org, Linus Torvalds , Waiman Long , Andrea Arcangeli , Alex Shi , Andi Kleen , Michel Lespinasse , Davidlohr Bueso , Matthew R Wilcox , Dave Hansen , Rik van Riel , Peter Hurley , Raghavendra K T , George Spelvin , "H. Peter Anvin" , Arnd Bergmann , Aswin On Mon, 2014-01-20 at 14:58 +0100, Peter Zijlstra wrote: > On Thu, Jan 16, 2014 at 04:08:20PM -0800, Tim Chen wrote: > > Remove unnecessary operation and make the cmpxchg(lock, node, NULL) == node > > check in mcs_spin_unlock() likely() as it is likely that a race did not occur > > most of the time. > > It might be good to describe why the node->locked=1 is thought > unnecessary. I concur it is, but upon reading this changelog I was left > wondering and had to go read the code and run through the logic to > convince myself. > > Having done so, I'm now wondering if we think so for the same reason -- > although I'm fairly sure we are. > > The argument goes like: everybody only looks at his own ->locked value, > therefore the only one possibly interested in node->locked is the lock > owner. However the lock owner doesn't care what's in it, it simply > assumes its 1 but really doesn't care one way or another. Yes, it is done for the same reason you mentioned. I'll update the comments better to reflect this. > > That said, a possible DEBUG mode might want to actually set it, validate > that all other linked nodes are 0 and upon unlock verify the same before > flipping next->locked to 1. I'll leave a comment to indicate this. If we need a DEBUG mode later, we can come back to add this easily. Thanks. Tim From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga09.intel.com ([134.134.136.24]:18681 "EHLO mga09.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752210AbaATTL2 (ORCPT ); Mon, 20 Jan 2014 14:11:28 -0500 Subject: Re: [PATCH v7 2/6] MCS Lock: optimizations and extra comments From: Tim Chen In-Reply-To: <20140120135803.GF31570@twins.programming.kicks-ass.net> References: <1389917300.3138.12.camel@schen9-DESK> <20140120135803.GF31570@twins.programming.kicks-ass.net> Content-Type: text/plain; charset="UTF-8" Date: Mon, 20 Jan 2014 11:11:25 -0800 Message-ID: <1390245085.3138.24.camel@schen9-DESK> Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Sender: linux-arch-owner@vger.kernel.org List-ID: To: Peter Zijlstra Cc: Ingo Molnar , Andrew Morton , Thomas Gleixner , "Paul E.McKenney" , Will Deacon , linux-kernel@vger.kernel.org, linux-mm , linux-arch@vger.kernel.org, Linus Torvalds , Waiman Long , Andrea Arcangeli , Alex Shi , Andi Kleen , Michel Lespinasse , Davidlohr Bueso , Matthew R Wilcox , Dave Hansen , Rik van Riel , Peter Hurley , Raghavendra K T , George Spelvin , "H. Peter Anvin" , Arnd Bergmann , Aswin Chandramouleeswaran , Scott J Norton , "Figo.zhang" Message-ID: <20140120191125.3BcnVNxCPpvVFXEd3t9h2k7xJjfo4w42rhnbSk0uvxM@z> On Mon, 2014-01-20 at 14:58 +0100, Peter Zijlstra wrote: > On Thu, Jan 16, 2014 at 04:08:20PM -0800, Tim Chen wrote: > > Remove unnecessary operation and make the cmpxchg(lock, node, NULL) == node > > check in mcs_spin_unlock() likely() as it is likely that a race did not occur > > most of the time. > > It might be good to describe why the node->locked=1 is thought > unnecessary. I concur it is, but upon reading this changelog I was left > wondering and had to go read the code and run through the logic to > convince myself. > > Having done so, I'm now wondering if we think so for the same reason -- > although I'm fairly sure we are. > > The argument goes like: everybody only looks at his own ->locked value, > therefore the only one possibly interested in node->locked is the lock > owner. However the lock owner doesn't care what's in it, it simply > assumes its 1 but really doesn't care one way or another. Yes, it is done for the same reason you mentioned. I'll update the comments better to reflect this. > > That said, a possible DEBUG mode might want to actually set it, validate > that all other linked nodes are 0 and upon unlock verify the same before > flipping next->locked to 1. I'll leave a comment to indicate this. If we need a DEBUG mode later, we can come back to add this easily. Thanks. Tim