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

[Ricard Wanderlof]

On Thu, 12 May 2016, Takashi Iwai wrote:

> > The code as it currently stands has macros like
> > 
> > #define mb()    __asm__ __volatile__ ("lock; addl $0,0(%%esp)": : :"memory")
> > #define rmb()   mb()
> > #define wmb()   __asm__ __volatile__ ("": : :"memory")
> > 
> > which surely are gcc specific. So surely it can't be a requirement that 
> > the patch work on other compilers as well?
> 
> Well, it's not the requirement.  It's just a question.

Ok.

> > It is relevant to verify which gcc versions support the builtins used in 
> > the patch however. I believe the kernel has requirements on which gcc 
> > versions it can be compiled with, but perhaps we need to be more lenient 
> > with userspace?
> 
> Yes.  And I bet that the kernel can be still built with a gcc version
> that doesn't support the new atomic primitive, too.

Yes, that would need to be looked in to of course.

> > In total, the various snd_atomic_* macros are used ~20 times in pcm_rate.c 
> > and ~30 times in pcm_plugin.c . If the atomic functionality is indeed 
> > necessary (and it appears to be), it would seem worth while abstracting 
> > the nitty gritty using macros I think, rather than cluttering up the code.
> 
> In other words, it's used only there.  Most of other codes don't care
> about it.  So, the usefulness of the atomic operation there is really
> doubtful.
>
> [...]
>
> > One would expect otherwise that this issue would have come up much 
> > sooner.
> 
> Yeah, that's why I stated the primary question.  The atomic operation
> there is mostly cosmetic, not really helpful.  In the whole other
> operation, we don't guarantee the thread safety at all.  So,
> implementing the atomic op only in a very small part of the whole code
> appears like a superfluous optimization to me.
> 
> That said: my preferred option is to scratch the whole atomic thing.

That's a valid point, still it was once added in a specific commit 
(a02e742609bb0ed9610809fc62b5f927dd086e62), so one would think it was 
added for a reason. It was a long time ago now, 15 years it seems, so of 
course it could have been part of larger thread safety operation that was 
never completed or in the end deemed unnecessary - the commit message 
itself doesn't provide many helpful hints.

The fact that we have seen a race condition which is fixed by the patch 
under discussion indicates that it is in fact requred, although it could 
also be that the patch simply shifts the timing around so that the 
potential real problem is not visible.

/Ricard
-- 
Ricard Wolf Wanderlöf                           ricardw(at)axis.com
Axis Communications AB, Lund, Sweden            www.axis.com
Phone +46 46 272 2016                           Fax +46 46 13 61 30