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>
