Re: bzr commit into mysql-6.0-perfschema branch (marc.alff:3167)

[Guilhem Bichot]

Hello Marc,

Marc Alff a écrit, Le 07.06.2009 20:17:
> #At file:///home/malff/BZR-TREE/mysql-6.0-perfschema/ based on
> revid:marc.alff@stripped
> 
>  3167 Marc Alff	2009-06-07
>       Cleanup of PFS_atomic

> -#ifdef PFS_ATOMIC_MODE_MUTEX
> -static pthread_mutex_t PFS_atomic::m_mutex_array[256];
> +#ifdef SAFE_MUTEX
> +/*
> +  Using SAFE_MUTEX is impossible, because of recursion.
> +  - code locks mutex X
> +    - P_S records the event
> +      - P_S needs an atomic counter A
> +        - safe mutex called for m_mutex[hash(A)]
> +          - safe mutex allocates/free memory
> +            - safe mutex locks THR_LOCK_malloc
> +              - P_S records the event
> +                - P_S needs an atomic counter B
> +                  - safe mutex called for m_mutex[hash(B)]
> +
> +  When hash(A) == hash(B), safe_mutex complains rightly that
> +  the mutex is already locked.
> +  In some cases, A == B, in particular for events_waits_history_long_index.
> +
> +  In short, the implementation of PFS_atomic should not cause events
> +  to be recorded in the performance schema.
> +
> +  Also, because SAFE_MUTEX redefines pthread_mutex_t, etc,
> +  this code is not inlined in pfs_atomic.h, but located here in pfs_atomic.cc.
> +
> +  What is needed is a plain, unmodified, pthread_mutex_t.

Good catch with the recursive problem.
As you have seen, Serg and I recommend using my_atomic_rwlock_t instead 
of pthread_mutex_t, because my_atomic_rwlock_t has been designed 
precisely for cases where atomic ops are not implemented.

Now, if we changed the plain unmodified pthread_mutex_t above to 
my_atomic_rwlock_t, we would reintroduce the problem because
if SAFE_MUTEX is defined, my_atomic_rwlock_t is safe_mutex_t.
There are solutions:
1) we can make my_atomic_rwlock_t always be a plain unmodified POSIX 
pthread_mutex_t.
2) or we can make safe_mutex never generate any events; this one is more 
complicated because it uses my_malloc() and my_hash_insert() (if doing 
deadlock detection). We could disable deadlock detection for 
THR_LOCK_malloc; we could also not instrument THR_LOCK_malloc, which is 
a Serg's idea, where we would not instrument debug-specific objects so 
that the execution flow of the debug binary is closer to the one of the 
release binary. We could also turn off instrumentation for the thread 
when entering safe_mutex_lock() and turn it on again when leaving the 
function.

Don't change your code to use my_atomic_rwlock_t for now, let's first 
see what Serg, who authored my_atomic_rwlock_t, would recommend.

-- 
Mr. Guilhem Bichot <guilhem@stripped>
Sun Microsystems / MySQL, Lead Software Engineer
Bordeaux, France
www.sun.com / www.mysql.com