[waffle] [PATCH v2 2/3] wcore: rework non-compatible mutex initialization

[Jose Fonseca]

On 26/03/15 14:50, Chad Versace wrote:
> Quoting Emil Velikov (2015-03-25 07:30:00)
>> On 24 March 2015 at 17:37, Jose Fonseca <jfonseca at vmware.com> wrote:
>>> Is wcore_display_teardown called only once, or when destroying each display?
>>>
>>> If the latter, then it's not safe to call `mtx_destroy(&mutex)` on
>>> `wcore_display_teardown`.  Otherwise when  wcore_display_init is called
>>> next, wcore_display_init_once() will not be called a 2nd time, hence mutex
>>> will stay invalid.
>>>
>>>
>>> This should probably done at waffle_teardown.
>
> Right. The mutex should be destroyed, if anywhere, in waffle_teardown().
> But, we should probably not destroy it at all, because of my next
> suggestion...
>
>>> BTW, if the mutex was initialized at waffle_init, then once_flag wouldn't
>>> even be necessary.
>
> Technically correct, but I eventually want to deprecate waffle_init(),
> moving its platform parameter to a new waffle_display_connect()
> function. Using a once_flag in wcore_display_init() fits better with
> that longterm goal. Let's keep Emil's once_flag in this patch.
>
> If use a once_flag, then I believe there is no safe place to
> destroy the mutex, because we can't re-initialize the mutex by calling
> wcore_display_init_once() a second time.
>
> Which leads to the question: Emil, what benefit do you expect from
> destroying the mutex? If Waffle were continuously creating mutexes, then
> Waffle would need to destroy them too to prevent leaks. But Waffle only
> creates a small, fixed number of global mutexes during the process's
> lifetime. And "leaking" them doesn't lead to ongoing memory loss.
> Moreover, with pthreads... see my comments below.
>
>> Indeed you're bang on the spot. id_counter should be locked throughout
>> a series of display_{connect, disconnect}, thus we should push
>> mtx_{init,destroy} up to waffle_{init,teardown}. What worries me is
>> that none of the tests (ran in valgrind) point out any issues.
>
> I could be wrong, but I believe pthread_mutex_create/destroy don't
> allocate/free memory. They just initialize/reset the fields in the
> pthread_mutex_t struct. (Otherwise, how would PTHREAD_MUTEX_INITIALIZER
> work?). That may explain why you didn't see the expected Valgrind
> errors, because no allocation took place.

I think it's fine to let it leak on destroy, but just FYI on Windows 
mutex (ie, CriticalSections) may allocate/free memory on certain 
configurations.  IIRC, they can allocate memory for stack backtraces, 
for debugging purposes.

The approach we use to statically initialize critical sections on 
windows is unofficial.  Read 
http://locklessinc.com/articles/pthreads_on_windows/ for the gritty details.

Jose