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

[Takashi Iwai <tiwai@suse.de>]

On Tue, 17 May 2016 17:28:43 +0200,
Ricard Wanderlof wrote:
> 
> 
> On Tue, 17 May 2016, Takashi Iwai wrote:
> 
> > No, my proposal is to make all snd_atomic_*() NOP unless a configure
> > option is passed.
> 
> Ok, I must honestly say I hadn't studied the actual pcm_plugin.c code in 
> great detail before (I didn't create the patch but was in the train of 
> discussion at the time). I had just assumed that the w->begin and w->end 
> variables were some form of counters used outside the atomic functions, 
> but I can see now that they are not.
> 
> Looking at it now it appears that all this atomic stuff is trying to 
> accomplish is to avoid the (sole) read in snd_pcm_plugin_status() from 
> happening during one of the many potential writes in the other functions 
> in the file, and the only reason for that in turn seems to be to get the 
> acceses to *pcm->appl.ptr and *pcm->hw.ptr as well as other things needed 
> for the return snd_pcm_status_t* consistent.
> 
> But that in turn means that fixing the atomicity of the w->begin and 
> w->end accesses as proposed in the patch just glosses over that particular 
> implementation issue; if indeed something is calling the functions doing 
> the writes concurrently, something else is bound to get screwed up, unless 
> by chance the two calls touch different variables, or the timing is such 
> that two things aren't touched at the same time.
> 
> Right now it feels a bit uncomfortable to fix it in this way. It's just 
> luck if it doesn't die somewhere else. Or am I missing something (highly 
> likely)?

Right, the current atomic ops usage is only covering the very light
locking scheme that might casually work for some cases.  But it never
works for heavy use, and a more proper way of concurrent accesses
would be needed in anyway.  That's the reason I proposed to drop off
these.  This makes things looking safe although they are not really
usable.

But how dangerous is this move?  I'm not 100% sure so far...


Takashi