From owner-cvs-all@FreeBSD.ORG Tue Oct 30 08:53:19 2007 Return-Path: Delivered-To: cvs-all@hub.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 1D5E716A420; Tue, 30 Oct 2007 08:53:19 +0000 (UTC) (envelope-from davidxu@FreeBSD.org) Received: from freefall.freebsd.org (freefall.freebsd.org [IPv6:2001:4f8:fff6::28]) by mx1.freebsd.org (Postfix) with ESMTP id 074CF13C48D; Tue, 30 Oct 2007 08:53:19 +0000 (UTC) (envelope-from davidxu@FreeBSD.org) Received: from [127.0.0.1] (root@localhost [127.0.0.1]) by freefall.freebsd.org (8.14.1/8.14.1) with ESMTP id l9U8rEUp069730; Tue, 30 Oct 2007 08:53:15 GMT (envelope-from davidxu@freebsd.org) Message-ID: <4726F130.2060709@freebsd.org> Date: Tue, 30 Oct 2007 16:54:08 +0800 From: David Xu User-Agent: Mozilla/5.0 (X11; U; FreeBSD i386; en-US; rv:1.7.13) Gecko/20070516 X-Accept-Language: en-us, en MIME-Version: 1.0 To: Kris Kennaway References: <200710292101.l9TL1mAE049561@repoman.freebsd.org> <47268F17.1000106@freebsd.org> <4726E9AB.4050209@FreeBSD.org> In-Reply-To: <4726E9AB.4050209@FreeBSD.org> Content-Type: text/plain; charset=us-ascii; 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-all@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: CVS commit messages for the entire tree List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 30 Oct 2007 08:53:19 -0000 Kris Kennaway wrote: > > Yes, but only if we can do it in a way that does not reduce performance > in other cases. As you know, and as I mentioned already to Dan, this is > architecturally hard. > >>> also this commit does not change mutex_self_lock() to handle the >>> PTHREAD_MUTEX_ADAPTIVE_NP, what is the PTHREAD_MUTEX_ADAPTIVE_NP >>> definition when the mutex is already locked by the currect thread ? >>> deadlock or return error code ? > > > As I mentioned in the commit, it is defined to be the same as the > default "errorcheck" type in all other aspects than the adaptive > spinning. However I see I missed a case: > > Index: thread/thr_mutex.c > =================================================================== > RCS file: /home/ncvs/src/lib/libthr/thread/thr_mutex.c,v > retrieving revision 1.55 > diff -u -r1.55 thr_mutex.c > --- thread/thr_mutex.c 29 Oct 2007 21:01:47 -0000 1.55 > +++ thread/thr_mutex.c 30 Oct 2007 08:16:18 -0000 > @@ -558,6 +558,7 @@ > > switch (m->m_type) { > case PTHREAD_MUTEX_ERRORCHECK: > + case PTHREAD_MUTEX_ADAPTIVE_NP: > if (abstime) { > clock_gettime(CLOCK_REALTIME, &ts1); > TIMESPEC_SUB(&ts2, abstime, &ts1); > >> 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. > 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. I remembered mysql uses this macro to initialize spin mutex, and you indead needs a patch to let it work, 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. Regards, David Xu