Date: Tue, 30 Oct 2007 09:22:03 +0100 From: Kris Kennaway <kris@FreeBSD.org> To: Daniel Eischen <deischen@freebsd.org> Cc: cvs-src@freebsd.org, src-committers@freebsd.org, David Xu <davidxu@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 Message-ID: <4726E9AB.4050209@FreeBSD.org> In-Reply-To: <Pine.GSO.4.64.0710292207140.19572@sea.ntplx.net> References: <200710292101.l9TL1mAE049561@repoman.freebsd.org> <47268F17.1000106@freebsd.org> <Pine.GSO.4.64.0710292207140.19572@sea.ntplx.net>
next in thread | previous in thread | raw e-mail | index | archive | help
Daniel Eischen wrote:
> On Tue, 30 Oct 2007, David Xu wrote:
>
>> Kris Kennaway wrote:
>>> kris 2007-10-29 21:01:47 UTC
>>>
>>> FreeBSD src repository
>>>
>>> Modified files:
>>> lib/libthr/thread thr_mutex.c lib/libkse/thread
>>> thr_mutex.c include pthread.h Log:
>>> Add a new "non-portable" mutex type, PTHREAD_MUTEX_ADAPTIVE_NP. This
>>> is also implemented in glibc and is used by a number of existing
>>> applications (mysql, firefox, etc).
>>> This mutex type is a default mutex with the additional property that
>>> it spins briefly when attempting to acquire a contested lock, doing
>>> trylock operations in userland before entering the kernel to block if
>>> eventually unsuccessful.
>>> The expectation is that applications requesting this mutex type know
>>> that the mutex is likely to be only held for very brief periods, so it
>>> is faster to spin in userland and probably succeed in acquiring the
>>> mutex, than to enter the kernel and sleep, only to be woken up almost
>>> immediately. This can help significantly in certain cases when
>>> pthread mutexes are heavily contended and held for brief durations
>>> (such as mysql).
>>> Spin up to 200 times before entering the kernel, which represents
>>> only
>>> a few us on modern CPUs. No performance degradation was observed with
>>> this value and it is sufficient to avoid a large performance drop in
>>> mysql performance in the heavily contended pthread mutex case.
>>> The libkse implementation is a NOP.
>>> Reviewed by: jeff
>>> MFC after: 3 days
>>> Revision Changes Path
>>> 1.41 +2 -0 src/include/pthread.h
>>> 1.54 +3 -0 src/lib/libkse/thread/thr_mutex.c
>>> 1.55 +29 -0 src/lib/libthr/thread/thr_mutex.c
>>>
>>
>> I am not sure PTHREAD_MUTEX_ADAPTIVE_NP is a correct solution, in fact
>> I think this is Linux crap, shouldn't PTHREAD_PRIO_PROTECT and
>> PTHREAD_PRIO_INHERIT mutex be adaptivly spinned ?
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.
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?4726E9AB.4050209>