From owner-cvs-all@FreeBSD.ORG Tue Oct 30 10:14:39 2007 Return-Path: <owner-cvs-all@FreeBSD.ORG> Delivered-To: cvs-all@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id CFA2C16A417; Tue, 30 Oct 2007 10:14:39 +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 8289E13C4B6; Tue, 30 Oct 2007 10:14:38 +0000 (UTC) (envelope-from kris@FreeBSD.org) Message-ID: <47270410.2020802@FreeBSD.org> Date: Tue, 30 Oct 2007 11:14:40 +0100 From: Kris Kennaway <kris@FreeBSD.org> User-Agent: Thunderbird 2.0.0.6 (Macintosh/20070728) MIME-Version: 1.0 To: David Xu <davidxu@FreeBSD.org> References: <200710292101.l9TL1mAE049561@repoman.freebsd.org> <47268F17.1000106@freebsd.org> <Pine.GSO.4.64.0710292207140.19572@sea.ntplx.net> <4726E9AB.4050209@FreeBSD.org> <4726F130.2060709@freebsd.org> <4726F7E9.2060403@FreeBSD.org> <4726FB01.4060704@freebsd.org> In-Reply-To: <4726FB01.4060704@freebsd.org> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: Daniel Eischen <deischen@FreeBSD.org>, 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-all@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: CVS commit messages for the entire tree <cvs-all.freebsd.org> List-Unsubscribe: <http://lists.freebsd.org/mailman/listinfo/cvs-all>, <mailto:cvs-all-request@freebsd.org?subject=unsubscribe> List-Archive: <http://lists.freebsd.org/pipermail/cvs-all> List-Post: <mailto:cvs-all@freebsd.org> List-Help: <mailto:cvs-all-request@freebsd.org?subject=help> List-Subscribe: <http://lists.freebsd.org/mailman/listinfo/cvs-all>, <mailto:cvs-all-request@freebsd.org?subject=subscribe> X-List-Received-Date: Tue, 30 Oct 2007 10:14:40 -0000 David Xu wrote: > Kris Kennaway wrote: >>> >>> 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 didn't find any place in thr_mutex.c that you have set > PTHREAD_MUTEX_ADAPTIVE_NP type, or I missed something ? PTHREAD_ADAPTIVE_MUTEX_INITIALIZER_NP is just in <pthread.h>. /* * Static initialization values. */ #define PTHREAD_MUTEX_INITIALIZER NULL #define PTHREAD_ADAPTIVE_MUTEX_INITIALIZER_NP NULL #define PTHREAD_COND_INITIALIZER NULL #define PTHREAD_RWLOCK_INITIALIZER NULL >>> 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 ;-) >> > see above, I didn't see any code set PTHREAD_MUTEX_ADAPTIVE_NP type. The code is already in mysql for use with glibc. It basically does #ifdef PTHREAD_ADAPTIVE_MUTEX_INITIALIZER_NP /* * Use PTHREAD_MUTEX_ADAPTIVE_NP for the mutexes we know will benefit * from it */ ... #endif so it just works. > My code even needn't to recompile mysql and improve 40% performance. :-) OK, but that's not good enough. As I have mentioned, you should have serious concerns about performance loss in other cases with an approach that always spins in userland when acquiring any mutex. >> > 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 >> > I am waiting for others, since this is first time we have to add a new > mutex type. I don't understand what you are saying here. The glibc interface is defined by glibc, so if we want to do something different then either compatibility interfaces are needed (so what was the point of being different other than "NIH"?), or we need to manually patch the applications to do it "our way". Kris