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

[Emil Velikov]

On 26 March 2015 at 14:50, Chad Versace <chad.versace at intel.com> 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.
I don't have particular reason for wanting to destroy the mutex, other
than being a "good citizen". On the topic of leaking memory, I don't
think that it will be a major concern even if the c11
implementation(s) (unlike the pthreads one) allocates memory.

> 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 was thinking about thread 2 diving into display_connect() and
attempting to lock a destroyed mutex. Afaics in such case the function
will return EINVAL, which I naively assumed that is a undefined
behaviour. Although with the failed locking around id_counter I assume
another problem could have happened.

Cheers
Emil