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>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?4726F130.2060709>