From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Subject: Re: [PATCH] mutex: Introduce arch_mutex_cpu_relax() From: Gerald Schaefer Reply-To: gerald.schaefer@de.ibm.com In-Reply-To: <1287428051.1998.2124.camel@laptop> References: <1287070392.8344.15.camel@thinkpad> <1287071960.29097.284.camel@twins> <1287077465.8344.47.camel@thinkpad> <1287078025.8344.56.camel@thinkpad> <20101014151349.826b5271.akpm@linux-foundation.org> <1287140101.2547.6.camel@thinkpad> <1287140855.2547.18.camel@thinkpad> <1287428051.1998.2124.camel@laptop> Content-Type: text/plain; charset="UTF-8" Date: Tue, 19 Oct 2010 14:24:08 +0200 Message-ID: <1287491048.3545.19.camel@thinkpad> Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-Archive: List-Post: To: Peter Zijlstra Cc: Andrew Morton , Ingo Molnar , Martin Schwidefsky , LKML , linux-s390 , Heiko Carstens List-ID: On Mo, 2010-10-18 at 20:54 +0200, Peter Zijlstra wrote: > On Fri, 2010-10-15 at 13:07 +0200, Gerald Schaefer wrote: > > From: Gerald Schaefer > > > > The spinning mutex implementation uses cpu_relax() in busy loops as a > > compiler barrier. Depending on the architecture, cpu_relax() may do more > > than needed in this specific mutex spin loops. On System z we also give > > up the time slice of the virtual cpu in cpu_relax(), which prevents > > effective spinning on the mutex. > > > > This patch replaces cpu_relax() in the spinning mutex code with > > arch_mutex_cpu_relax(), which can be defined by each architecture in > > include/asm/mutex.h. The default is still cpu_relax(), so this should > > not affect other architectures than System z for now. > > Ingo's randconfig build found the following, .config attached. > > including "asm/mutex.h" isn't advised. > > CC kernel/mutex.o > In file included from /usr/src/linux-2.6/kernel/mutex.c:33: > /usr/src/linux-2.6/include/asm-generic/mutex-null.h:13:1: warning: "__mutex_fastpath_lock" redefined > In file included from /usr/src/linux-2.6/arch/x86/include/asm/mutex.h:4, > from /usr/src/linux-2.6/include/linux/mutex.h:19, > from /usr/src/linux-2.6/kernel/mutex.c:20: > /usr/src/linux-2.6/arch/x86/include/asm/mutex_64.h:19:1: warning: this is the location of the previous definition > In file included from /usr/src/linux-2.6/kernel/mutex.c:33: > /usr/src/linux-2.6/include/asm-generic/mutex-null.h:15:1: warning: "__mutex_fastpath_unlock" redefined > In file included from /usr/src/linux-2.6/arch/x86/include/asm/mutex.h:4, > from /usr/src/linux-2.6/include/linux/mutex.h:19, > from /usr/src/linux-2.6/kernel/mutex.c:20: > /usr/src/linux-2.6/arch/x86/include/asm/mutex_64.h:62:1: warning: this is the location of the previous definition > In file included from /usr/src/linux-2.6/kernel/mutex.c:33: > /usr/src/linux-2.6/include/asm-generic/mutex-null.h:13:1: warning: "__mutex_fastpath_lock" redefined > In file included from /usr/src/linux-2.6/arch/x86/include/asm/mutex.h:4, > from /usr/src/linux-2.6/include/linux/mutex.h:19, > from /usr/src/linux-2.6/kernel/mutex.c:20: > /usr/src/linux-2.6/arch/x86/include/asm/mutex_64.h:19:1: warning: this is the location of the previous definition > In file included from /usr/src/linux-2.6/kernel/mutex.c:33: > /usr/src/linux-2.6/include/asm-generic/mutex-null.h:15:1: warning: "__mutex_fastpath_unlock" redefined > In file included from /usr/src/linux-2.6/arch/x86/include/asm/mutex.h:4, > from /usr/src/linux-2.6/include/linux/mutex.h:19, > from /usr/src/linux-2.6/kernel/mutex.c:20: > /usr/src/linux-2.6/arch/x86/include/asm/mutex_64.h:62:1: warning: this is the location of the previous definition Ok, I see now that including from include/linux/mutex.h is not a good idea, because of this code in kernel/mutex.c (and the conflict with CONFIG_DEBUG_MUTEXES set): #ifdef CONFIG_DEBUG_MUTEXES # include "mutex-debug.h" # include #else # include "mutex.h" # include #endif Putting the architecture specific details of arch_mutex_cpu_relax() somewhere else than doesn't seem like a good idea either. Also, putting an "#ifndef CONFIG_DEBUG_MUTEXES" around my "#include " in include/linux/mutex.h would fix the conflict, but that also looks rather ugly. So I guess I'll just go back to the original Kconfig approach, which at least avoids all this header file mess. I'll send a new patch.