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