alsa-lib:Make snd_atomic_write_* truly atomic
PATCH 1/1 alsa-lib:Make snd_atomic_write_* truly atomic
Under some usecases a race condition appears inside the snd_atomic_write_* functions. The 'begin' and 'end' variables are updated with the ++ operator which is not atomic but needs to be. This can be achieved with the gcc atomic_* built-ins. Combined with __ATOMIC_SEQ_CST as the memory model, memory barriers are introduced so those can also be removed from the code.
--- alsa-lib/include/iatomic.h | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)
alsa-lib/include/iatomic.h +2 -4
@@ -140,14 +140,12 @@ static __inline__ void snd_atomic_write_init(snd_atomic_write_t *w)static __inline__void snd_atomic_write_begin (snd_atomic_write_t *w) { - w->begin++; -wmb (); +__atomic_add_fetch (&w->begin,1 ,__ATOMIC_SEQ_CST); }static __inline__void snd_atomic_write_end (snd_atomic_write_t *w) { -wmb (); - w->end++; +__atomic_add_fetch (&w->end,1 , __ATOMIC_SEQ_CST); } static __inline__ void snd_atomic_read_init(snd_atomic_read_t *r,
snd_atomic_write_t *w)
Signature · 5 lines
2.1.4 _______________________________________________ Alsa-devel mailing list [email protected] http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
Quoted 11 lines · Andreas x Larsson
Under some usecases a race condition appears inside the snd_atomic_write_* functions. The 'begin' and 'end' variables are updated with the ++ operator which is not atomic but needs to be. This can be achieved with the gcc atomic_* built-ins. Combined with
__ATOMIC_SEQ_CSTas the memory model, memory barriers are introduced so those can also be removed from the code.Signed-off-by Andreas x Larsson <[email protected]>Signed-off-by Arvid Nihlgård Lindell <[email protected]>
Isn't this dependent on gcc version?
And, if we use the gcc atomic operation, the whole macros should be rewritten in another way -- or consider to drop the whole. It's only partly used in pcm_rate.c and pcm_plugin.c in anyway.
thanks,
Takashi
Quoted 25 lines
--- alsa-lib/include/iatomic.h | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)alsa-lib/include/iatomic.h +2
-4@@ -140,14 +140,12 @@ static __inline__ void snd_atomic_write_init(snd_atomic_write_t *w)static __inline__void snd_atomic_write_begin (snd_atomic_write_t *w) { - w->begin++; -wmb (); +__atomic_add_fetch (&w->begin,1 ,__ATOMIC_SEQ_CST); }static __inline__void snd_atomic_write_end (snd_atomic_write_t *w) { -wmb (); - w->end++; +__atomic_add_fetch (&w->end,1 , __ATOMIC_SEQ_CST); }
Quoted 8 lines
static
__inline__void snd_atomic_read_init(snd_atomic_read_t *r,snd_atomic_write_t*w)Signature · 5 lines
2.1.4 _______________________________________________ Alsa-devel mailing list [email protected] http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
_______________________________________________
Alsa-devel mailing list
[email protected]
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
Quoted 11 lines · Takashi Iwai
Quoted 9 lines · Andreas x Larsson
Under some usecases a race condition appears inside the snd_atomic_write_* functions. The 'begin' and 'end' variables are updated with the ++ operator which is not atomic but needs to be. This can be achieved with the gcc atomic_* built-ins. Combined with
__ATOMIC_SEQ_CSTas the memory model, memory barriers are introduced so those can also be removed from the code.
Quoted 4 lines
Quoted 2 lines
Signed-off-by Andreas x Larsson <[email protected]>Signed-off-by Arvid Nihlgård Lindell <[email protected]>Isn't this dependent on gcc version?
Do you mean that the problem is dependent on gcc version, or the solution? If the problem only occurs with certain gcc versions, but the _atomic_* functions are the proper solution in all cases, then I would think that is the way to go.
Quoted 3 lines
And, if we use the gcc atomic operation, the whole macros should be rewritten in another way -- or consider to drop the whole. It's only partly used in
pcm_rate.candpcm_plugin.cin anyway.
Do you mean that the inline functions should be replaced with #define macros, or that they should be omitted entirely and the resulting code be integrated into the relevant places in the .c files?
/Ricard
Quoted 39 lines
Quoted 26 lines
--- alsa-lib/include/iatomic.h | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)alsa-lib/include/iatomic.h +2
-4@@ -140,14 +140,12 @@ static __inline__ void snd_atomic_write_init(snd_atomic_write_t *w)static __inline__void snd_atomic_write_begin (snd_atomic_write_t *w) { - w->begin++; -wmb (); +__atomic_add_fetch (&w->begin,1 ,__ATOMIC_SEQ_CST); }static __inline__void snd_atomic_write_end (snd_atomic_write_t *w) { -wmb (); - w->end++; +__atomic_add_fetch (&w->end,1 , __ATOMIC_SEQ_CST); }Quoted 8 lines
static
__inline__void snd_atomic_read_init(snd_atomic_read_t *r,snd_atomic_write_t*w)Signature · 5 lines
2.1.4 _______________________________________________ Alsa-devel mailing list [email protected] http://mailman.alsa-project.org/mailman/listinfo/alsa-devel_______________________________________________
Alsa-devel mailing list
[email protected]
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
Signature · 3 lines
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
Quoted 18 lines · Ricard Wanderlof
Quoted 14 lines · Takashi Iwai
Quoted 11 lines · Andreas x Larsson
Under some usecases a race condition appears inside the snd_atomic_write_* functions. The 'begin' and 'end' variables are updated with the ++ operator which is not atomic but needs to be. This can be achieved with the gcc atomic_* built-ins. Combined with
__ATOMIC_SEQ_CSTas the memory model, memory barriers are introduced so those can also be removed from the code.Signed-off-by Andreas x Larsson <[email protected]>Signed-off-by Arvid Nihlgård Lindell <[email protected]>
Quoted 6 lines
Quoted 1 line
Isn't this dependent on gcc version?
Do you mean that the problem is dependent on gcc version, or the solution? If the problem only occurs with certain gcc versions, but the _atomic_* functions are the proper solution in all cases, then I would think that is the way to go.
I meant the solution, your patch. Does it work with older version of gcc, and any other compilers than gcc?
Quoted 7 lines
Quoted 3 lines
And, if we use the gcc atomic operation, the whole macros should be rewritten in another way -- or consider to drop the whole. It's only partly used in
pcm_rate.candpcm_plugin.cin anyway.Do you mean that the inline functions should be replaced with #define macros, or that they should be omitted entirely and the resulting code be integrated into the relevant places in the .c files?
No, no, I questioned about the usefulness of snd_atomic_*() themselves. Does it make sense to provide this thin wrapper? And, above all, does it make sense at all to use this? When looking at the current code, it's used in only very limited places.
Takashi
Quoted 29 lines
/Ricard
Quoted 26 lines
Quoted 26 lines
--- alsa-lib/include/iatomic.h | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)alsa-lib/include/iatomic.h +2
-4@@ -140,14 +140,12 @@ static __inline__ void snd_atomic_write_init(snd_atomic_write_t *w)static __inline__void snd_atomic_write_begin (snd_atomic_write_t *w) { - w->begin++; -wmb (); +__atomic_add_fetch (&w->begin,1 ,__ATOMIC_SEQ_CST); }static __inline__void snd_atomic_write_end (snd_atomic_write_t *w) { -wmb (); - w->end++; +__atomic_add_fetch (&w->end,1 , __ATOMIC_SEQ_CST); }
Quoted 19 lines
Quoted 13 lines
Quoted 8 lines
static
__inline__void snd_atomic_read_init(snd_atomic_read_t *r,snd_atomic_write_t*w)Signature · 5 lines
2.1.4 _______________________________________________ Alsa-devel mailing list [email protected] http://mailman.alsa-project.org/mailman/listinfo/alsa-devel_______________________________________________
Alsa-devel mailing list
[email protected]
http://mailman.alsa-project.org/mailman/listinfo/alsa-develSignature · 3 lines
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
_______________________________________________
Alsa-devel mailing list
[email protected]
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
Quoted 16 lines · Takashi Iwai
Quoted 14 lines · Ricard Wanderlof
Quoted 13 lines
Quoted 11 lines
Under some usecases a race condition appears inside the snd_atomic_write_* functions. The 'begin' and 'end' variables are updated with the ++ operator which is not atomic but needs to be. This can be achieved with the gcc atomic_* built-ins. Combined with
__ATOMIC_SEQ_CSTas the memory model, memory barriers are introduced so those can also be removed from the code.Signed-off-by Andreas x Larsson <[email protected]>Signed-off-by Arvid Nihlgård Lindell <[email protected]>Isn't this dependent on gcc version?
Quoted 7 lines
Quoted 4 lines
Do you mean that the problem is dependent on gcc version, or the solution? If the problem only occurs with certain gcc versions, but the _atomic_* functions are the proper solution in all cases, then I would think that is the way to go.
I meant the solution, your patch. Does it work with older version of gcc, and any other compilers than gcc?
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?
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?
Quoted 4 lines
Quoted 4 lines
Quoted 3 lines
And, if we use the gcc atomic operation, the whole macros should be rewritten in another way -- or consider to drop the whole. It's only partly used in
pcm_rate.candpcm_plugin.cin anyway.
Quoted 8 lines
Quoted 3 lines
Do you mean that the inline functions should be replaced with #define macros, or that they should be omitted entirely and the resulting code be integrated into the relevant places in the .c files?
No, no, I questioned about the usefulness of snd_atomic_*() themselves. Does it make sense to provide this thin wrapper? And, above all, does it make sense at all to use this? When looking at the current code, it's used in only very limited places.
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.
But that seems to be a different issue altogether than what the patch is intended to solve.
However, one thing struck me. In our case we've seen the problem that the patch is intended to solve on the MIPS architecture. In this case we get the following definitions for mb() et al:
/* Generic __sync_synchronize is available from gcc 4.1 */
#define mb() __sync_synchronize()
#define rmb() mb()
#define wmb() mb()
Could in fact the problem be that __sync_synchronize() is buggy on this platform, if the problem hasn't turned up anywhere else? One would expect otherwise that this issue would have come up much sooner.
/Ricard
Quoted 45 lines
Quoted 44 lines
Quoted 38 lines
Quoted 25 lines
--- alsa-lib/include/iatomic.h | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)alsa-lib/include/iatomic.h +2
-4@@ -140,14 +140,12 @@ static __inline__ void snd_atomic_write_init(snd_atomic_write_t *w)static __inline__void snd_atomic_write_begin (snd_atomic_write_t *w) { - w->begin++; -wmb (); +__atomic_add_fetch (&w->begin,1 ,__ATOMIC_SEQ_CST); }static __inline__void snd_atomic_write_end (snd_atomic_write_t *w) { -wmb (); - w->end++; +__atomic_add_fetch (&w->end,1 , __ATOMIC_SEQ_CST); }Quoted 8 lines
static
__inline__void snd_atomic_read_init(snd_atomic_read_t *r,snd_atomic_write_t*w)Signature · 5 lines
2.1.4 _______________________________________________ Alsa-devel mailing list [email protected] http://mailman.alsa-project.org/mailman/listinfo/alsa-devel_______________________________________________
Alsa-devel mailing list
[email protected]
http://mailman.alsa-project.org/mailman/listinfo/alsa-develSignature · 3 lines
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
Signature · 7 lines
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 _______________________________________________ Alsa-devel mailing list [email protected] http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
Quoted 28 lines · Ricard Wanderlof
Quoted 16 lines · Takashi Iwai
Quoted 14 lines · Ricard Wanderlof
Quoted 13 lines
Quoted 11 lines
Under some usecases a race condition appears inside the snd_atomic_write_* functions. The 'begin' and 'end' variables are updated with the ++ operator which is not atomic but needs to be. This can be achieved with the gcc atomic_* built-ins. Combined with
__ATOMIC_SEQ_CSTas the memory model, memory barriers are introduced so those can also be removed from the code.Signed-off-by Andreas x Larsson <[email protected]>Signed-off-by Arvid Nihlgård Lindell <[email protected]>Isn't this dependent on gcc version?
Quoted 7 lines
Quoted 4 lines
Do you mean that the problem is dependent on gcc version, or the solution? If the problem only occurs with certain gcc versions, but the _atomic_* functions are the proper solution in all cases, then I would think that is the way to go.
I meant the solution, your patch. Does it work with older version of gcc, and any other compilers than gcc?
Quoted 8 lines
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.
Quoted 4 lines
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.
Quoted 13 lines
Quoted 4 lines
Quoted 4 lines
Quoted 3 lines
And, if we use the gcc atomic operation, the whole macros should be rewritten in another way -- or consider to drop the whole. It's only partly used in
pcm_rate.candpcm_plugin.cin anyway.Quoted 8 lines
Quoted 3 lines
Do you mean that the inline functions should be replaced with #define macros, or that they should be omitted entirely and the resulting code be integrated into the relevant places in the .c files?
No, no, I questioned about the usefulness of snd_atomic_*() themselves. Does it make sense to provide this thin wrapper? And, above all, does it make sense at all to use this? When looking at the current code, it's used in only very limited places.
Quoted 4 lines
In total, the various snd_atomic_* macros are used ~20 times in
pcm_rate.cand ~30 times inpcm_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.
Quoted 9 lines
But that seems to be a different issue altogether than what the patch is intended to solve.
However, one thing struck me. In our case we've seen the problem that the patch is intended to solve on the MIPS architecture. In this case we get the following definitions for
mb()et al:/* Generic
__sync_synchronizeis available from gcc 4.1 */
Quoted 7 lines
#define
mb()__sync_synchronize()
#definermb()mb()
#definewmb()mb()Could in fact the problem be that
__sync_synchronize()is buggy on this platform, if the problem hasn't turned up anywhere else? 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.
thanks,
Takashi
Quoted 43 lines
/Ricard
Quoted 39 lines
Quoted 39 lines
Quoted 38 lines
Quoted 25 lines
--- alsa-lib/include/iatomic.h | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)alsa-lib/include/iatomic.h +2
-4@@ -140,14 +140,12 @@ static __inline__ void snd_atomic_write_init(snd_atomic_write_t *w)static __inline__void snd_atomic_write_begin (snd_atomic_write_t *w) { - w->begin++; -wmb (); +__atomic_add_fetch (&w->begin,1 ,__ATOMIC_SEQ_CST); }static __inline__void snd_atomic_write_end (snd_atomic_write_t *w) { -wmb (); - w->end++; +__atomic_add_fetch (&w->end,1 , __ATOMIC_SEQ_CST); }Quoted 8 lines
static
__inline__void snd_atomic_read_init(snd_atomic_read_t *r,snd_atomic_write_t*w)Signature · 5 lines
2.1.4 _______________________________________________ Alsa-devel mailing list [email protected] http://mailman.alsa-project.org/mailman/listinfo/alsa-devel_______________________________________________
Alsa-devel mailing list
[email protected]
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
Quoted 12 lines
Quoted 6 lines
Quoted 5 lines
Signature · 3 lines
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 30Signature · 3 lines
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
_______________________________________________
Alsa-devel mailing list
[email protected]
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
Quoted 10 lines · Takashi Iwai
Quoted 8 lines
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.
Quoted 7 lines
Quoted 4 lines
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.
Quoted 14 lines
Quoted 4 lines
In total, the various snd_atomic_* macros are used ~20 times in
pcm_rate.cand ~30 times inpcm_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.
[...]
Quoted 2 lines
One would expect otherwise that this issue would have come up much sooner.
Quoted 7 lines
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
Signature · 7 lines
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 _______________________________________________ Alsa-devel mailing list [email protected] http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
Quoted 5 lines · Takashi Iwai
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?
/Ricard
Signature · 7 lines
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 _______________________________________________ Alsa-devel mailing list [email protected] http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
Quoted 4 lines · Ricard Wanderlof
On Thu, 12 May 2016, Takashi Iwai wrote:
Quoted 8 lines
Quoted 5 lines
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...
IMO, we just need to make a clear statement about the thread safety of alsa-lib. For a single threaded op, it can't give any regression, of course.
Takashi
Quoted 11 lines · Takashi Iwai
Quoted 8 lines
Quoted 5 lines
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.
Quoted 3 lines
IMO, we just need to make a clear statement about the thread safety of alsa-lib. For a single threaded op, it can't give any regression, of course.
Just removing the memory barriers should have absolutely no effect in the single threaded case I would have thought, especially since there are no I/O or other hardware related things (such as DMA descriptors) being poked at.
/Ricard
Signature · 3 lines
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
Quoted 29 lines · Ricard Wanderlof
Quoted 11 lines · Takashi Iwai
Quoted 8 lines
Quoted 5 lines
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.
Quoted 6 lines
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()andrmb()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
Quoted 3 lines · Takashi Iwai
There is a manual lock loop with
snd_atomic_read_wait(), as found inpcm_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.
Quoted 8 lines
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?
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.
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. :-)
/Ricard
Signature · 3 lines
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
Quoted 33 lines · Ricard Wanderlof
Quoted 3 lines · Takashi Iwai
There is a manual lock loop with
snd_atomic_read_wait(), as found inpcm_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.
Quoted 8 lines
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.hlooking something like:static
__inline__voidsnd_atomic_write_begin(snd_atomic_write_t *w){
+ #ifdefUSE_ATOMIC(quoted diff) +3
-2+ __atomic_add_fetch(&w->begin, 1,__ATOMIC_SEQ_CST); + #else - w->begin++; + #endif - wmb(); }
Quoted 2 lines
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.
Quoted 3 lines
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 :)
Quoted 3 lines
Also, it would prohibit building the building of alsa-lib for such applications with gcc < 4.7 (when
__atomic_add_fetchwas introduced), but perhaps that's more of an incentive than anything else. :-)
Yep.
Takashi
Quoted 2 lines · Takashi Iwai
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)?
/Ricard
Signature · 3 lines
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
Quoted 27 lines · Ricard Wanderlof
Quoted 2 lines · Takashi Iwai
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.ccode 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 thew->beginandw->endvariables 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.ptrand*pcm->hw.ptras 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->beginandw->endaccesses 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.
Quoted 3 lines
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