Date: Tue, 30 Oct 2007 16:54:08 +0800 From: David Xu <davidxu@FreeBSD.org> To: Kris Kennaway <kris@FreeBSD.org> 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 Message-ID: <4726F130.2060709@freebsd.org> In-Reply-To: <4726E9AB.4050209@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>
index | next in thread | previous in thread | raw e-mail
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
home |
help
Want to link to this message? Use this
URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?4726F130.2060709>