Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1762166AbcLPWfU (ORCPT ); Fri, 16 Dec 2016 17:35:20 -0500 Received: from mail-wj0-f194.google.com ([209.85.210.194]:36013 "EHLO mail-wj0-f194.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752990AbcLPWfP (ORCPT ); Fri, 16 Dec 2016 17:35:15 -0500 Subject: Re: [PATCH v2 05/11] locking/ww_mutex: Add waiters in stamp order To: Peter Zijlstra , =?UTF-8?Q?Nicolai_H=c3=a4hnle?= References: <1480601214-26583-1-git-send-email-nhaehnle@gmail.com> <1480601214-26583-6-git-send-email-nhaehnle@gmail.com> <20161206165544.GX3045@worktop.programming.kicks-ass.net> <20161216171524.GU3107@twins.programming.kicks-ass.net> <98cfeb6e-f312-ba13-00b4-f5b125b24f8d@gmail.com> <20161216200023.GH3124@twins.programming.kicks-ass.net> Cc: linux-kernel@vger.kernel.org, Ingo Molnar , Maarten Lankhorst , Daniel Vetter , Chris Wilson , dri-devel@lists.freedesktop.org From: =?UTF-8?Q?Nicolai_H=c3=a4hnle?= Message-ID: Date: Fri, 16 Dec 2016 23:35:12 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.5.1 MIME-Version: 1.0 In-Reply-To: <20161216200023.GH3124@twins.programming.kicks-ass.net> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 1896 Lines: 45 On 16.12.2016 21:00, Peter Zijlstra wrote: > On Fri, Dec 16, 2016 at 07:11:41PM +0100, Nicolai H?hnle wrote: >> mutex_optimistic_spin() already calls __mutex_trylock, and for the no-spin >> case, __mutex_unlock_slowpath() only calls wake_up_q() after releasing the >> wait_lock. > > mutex_optimistic_spin() is a no-op when !CONFIG_MUTEX_SPIN_ON_OWNER Does this change the conclusion in a meaningful way? I did mention the no-spin case in the very part you quoted... Again, AFAIU we're talking about the part of my proposal that turns what is effectively __mutex_trylock(lock, ...); spin_lock_mutex(&lock->wait_lock, flags); (independent of whether the trylock succeeds or not!) into spin_lock_mutex(&lock->wait_lock, flags); __mutex_trylock(lock, ...); in an effort to streamline the code overall. Also AFAIU, you're concerned that spin_lock_mutex(...) has to wait for an unlock from mutex_unlock(), but when does that actually happen with relevant probability? When we spin optimistically, that could happen -- except that __mutex_trylock is already called in mutex_optimistic_spin, so it doesn't matter. When we don't spin -- whether due to .config or !first -- then the chance of overlap with mutex_unlock is exceedingly small. Even if we do overlap, we'll have to wait for mutex_unlock to release the wait_lock anyway! So what good does acquiring the lock first really do? Anyway, this is really more of an argument about whether there's really a good reason to calling __mutex_trylock twice in that loop. I don't think there is, your arguments certainly haven't been convincing, but the issue can be side-stepped for this patch by keeping the trylock calls as they are and just setting first = true unconditionally for ww_ctx != NULL (but keep the logic for when to set the HANDOFF flag as-is). Should probably rename the variable s/first/handoff/ then. Nicolai