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

[Marc Alff]

Hi Guilhem

Guilhem Bichot wrote:
> 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.

That is also my analysis.

> There are solutions:
> 1) we can make my_atomic_rwlock_t always be a plain unmodified POSIX
> pthread_mutex_t.

I think this is the only viable solution.

If my_atomic_rwlock_t uses safe_mutex, it means none of the code
executed by safe_mutex can ever use an atomic operation ... that's a
pretty serious, and undocumented, limitation.

> 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.

I would rather not use safe_mutex at all.

At some point, the code has to cut dependencies instead of
re-introducing cycles all other the place.

If an "atomic" operation end up executing hundreds of lines of code,
including debug traces, file io, memory allocations, performance schema
instrumentation (disabled or not), safe mutex, dtrace calls, etc, that
looks like a serious design issue.
Sure, we could disable the performance schema during this recursive
call, but other things will show up and cause problems, for the same
reasons (un anticipated recursion).

What needs to be fixed is the root cause (the recursion), not the
symptoms (by disabling a lot of things that broke).

Beside, the sequence:
- lock()
- 1 atomic cas / add / load / store / etc
- unlock()

is safe, and will not create dead locks between mutexes, so there is no
benefit to gain from safe_mutex.

> 
> 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.
> 

Ok.

Regards,
-- Marc