cvs commit: src/lib/libthr/thread thr_mutex.c src/lib/libkse/thread thr_mutex.c src/include pthread.h

[David Xu]

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