[pulseaudio-discuss] pa_once can run twice?

[Dalleau, Frederic]

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.

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.

Regards,
Frédéric
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-once-Fix-concurrency-issue-in-pa_once.patch
Type: application/octet-stream
Size: 4096 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/pulseaudio-discuss/attachments/20120425/46b92c57/attachment.obj>