[alsa-devel] [PATCH] alsa-lib:Make snd_atomic_write_* truly atomic

[Takashi Iwai]

On Tue, 17 May 2016 15:29:33 +0200,
Ricard Wanderlof wrote:
> 
> 
> On Tue, 17 May 2016, Takashi Iwai wrote:
> 
> > There is a manual lock loop with snd_atomic_read_wait(), as found in
> > pcm_plugin.c.  So, removing the stuff may potentially damage some apps
> > that wrongly assume the thread safety.
> 
> Yes, I think could be true. Certainly GStreamer seems to assume this.
> 
> > Now I've been considering this issue again, I'm inclined to suggest:
> > let's begin with deprecating the atomic stuff there, but opt-in via
> > via a configure option for a while, in case someone still really needs
> > it.
> > 
> > By making it optional, it's fine to take your patch and cleanup the
> > old code.  Then we don't have to think of the compatibility with the
> > old toolchain so much.
> 
> I'm trying to consider how this would be done in practice. Basically, add 
> a configuration option like '--with-atomic' with the codec in iatomic.h 
> looking something like:
> 
>   static __inline__ void snd_atomic_write_begin(snd_atomic_write_t *w)
>   {
> + #ifdef USE_ATOMIC
> +   __atomic_add_fetch(&w->begin, 1,__ATOMIC_SEQ_CST);
> + #else
> -   w->begin++;
> + #endif
> -   wmb();
>   }
> 
> Thus removing the (useless?) wmb() in any case, and going to w->begin++ in 
> the non-atomic case, and __atomic_add_fetch() in the atomic case?

No, my proposal is to make all snd_atomic_*() NOP unless a configure
option is passed.

> However, this does mean that those that need/expect atomic operations must 
> explicitly add a new configuration option, but perhaps that is ok, as one 
> could argue that the applications that need it are buggy to start with. 

Right.  We can spot out whether the application bugs, and yet some
excuse for breakage :)

> Also, it would prohibit building the building of alsa-lib for such 
> applications with gcc < 4.7 (when __atomic_add_fetch was introduced), but 
> perhaps that's more of an incentive than anything else. :-)

Yep.


Takashi