On Tue, 17 May 2016 14:11:43 +0200, Ricard Wanderlof wrote:
On Fri, 13 May 2016, Takashi Iwai wrote:
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.
The question is, if we decide to remove this atomic stuff, how do we verify that it actually causes no problems, on all architectures, etc?
It's not what one can show in 100%, as you know. And, it's the reason why the code is still left over years...
We have come up upon this problem in conjunction with GStreamer. It appears that GStreamer uses one thread for the streaming and a separate one for the device management (open/close) on the same device handle, which triggers the problem. So it appears that GStreamer falsly is assuming some form of thread safety here.
The atomic stuff is so specific to a couple of files, it seems as if it was added for a specific purpose, such as this particular case. In the other hand, looking at the git history of include/iatomic.h, it's gone through a number of cleanups over the years, lately mostly to remove originally buggy code, and what is remaining I guess like you say is just what nobody dared remove.
On the other hand, as the code stands, I can't really understand how it enforces atomicity. The read and write memory barriers just guarantee ordering, but there's nothing to stop a context switch from occurring in the middle of a ++ operation. wmb() and rmb() might influence timing though so that code at least might behave differently with those operations in place.
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.
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.
Takashi