[pulseaudio-discuss] pa_once can run twice?

[David Henningsson]

On 04/25/2012 01:09 PM, Dalleau, Frederic wrote:
> Hi David,
>
> On Tue, Apr 24, 2012 at 3:01 PM, David Henningsson
> <david.henningsson at canonical.com>  wrote:
>> On 04/23/2012 05:03 PM, Dalleau, Frederic wrote:
>>>
>>> Hi David,
>>>
>>> On Fri, Apr 20, 2012 at 4:44 PM, David Henningsson
>>> <david.henningsson at canonical.com>    wrote:
>>>>
>>>> While researching a bug I came across something that might be a bug in
>>>> the
>>>> pa_once logic, but this stuff is tricky, so I might also be missing
>>>> something.
>>>>
>>>> Imagine this:
>>>>
>>>>   * Thread 1 runs pa_once_begin, succeeds and starts running the payload
>>>> (i e
>>>> the code that should only run once).
>>>>   * Thread 2 starts running pa_once_begin, but only the first row. We're
>>>> now
>>>> right *before* pa_atomic_inc(&control->ref) but *after*
>>>> pa_atomic_load(&control->done).
>>>>   * Thread 1 finishes the payload, runs pa_once_done which sets
>>>> control->done
>>>> and frees the mutex.
>>>>   * Thread 2 continues, pa_once_begin succeeds and the payload is now run
>>>> a
>>>> second time!
>>>>
>>>
>>> After reading your mail, I made some experiments by adding a usleep() call
>>> in Thread 1 between pa_atomic_load(&control->done) and
>>> pa_atomic_inc(&control->ref)
>>> and that failed once-test 100% of time.
>>>
>>> I reverted the usleep and made another experiment using 50000 iterations
>>> in
>>> once-test and it just failed.
>>>
>>> Good catch !
>>
>>
>> I tried to look up implementation/algorithm suggestions, and for the ones I
>> found [1], there was no freeing of the mutex. Without freeing, the code
>> becomes simpler. The attached patch is a version of that. I've just tried a
>> simple once-test (but do feel free to run it 50000 times :-) ).
>>
>> But of course, now we leak a mutex. But that's what we already do with the
>> static mutexes we use in a few places already, so maybe it doesn't matter
>> much?
>
> Your patch has passed the 50000 iteration test, but I'm reluctant in the idea of
> leaking memory since solutions exists.
>
> I have attached a version which uses reference counting to free the
> mutex. I can't find
> any significant difference in cpu usage, but it must be double checked
> for correctness.

Right, so it's a trade-off between keeping a mutex allocated for my 
version, and three extra atomic ops for your version. Maybe it doesn't 
matter much and we're essentially bikeshedding?

> libatomic_ops documentation suggest the following :
> http://www.hpl.hp.com/research/linux/atomic_ops/example.php4
> With a static mutex, it uses a few extra bytes (pthread_mutex_t is 24
> bytes on my box), but it may also offer the best performances of all:
> no allocation
> and no busy loop waiting for mutex creation. I'm giving it a try but
> it needs a bit
> of plumbing.

That looks slightly more elegant than our solutions IMO. Perhaps we can 
use pa_static_mutex_get function for the static mutex?

-- 
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic