[vlc-devel] [GCI] last.fm access module

[Rémi Denis-Courmont]

Le samedi 8 janvier 2011 15:18:14 Jakub Wieczorek, vous avez écrit :
> 2011/1/8 Rémi Denis-Courmont <remi at remlab.net>:
> > Le samedi 8 janvier 2011 01:22:37 Jakub Wieczorek, vous avez écrit :
> >> Any update on this? Is it ready to go in?
> > 
> >  - b_error is not protected correctly.
> 
> Yes, that's intentional. To protect this correctly I'd have to do a
> mutex lock/unlock on each Read() invocation, which may harm
> performance, to the best of my knowledge.

You have to protect it somehow. If you really fear the performance cost, use 
an VLC atomic variable. But I am not convinced you will ever notice the 
difference.

> And I'm also under the impression a boolean datum doesn't need to
> be protected in certain cases such as this one.

There are several problems with atomicity:

- Different architecture support different sizes. There is no warranty that 
sizeof(bool) and signedness of bool matches the CPU architecture constraints.

- At the very least you need a read memory barrier to synchronize the memory 
(e.g. stall the per-CPU cache) with other CPU cores; you have no warranty that 
both threads run on the same CPU.

- Compiler optimizations can do really unexpected things. For instance the 
machine code often keeps values cached in a register if the optimizing 
compiler notices that the value cannot be modified in a code path.

> Please let me know if I'm mistaken.
> 
> >  - the mutex seems leaked on cancellation
> 
> Right. I guess that's what mutex_cleanup_push is for? I'll take a look.
> Thanks.

Yes. When a thread is cancelled at vlc_cond_wait(), the mutex is locked before 
cancellation is acted on. We don't leverage this "feature" ever in VLC, but 
it's the POSIX convention.

-- 
Rémi Denis-Courmont
http://www.remlab.net/
http://fi.linkedin.com/in/remidenis