From owner-cvs-src@FreeBSD.ORG Tue Oct 30 09:22:47 2007 Return-Path: Delivered-To: cvs-src@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id DBD4816A417; Tue, 30 Oct 2007 09:22:47 +0000 (UTC) (envelope-from kris@FreeBSD.org) Received: from weak.local (pointyhat.freebsd.org [IPv6:2001:4f8:fff6::2b]) by mx1.freebsd.org (Postfix) with ESMTP id 4270F13C480; Tue, 30 Oct 2007 09:22:46 +0000 (UTC) (envelope-from kris@FreeBSD.org) Message-ID: <4726F7E9.2060403@FreeBSD.org> Date: Tue, 30 Oct 2007 10:22:49 +0100 From: Kris Kennaway User-Agent: Thunderbird 2.0.0.6 (Macintosh/20070728) MIME-Version: 1.0 To: David Xu References: <200710292101.l9TL1mAE049561@repoman.freebsd.org> <47268F17.1000106@freebsd.org> <4726E9AB.4050209@FreeBSD.org> <4726F130.2060709@freebsd.org> In-Reply-To: <4726F130.2060709@freebsd.org> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: Daniel Eischen , cvs-src@FreeBSD.org, src-committers@FreeBSD.org, cvs-all@FreeBSD.org Subject: Re: cvs commit: src/lib/libthr/thread thr_mutex.c src/lib/libkse/thread thr_mutex.c src/include pthread.h X-BeenThere: cvs-src@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: CVS commit messages for the src tree List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 30 Oct 2007 09:22:48 -0000 David Xu wrote: >>> As I said in previous email, I would rather see our default >>> mutex implementations improved instead of adding new interfaces. >>> If it's really necessary in the short term, perhaps an >>> environment variable that can be set to force all mutexes >>> to be adaptive (and when kern.smp.cpus > 1 perhaps?). >> >> >> There might be a case for adding that for people who want to >> experiment, but it's not appropriate as a replacement since it's >> highly application specific, and the application already knows whether >> it wants this property or not. It is also often not appropriate to >> use this behaviour on every mutex used by an application. >> > > I do think many application writters do not know what should be done for > his mutexes, generic spinning may be OK, but can be turned off. That is a fairly radical change you are proposing. One would expect that this adaptive spinning algorithm *reduces* performance in cases where the mutex is usually held for longer periods of time. If you think this theoretical expectation is wrong, you need to provide some careful measurements supporting that somewhat surprising hypothesis :) >> When arguing about this commit, keep in mind that with this simple >> change mysql performs 30% better out of the box at high loads (without >> requiring any patches). That is not something that should be lightly >> dismissed until you have a better replacement ready. >> >> Kris >> > > I object adding PTHREAD_MUTEX_ADAPTIVE_NP, because there is no > corresponding PTHREAD_ADPATIVE_MUTEX_INITIALIZER, but normal > pthread mutex has macro PTHREAD_MUTEX_INITIALIZER, so it is > inconsistent, maybe adding pthread_mutexattr_setspin() etcs are better > than this hack for our implementation, it is not portable though. There is an PTHREAD_ADAPTIVE_MUTEX_INITIALIZER_NP, which is again the name that existing applications expect for it. The fact that this interface *already exists* in other operating systems and *is already used* by real applications overrides objections about one name choice vs another. The best you can argue for is to use a different name with a compatibility #define, but I dont see the point of this. > I remembered mysql uses this macro to initialize spin mutex, and you > indead needs a patch to let it work No, with the code I committed mysql detects and uses it out of the box, without requiring any patches. It is easy to measure the resulting 30% performance improvement at high loads ;-) > in fact spin mutex in libthr is > arguable, normally you can use pthread_mutex_trylock() in application to > simulate spinning, adding such mutex type it in libthr just simplified > it, but it is not portable. That is what the "_NP" indicates (although remember that this interface is compatible with glibc). Kris