Re: [alsa-devel] Races in alsa-lib with threads
At Tue, 13 Nov 2012 13:39:24 +0000, Krakora Robert-B42341 wrote:
[1 <text/plain; us-ascii (quoted-printable)>]
From: Takashi Iwai [tiwai@suse.de] Sent: Tuesday, November 13, 2012 2:30 AM To: Trent Piepho Cc: Krakora Robert-B42341; alsa-devel@alsa-project.org Subject: Re: [alsa-devel] Races in alsa-lib with threads
At Tue, 13 Nov 2012 02:14:08 -0500, Trent Piepho wrote:
On Tue, Nov 13, 2012 at 1:32 AM, Takashi Iwai tiwai@suse.de wrote:
At Mon, 12 Nov 2012 19:40:42 +0000 (UTC), Rob Krakora wrote:
Would you be able to point me the the ALSA documentation that indicates the stipulations on handle usage using multiple threads? I cannot find it.
Think other way round: The fact that it isn't documented means it's not safe to use in that way :)
The introduction on the alsa-project home page says, "SMP and thread-safe design. " Some people might misunderstand what thread-safe means. Maybe some clarification could be added. "Different streams are SMP and thread-safe (calls for the same stream must be serialized)."
Yeah this should be corrected. SMP things are just about the kernel side at that time.
It's a Wiki, feel free to correct / improve the text.
Takashi
Hi Takashi,
The way that the GStreamer ALSA Sink Plugin is using ALSA Lib assumes that it is thread safe. The fix crafted by one of Trent's colleagues (attached) seems to be the way to go. However, I don't know how it would impact other functionality within ALSA Lib.
No, sorry, we don't want to introduce pthread_lock in alsa-lib PCM stuff.
We already have a few places, yes, but they are exceptional (mostly for funky rarely used plugins that we can drop or mixer codes that are no hot path).
Takashi
From: Takashi Iwai [tiwai@suse.de] Sent: Tuesday, November 13, 2012 8:50 AM To: Krakora Robert-B42341 Cc: Trent Piepho; alsa-devel@alsa-project.org Subject: Re: [alsa-devel] Races in alsa-lib with threads
At Tue, 13 Nov 2012 13:39:24 +0000, Krakora Robert-B42341 wrote:
[1 <text/plain; us-ascii (quoted-printable)>]
From: Takashi Iwai [tiwai@suse.de] Sent: Tuesday, November 13, 2012 2:30 AM To: Trent Piepho Cc: Krakora Robert-B42341; alsa-devel@alsa-project.org Subject: Re: [alsa-devel] Races in alsa-lib with threads
At Tue, 13 Nov 2012 02:14:08 -0500, Trent Piepho wrote:
On Tue, Nov 13, 2012 at 1:32 AM, Takashi Iwai tiwai@suse.de wrote:
At Mon, 12 Nov 2012 19:40:42 +0000 (UTC), Rob Krakora wrote:
Would you be able to point me the the ALSA documentation that indicates the stipulations on handle usage using multiple threads? I cannot find it.
Think other way round: The fact that it isn't documented means it's not safe to use in that way :)
The introduction on the alsa-project home page says, "SMP and thread-safe design. " Some people might misunderstand what thread-safe means. Maybe some clarification could be added. "Different streams are SMP and thread-safe (calls for the same stream must be serialized)."
Yeah this should be corrected. SMP things are just about the kernel side at that time.
It's a Wiki, feel free to correct / improve the text.
Takashi
Hi Takashi,
The way that the GStreamer ALSA Sink Plugin is using ALSA Lib assumes that it is thread safe. The fix crafted >by one of Trent's colleagues (attached) seems to be the way to go. However, I don't know how it would impact >other functionality within ALSA Lib.
No, sorry, we don't want to introduce pthread_lock in alsa-lib PCM stuff.
We already have a few places, yes, but they are exceptional (mostly for funky rarely used plugins that we can drop or mixer codes that are no hot path).
Takashi
Hi Takashi,
OK, but it sure seems as though the better fix would be in ALSA Lib (but then ALSA Lib assumes a great deal of risk unless it is re-architected for thread safety). However, you have an already established paradigm so I will see what I can do in the GStreamer ALSA Sink plugin.
Best Regards,
Rob Krakora
At Tue, 13 Nov 2012 14:04:22 +0000, Krakora Robert-B42341 wrote:
From: Takashi Iwai [tiwai@suse.de] Sent: Tuesday, November 13, 2012 8:50 AM To: Krakora Robert-B42341 Cc: Trent Piepho; alsa-devel@alsa-project.org Subject: Re: [alsa-devel] Races in alsa-lib with threads
At Tue, 13 Nov 2012 13:39:24 +0000, Krakora Robert-B42341 wrote:
[1 <text/plain; us-ascii (quoted-printable)>]
From: Takashi Iwai [tiwai@suse.de] Sent: Tuesday, November 13, 2012 2:30 AM To: Trent Piepho Cc: Krakora Robert-B42341; alsa-devel@alsa-project.org Subject: Re: [alsa-devel] Races in alsa-lib with threads
At Tue, 13 Nov 2012 02:14:08 -0500, Trent Piepho wrote:
On Tue, Nov 13, 2012 at 1:32 AM, Takashi Iwai tiwai@suse.de wrote:
At Mon, 12 Nov 2012 19:40:42 +0000 (UTC), Rob Krakora wrote: > Would you be able to point me the the ALSA documentation that indicates the > stipulations on handle usage using multiple threads? I cannot find it.
Think other way round: The fact that it isn't documented means it's not safe to use in that way :)
The introduction on the alsa-project home page says, "SMP and thread-safe design. " Some people might misunderstand what thread-safe means. Maybe some clarification could be added. "Different streams are SMP and thread-safe (calls for the same stream must be serialized)."
Yeah this should be corrected. SMP things are just about the kernel side at that time.
It's a Wiki, feel free to correct / improve the text.
Takashi
Hi Takashi,
The way that the GStreamer ALSA Sink Plugin is using ALSA Lib assumes that it is thread safe. The fix crafted >by one of Trent's colleagues (attached) seems to be the way to go. However, I don't know how it would impact >other functionality within ALSA Lib.
No, sorry, we don't want to introduce pthread_lock in alsa-lib PCM stuff.
We already have a few places, yes, but they are exceptional (mostly for funky rarely used plugins that we can drop or mixer codes that are no hot path).
Takashi
Hi Takashi,
OK, but it sure seems as though the better fix would be in ALSA Lib (but then ALSA Lib assumes a great deal of risk unless it is re-architected for thread safety).
Yeah, introducing a thread-safe requires some fundamental redesign. Otherwise we need pthread lock which is always risky to break something.
However, you have an already established paradigm so I will see what I can do in the GStreamer ALSA Sink plugin.
Thanks!
Takashi
Date 13.11.2012 14:50, Takashi Iwai wrote:
At Tue, 13 Nov 2012 13:39:24 +0000, Krakora Robert-B42341 wrote:
[1 <text/plain; us-ascii (quoted-printable)>]
From: Takashi Iwai [tiwai@suse.de] Sent: Tuesday, November 13, 2012 2:30 AM To: Trent Piepho Cc: Krakora Robert-B42341; alsa-devel@alsa-project.org Subject: Re: [alsa-devel] Races in alsa-lib with threads
At Tue, 13 Nov 2012 02:14:08 -0500, Trent Piepho wrote:
On Tue, Nov 13, 2012 at 1:32 AM, Takashi Iwai tiwai@suse.de wrote:
At Mon, 12 Nov 2012 19:40:42 +0000 (UTC), Rob Krakora wrote:
Would you be able to point me the the ALSA documentation that indicates the stipulations on handle usage using multiple threads? I cannot find it.
Think other way round: The fact that it isn't documented means it's not safe to use in that way :)
The introduction on the alsa-project home page says, "SMP and thread-safe design. " Some people might misunderstand what thread-safe means. Maybe some clarification could be added. "Different streams are SMP and thread-safe (calls for the same stream must be serialized)."
Yeah this should be corrected. SMP things are just about the kernel side at that time.
It's a Wiki, feel free to correct / improve the text.
Takashi
Hi Takashi,
The way that the GStreamer ALSA Sink Plugin is using ALSA Lib assumes that it is thread safe. The fix crafted by one of Trent's colleagues (attached) seems to be the way to go. However, I don't know how it would impact other functionality within ALSA Lib.
No, sorry, we don't want to introduce pthread_lock in alsa-lib PCM stuff.
We already have a few places, yes, but they are exceptional (mostly for funky rarely used plugins that we can drop or mixer codes that are no hot path).
I tried to put notes on the ALSA Wiki, please, extend / fix / review.
http://www.alsa-project.org/main/index.php/SMP_Design
Jaroslav
From: Jaroslav Kysela [perex@perex.cz] Sent: Tuesday, November 13, 2012 9:08 AM To: Takashi Iwai Cc: Krakora Robert-B42341; alsa-devel@alsa-project.org; Trent Piepho Subject: Re: [alsa-devel] Races in alsa-lib with threads
Date 13.11.2012 14:50, Takashi Iwai wrote:
At Tue, 13 Nov 2012 13:39:24 +0000, Krakora Robert-B42341 wrote:
[1 <text/plain; us-ascii (quoted-printable)>]
From: Takashi Iwai [tiwai@suse.de] Sent: Tuesday, November 13, 2012 2:30 AM To: Trent Piepho Cc: Krakora Robert-B42341; alsa-devel@alsa-project.org Subject: Re: [alsa-devel] Races in alsa-lib with threads
At Tue, 13 Nov 2012 02:14:08 -0500, Trent Piepho wrote:
On Tue, Nov 13, 2012 at 1:32 AM, Takashi Iwai tiwai@suse.de wrote:
At Mon, 12 Nov 2012 19:40:42 +0000 (UTC), Rob Krakora wrote:
Would you be able to point me the the ALSA documentation that indicates the stipulations on handle usage using multiple threads? I cannot find it.
Think other way round: The fact that it isn't documented means it's not safe to use in that way :)
The introduction on the alsa-project home page says, "SMP and thread-safe design. " Some people might misunderstand what thread-safe means. Maybe some clarification could be added. "Different streams are SMP and thread-safe (calls for the same stream must be serialized)."
Yeah this should be corrected. SMP things are just about the kernel side at that time.
It's a Wiki, feel free to correct / improve the text.
Takashi
Hi Takashi,
The way that the GStreamer ALSA Sink Plugin is using ALSA Lib assumes that it is thread safe. The fix crafted by one of Trent's colleagues (attached) seems to be the way to go. However, I don't know how it would impact other functionality within ALSA Lib.
No, sorry, we don't want to introduce pthread_lock in alsa-lib PCM stuff.
We already have a few places, yes, but they are exceptional (mostly for funky rarely used plugins that we can drop or mixer codes that are no hot path).
I tried to put notes on the ALSA Wiki, please, extend / fix / review.
http://www.alsa-project.org/main/index.php/SMP_Design
Jaroslav
-- Jaroslav Kysela perex@perex.cz Linux Kernel Sound Maintainer ALSA Project; Red Hat, Inc.
Hi Jaroslav,
Looks good, except I think the text in the link on the main page should be changed from "see these notes" to "PLEASE READ FIRST" in all caps for emphasis.
Best Regards,
Rob Krakora
At Tue, 13 Nov 2012 15:08:29 +0100, Jaroslav Kysela wrote:
Date 13.11.2012 14:50, Takashi Iwai wrote:
At Tue, 13 Nov 2012 13:39:24 +0000, Krakora Robert-B42341 wrote:
[1 <text/plain; us-ascii (quoted-printable)>]
From: Takashi Iwai [tiwai@suse.de] Sent: Tuesday, November 13, 2012 2:30 AM To: Trent Piepho Cc: Krakora Robert-B42341; alsa-devel@alsa-project.org Subject: Re: [alsa-devel] Races in alsa-lib with threads
At Tue, 13 Nov 2012 02:14:08 -0500, Trent Piepho wrote:
On Tue, Nov 13, 2012 at 1:32 AM, Takashi Iwai tiwai@suse.de wrote:
At Mon, 12 Nov 2012 19:40:42 +0000 (UTC), Rob Krakora wrote: > Would you be able to point me the the ALSA documentation > that indicates the stipulations on handle usage using > multiple threads? I cannot find it.
Think other way round: The fact that it isn't documented means it's not safe to use in that way :)
The introduction on the alsa-project home page says, "SMP and thread-safe design. " Some people might misunderstand what thread-safe means. Maybe some clarification could be added. "Different streams are SMP and thread-safe (calls for the same stream must be serialized)."
Yeah this should be corrected. SMP things are just about the kernel side at that time.
It's a Wiki, feel free to correct / improve the text.
Takashi
Hi Takashi,
The way that the GStreamer ALSA Sink Plugin is using ALSA Lib assumes that it is thread safe. The fix crafted by one of Trent's colleagues (attached) seems to be the way to go. However, I don't know how it would impact other functionality within ALSA Lib.
No, sorry, we don't want to introduce pthread_lock in alsa-lib PCM stuff.
We already have a few places, yes, but they are exceptional (mostly for funky rarely used plugins that we can drop or mixer codes that are no hot path).
I tried to put notes on the ALSA Wiki, please, extend / fix / review.
This looks much clearer now. Thanks!
Takashi
From: Takashi Iwai [tiwai@suse.de] Sent: Tuesday, November 13, 2012 8:50 AM To: Krakora Robert-B42341 Cc: Trent Piepho; alsa-devel@alsa-project.org Subject: Re: [alsa-devel] Races in alsa-lib with threads
At Tue, 13 Nov 2012 13:39:24 +0000, Krakora Robert-B42341 wrote:
[1 <text/plain; us-ascii (quoted-printable)>]
From: Takashi Iwai [tiwai@suse.de] Sent: Tuesday, November 13, 2012 2:30 AM To: Trent Piepho Cc: Krakora Robert-B42341; alsa-devel@alsa-project.org Subject: Re: [alsa-devel] Races in alsa-lib with threads
At Tue, 13 Nov 2012 02:14:08 -0500, Trent Piepho wrote:
On Tue, Nov 13, 2012 at 1:32 AM, Takashi Iwai tiwai@suse.de wrote:
At Mon, 12 Nov 2012 19:40:42 +0000 (UTC), Rob Krakora wrote:
Would you be able to point me the the ALSA documentation that indicates the stipulations on handle usage using multiple threads? I cannot find it.
Think other way round: The fact that it isn't documented means it's not safe to use in that way :)
The introduction on the alsa-project home page says, "SMP and thread-safe design. " Some people might misunderstand what thread-safe means. Maybe some clarification could be added. "Different streams are SMP and thread-safe (calls for the same stream must be serialized)."
Yeah this should be corrected. SMP things are just about the kernel side at that time.
It's a Wiki, feel free to correct / improve the text.
Takashi
Hi Takashi,
The way that the GStreamer ALSA Sink Plugin is using ALSA Lib assumes that it is thread safe. The fix crafted by one of Trent's colleagues (attached) seems to be the way to go. However, I don't know how it would impact other functionality within ALSA Lib.
No, sorry, we don't want to introduce pthread_lock in alsa-lib PCM stuff.
We already have a few places, yes, but they are exceptional (mostly for funky rarely used plugins that we can drop or mixer codes that are no hot path).
Takashi
Hi Takashi,
A colleague found this from two years ago: http://mailman.alsa-project.org/pipermail/alsa-devel/2010-June/028347.html.
IMHO, it seems trivial and harmless to serialize appl.ptr updates with mutexes unless there is a potential for deadlock or other possible race conditions in the affected ALSA Lib functions. I know it breaks the paradigm, but you can see how snd_pcm_delay() function can lead a developer to believe that it can be called on a different thread time from snd_pcm_writei().
Best Regards,
Rob Krakora
On Tue, Nov 13, 2012 at 8:50 AM, Takashi Iwai tiwai@suse.de wrote:
At Tue, 13 Nov 2012 13:39:24 +0000, Krakora Robert-B42341 wrote:
The way that the GStreamer ALSA Sink Plugin is using ALSA Lib assumes that it is thread safe. The fix crafted by one of Trent's colleagues (attached) seems to be the way to go. However, I don't know how it would impact other functionality within ALSA Lib.
No, sorry, we don't want to introduce pthread_lock in alsa-lib PCM stuff.
The problem with introduction serialization outside alsa-lib is that you must now serialize entire ALSA calls. The locks must be held for far too long.
Consider snd_pcm_writei(), most of the time is usually spent blocked waiting for a period to elapse. It is perfectly ok to call snd_pcm_delay() during this time. But if one isn't allowed to make any other pcm calls during snd_pcm_writei() then this can't be done.
It's a pretty big problem. Most code using blocking mode is going to write another period as soon as the writei call returns from the last write. It will spend almost all its time inside snd_pcm_writei() and thus will always be holding the app's pcm stream lock. As soon as it releases the lock it grabs it again for the next write. Another thread trying to call snd_pcm_delay() will virtually NEVER get a chance. Not only is it unnecessary stopped from getting the delay while another thread is blocked inside writei(), but it won't get a chance to call it between writei() calls either.
But there doesn't need to be a conflict, since the actual critical section that needs locking is very small, far smaller than the time spent sleeping inside writei() or wait(). How can just the critical section be protected without placing the locking inside alsa-lib?
Trent Piepho wrote:
Consider snd_pcm_writei(), most of the time is usually spent blocked waiting for a period to elapse. It is perfectly ok to call snd_pcm_delay() during this time. But if one isn't allowed to make any other pcm calls during snd_pcm_writei() then this can't be done.
Then use non-blocking mode. Blocking mode is suitable only for applications that do not want to access the device while waiting.
Regards, Clemens
On Tue, Nov 13, 2012 at 3:23 PM, Clemens Ladisch clemens@ladisch.de wrote:
Trent Piepho wrote:
Consider snd_pcm_writei(), most of the time is usually spent blocked waiting for a period to elapse. It is perfectly ok to call snd_pcm_delay() during this time. But if one isn't allowed to make any other pcm calls during snd_pcm_writei() then this can't be done.
Then use non-blocking mode. Blocking mode is suitable only for applications that do not want to access the device while waiting.
So you get the same problem with snd_pcm_wait(). Or snd_pcm_drain(). Any alsa-lib function than can block will exclude other threads from alsa-lib for much longer than necessary.
At Tue, 13 Nov 2012 14:41:40 -0500, Trent Piepho wrote:
On Tue, Nov 13, 2012 at 8:50 AM, Takashi Iwai tiwai@suse.de wrote:
At Tue, 13 Nov 2012 13:39:24 +0000, Krakora Robert-B42341 wrote:
The way that the GStreamer ALSA Sink Plugin is using ALSA Lib assumes that it is thread safe. The fix crafted by one of Trent's colleagues (attached) seems to be the way to go. However, I don't know how it would impact other functionality within ALSA Lib.
No, sorry, we don't want to introduce pthread_lock in alsa-lib PCM stuff.
The problem with introduction serialization outside alsa-lib is that you must now serialize entire ALSA calls. The locks must be held for far too long.
Consider snd_pcm_writei(), most of the time is usually spent blocked waiting for a period to elapse. It is perfectly ok to call snd_pcm_delay() during this time. But if one isn't allowed to make any other pcm calls during snd_pcm_writei() then this can't be done.
It's a pretty big problem. Most code using blocking mode is going to write another period as soon as the writei call returns from the last write. It will spend almost all its time inside snd_pcm_writei() and thus will always be holding the app's pcm stream lock. As soon as it releases the lock it grabs it again for the next write. Another thread trying to call snd_pcm_delay() will virtually NEVER get a chance. Not only is it unnecessary stopped from getting the delay while another thread is blocked inside writei(), but it won't get a chance to call it between writei() calls either.
But there doesn't need to be a conflict, since the actual critical section that needs locking is very small, far smaller than the time spent sleeping inside writei() or wait(). How can just the critical section be protected without placing the locking inside alsa-lib?
Well, in general it's no good idea to use a blocking write for multi-thread programs. Most of codes I know use a direct poll() call for a better control. The blocking mode is basically for dumb program that doesn't need a control.
Though, I see your point, too. Of course it'd be better if there is an atomic snd_pcm_delay() call. I think it's possible to give a consistent value without locking. Here I meant a value that doesn't express the exact position of the very current time, but at least a position that is being processed. It'll likely end up calling snd_pcm_mmap_avail(), and this won't involve with the sync / update procedure.
Well, just a thought, for now.
Takashi
At Tue, 13 Nov 2012 14:41:40 -0500, Trent Piepho wrote:
On Tue, Nov 13, 2012 at 8:50 AM, Takashi Iwai tiwai@suse.de wrote:
At Tue, 13 Nov 2012 13:39:24 +0000, Krakora Robert-B42341 wrote:
The way that the GStreamer ALSA Sink Plugin is using ALSA Lib assumes that it is thread safe. The fix crafted by one of Trent's colleagues (attached) seems to be the way to go. However, I don't know how it would impact other functionality within ALSA Lib.
No, sorry, we don't want to introduce pthread_lock in alsa-lib PCM stuff.
The problem with introduction serialization outside alsa-lib is that you must now serialize entire ALSA calls. The locks must be held for far too long.
Consider snd_pcm_writei(), most of the time is usually spent blocked waiting for a period to elapse. It is perfectly ok to call snd_pcm_delay() during this time. But if one isn't allowed to make any other pcm calls during snd_pcm_writei() then this can't be done.
It's a pretty big problem. Most code using blocking mode is going to write another period as soon as the writei call returns from the last write. It will spend almost all its time inside snd_pcm_writei() and thus will always be holding the app's pcm stream lock. As soon as it releases the lock it grabs it again for the next write. Another thread trying to call snd_pcm_delay() will virtually NEVER get a chance. Not only is it unnecessary stopped from getting the delay while another thread is blocked inside writei(), but it won't get a chance to call it between writei() calls either.
But there doesn't need to be a conflict, since the actual critical section that needs locking is very small, far smaller than the time spent sleeping inside writei() or wait(). How can just the critical section be protected without placing the locking inside alsa-lib?
Well, in general it's no good idea to use a blocking write for multi-thread programs. Most of codes I know use a direct poll() call for a better control. The blocking mode is basically for dumb program that doesn't need a control.
Though, I see your point, too. Of course it'd be better if there is an atomic snd_pcm_delay() call. I think it's possible to give a consistent value without locking. Here I meant a value that doesn't express the exact position of the very current time, but at least a position that is being processed. It'll likely end up calling snd_pcm_mmap_avail(), and this won't involve with the sync / update procedure.
Well, just a thought, for now.
Takashi
So, the "dumb program" is the GStreamer ALSA Sink plugin. What does an ALSA "poll" example look like?
snd_pcm_poll instead of snd_pcm_delay? ________________________________________ From: Krakora Robert-B42341 Sent: Tuesday, November 13, 2012 3:55 PM To: Takashi Iwai; Trent Piepho Cc: alsa-devel@alsa-project.org Subject: RE: [alsa-devel] Races in alsa-lib with threads
At Tue, 13 Nov 2012 14:41:40 -0500, Trent Piepho wrote:
On Tue, Nov 13, 2012 at 8:50 AM, Takashi Iwai tiwai@suse.de wrote:
At Tue, 13 Nov 2012 13:39:24 +0000, Krakora Robert-B42341 wrote:
The way that the GStreamer ALSA Sink Plugin is using ALSA Lib assumes that it is thread safe. The fix crafted by one of Trent's colleagues (attached) seems to be the way to go. However, I don't know how it would impact other functionality within ALSA Lib.
No, sorry, we don't want to introduce pthread_lock in alsa-lib PCM stuff.
The problem with introduction serialization outside alsa-lib is that you must now serialize entire ALSA calls. The locks must be held for far too long.
Consider snd_pcm_writei(), most of the time is usually spent blocked waiting for a period to elapse. It is perfectly ok to call snd_pcm_delay() during this time. But if one isn't allowed to make any other pcm calls during snd_pcm_writei() then this can't be done.
It's a pretty big problem. Most code using blocking mode is going to write another period as soon as the writei call returns from the last write. It will spend almost all its time inside snd_pcm_writei() and thus will always be holding the app's pcm stream lock. As soon as it releases the lock it grabs it again for the next write. Another thread trying to call snd_pcm_delay() will virtually NEVER get a chance. Not only is it unnecessary stopped from getting the delay while another thread is blocked inside writei(), but it won't get a chance to call it between writei() calls either.
But there doesn't need to be a conflict, since the actual critical section that needs locking is very small, far smaller than the time spent sleeping inside writei() or wait(). How can just the critical section be protected without placing the locking inside alsa-lib?
Well, in general it's no good idea to use a blocking write for multi-thread programs. Most of codes I know use a direct poll() call for a better control. The blocking mode is basically for dumb program that doesn't need a control.
Though, I see your point, too. Of course it'd be better if there is an atomic snd_pcm_delay() call. I think it's possible to give a consistent value without locking. Here I meant a value that doesn't express the exact position of the very current time, but at least a position that is being processed. It'll likely end up calling snd_pcm_mmap_avail(), and this won't involve with the sync / update procedure.
Well, just a thought, for now.
Takashi
So, the "dumb program" is the GStreamer ALSA Sink plugin. What does an ALSA "poll" example look like?
From: Trent Piepho [tpiepho@gmail.com] Sent: Tuesday, November 13, 2012 2:41 PM To: Takashi Iwai Cc: Krakora Robert-B42341; alsa-devel@alsa-project.org Subject: Re: [alsa-devel] Races in alsa-lib with threads
On Tue, Nov 13, 2012 at 8:50 AM, Takashi Iwai tiwai@suse.de wrote:
At Tue, 13 Nov 2012 13:39:24 +0000, Krakora Robert-B42341 wrote:
The way that the GStreamer ALSA Sink Plugin is using ALSA Lib assumes that it is thread safe. The fix crafted by one of Trent's colleagues (attached) seems to be the way to go. However, I don't know how it would impact other functionality within ALSA Lib.
No, sorry, we don't want to introduce pthread_lock in alsa-lib PCM stuff.
The problem with introduction serialization outside alsa-lib is that you must now serialize entire ALSA calls. The locks must be held for far too long.
Consider snd_pcm_writei(), most of the time is usually spent blocked waiting for a period to elapse. It is perfectly ok to call snd_pcm_delay() during this time. But if one isn't allowed to make any other pcm calls during snd_pcm_writei() then this can't be done.
It's a pretty big problem. Most code using blocking mode is going to write another period as soon as the writei call returns from the last write. It will spend almost all its time inside snd_pcm_writei() and thus will always be holding the app's pcm stream lock. As soon as it releases the lock it grabs it again for the next write. Another thread trying to call snd_pcm_delay() will virtually NEVER get a chance. Not only is it unnecessary stopped from getting the delay while another thread is blocked inside writei(), but it won't get a chance to call it between writei() calls either.
But there doesn't need to be a conflict, since the actual critical section that needs locking is very small, far smaller than the time spent sleeping inside writei() or wait(). How can just the critical section be protected without placing the locking inside alsa-lib?
I don't think it can.
On 11/13/2012 08:41 PM, Trent Piepho wrote:
On Tue, Nov 13, 2012 at 8:50 AM, Takashi Iwai tiwai@suse.de wrote:
At Tue, 13 Nov 2012 13:39:24 +0000, Krakora Robert-B42341 wrote:
The way that the GStreamer ALSA Sink Plugin is using ALSA Lib assumes that it is thread safe. The fix crafted by one of Trent's colleagues (attached) seems to be the way to go. However, I don't know how it would impact other functionality within ALSA Lib.
No, sorry, we don't want to introduce pthread_lock in alsa-lib PCM stuff.
The problem with introduction serialization outside alsa-lib is that you must now serialize entire ALSA calls. The locks must be held for far too long.
Consider snd_pcm_writei(), most of the time is usually spent blocked waiting for a period to elapse. It is perfectly ok to call snd_pcm_delay() during this time. But if one isn't allowed to make any other pcm calls during snd_pcm_writei() then this can't be done.
It's a pretty big problem. Most code using blocking mode is going to write another period as soon as the writei call returns from the last write. It will spend almost all its time inside snd_pcm_writei() and thus will always be holding the app's pcm stream lock. As soon as it releases the lock it grabs it again for the next write. Another thread trying to call snd_pcm_delay() will virtually NEVER get a chance. Not only is it unnecessary stopped from getting the delay while another thread is blocked inside writei(), but it won't get a chance to call it between writei() calls either.
But there doesn't need to be a conflict, since the actual critical section that needs locking is very small, far smaller than the time spent sleeping inside writei() or wait(). How can just the critical section be protected without placing the locking inside alsa-lib?
Hmm, but the great majority of programs are not interested in accessing alsa-lib from multiple threads in the way that's currently unsafe. That includes programs who are very dependent on low latency. I would not like to see all these programs to suffer from the overhead of adding locks here and there, even if those locks are small.
You claim that the Gstreamer ALSA sink plugin accesses alsa-lib from two threads simultaneously. Could you elaborate on how this can happen, maybe it is easy to fix?
From: alsa-devel-bounces@alsa-project.org [alsa-devel-bounces@alsa-project.org] on behalf of David Henningsson [david.henningsson@canonical.com] Sent: Wednesday, November 14, 2012 3:02 AM To: Trent Piepho Cc: Takashi Iwai; alsa-devel@alsa-project.org; Krakora Robert-B42341 Subject: Re: [alsa-devel] Races in alsa-lib with threads
On 11/13/2012 08:41 PM, Trent Piepho wrote:
On Tue, Nov 13, 2012 at 8:50 AM, Takashi Iwai tiwai@suse.de wrote:
At Tue, 13 Nov 2012 13:39:24 +0000, Krakora Robert-B42341 wrote:
The way that the GStreamer ALSA Sink Plugin is using ALSA Lib assumes that it is thread safe. The fix crafted by one of Trent's colleagues (attached) seems to be the way to go. However, I don't know how it would impact other functionality within ALSA Lib.
No, sorry, we don't want to introduce pthread_lock in alsa-lib PCM stuff.
The problem with introduction serialization outside alsa-lib is that you must now serialize entire ALSA calls. The locks must be held for far too long.
Consider snd_pcm_writei(), most of the time is usually spent blocked waiting for a period to elapse. It is perfectly ok to call snd_pcm_delay() during this time. But if one isn't allowed to make any other pcm calls during snd_pcm_writei() then this can't be done.
It's a pretty big problem. Most code using blocking mode is going to write another period as soon as the writei call returns from the last write. It will spend almost all its time inside snd_pcm_writei() and thus will always be holding the app's pcm stream lock. As soon as it releases the lock it grabs it again for the next write. Another thread trying to call snd_pcm_delay() will virtually NEVER get a chance. Not only is it unnecessary stopped from getting the delay while another thread is blocked inside writei(), but it won't get a chance to call it between writei() calls either.
But there doesn't need to be a conflict, since the actual critical section that needs locking is very small, far smaller than the time spent sleeping inside writei() or wait(). How can just the critical section be protected without placing the locking inside alsa-lib?
Hmm, but the great majority of programs are not interested in accessing alsa-lib from multiple threads in the way that's currently unsafe. That includes programs who are very dependent on low latency. I would not like to see all these programs to suffer from the overhead of adding locks here and there, even if those locks are small.
You claim that the Gstreamer ALSA sink plugin accesses alsa-lib from two threads simultaneously. Could you elaborate on how this can happen, maybe it is easy to fix?
-- David Henningsson, Canonical Ltd. https://launchpad.net/~diwic _______________________________________________ Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
Hi David,
It is not just merely a claim, it is a fact that the GStreamer ALSA Sink plugin calls snd_pcm_delay() on a different thread from snd_pcm_writei(). We believe that we may now have a workable solution and Jarloslav updated the ALSA wiki to reflect that ALSA is SMP/thread safe in the kernel space but no in user space. I believe that this is probably where the disconnect was; developers, that do not develop but merely use ALSA, misled by the original statement at the top of the ALSA wiki that ALSA was thread safe. I think we all understand the issue of added latency in regards to the addition of mutexes and are in agreement with the ALSA developers in this regard.
Best Regards,
Rob Krakora
On Wed, Nov 14, 2012 at 3:02 AM, David Henningsson david.henningsson@canonical.com wrote:
On 11/13/2012 08:41 PM, Trent Piepho wrote:
On Tue, Nov 13, 2012 at 8:50 AM, Takashi Iwai tiwai@suse.de wrote:
At Tue, 13 Nov 2012 13:39:24 +0000, Krakora Robert-B42341 wrote:
The way that the GStreamer ALSA Sink Plugin is using ALSA Lib assumes that it is thread safe. The fix crafted by one of Trent's colleagues (attached) seems to be the way to go. However, I don't know how it would impact other functionality within ALSA Lib.
No, sorry, we don't want to introduce pthread_lock in alsa-lib PCM stuff.
The problem with introduction serialization outside alsa-lib is that you must now serialize entire ALSA calls. The locks must be held for far too long.
Consider snd_pcm_writei(), most of the time is usually spent blocked waiting for a period to elapse. It is perfectly ok to call snd_pcm_delay() during this time. But if one isn't allowed to make any other pcm calls during snd_pcm_writei() then this can't be done.
It's a pretty big problem. Most code using blocking mode is going to write another period as soon as the writei call returns from the last write. It will spend almost all its time inside snd_pcm_writei() and thus will always be holding the app's pcm stream lock. As soon as it releases the lock it grabs it again for the next write. Another thread trying to call snd_pcm_delay() will virtually NEVER get a chance. Not only is it unnecessary stopped from getting the delay while another thread is blocked inside writei(), but it won't get a chance to call it between writei() calls either.
But there doesn't need to be a conflict, since the actual critical section that needs locking is very small, far smaller than the time spent sleeping inside writei() or wait(). How can just the critical section be protected without placing the locking inside alsa-lib?
Hmm, but the great majority of programs are not interested in accessing alsa-lib from multiple threads in the way that's currently unsafe. That includes programs who are very dependent on low latency. I would not like to see all these programs to suffer from the overhead of adding locks here and there, even if those locks are small.
Maybe they just don't know it's unsafe and don't hit a race often enough to notice?
You claim that the Gstreamer ALSA sink plugin accesses alsa-lib from two threads simultaneously. Could you elaborate on how this can happen, maybe it is easy to fix?
gstreamer has no lock around the call to snd_pcm_delay(). So it can race with snd_pcm_wait() or snd_pcm_writei(), which are called in another thread. There is a lock around the block of code calling wait()/writei(), but this lock isn't used for calling delay(). It seems that they didn't know there would be a problem.
And if you're not using the alsa rate plugin, then I don't think snd_pcm_delay() has a race with snd_pcm_writei(). delay for a hw device will call an ioctl to just get the delay. I think that's ok, assuming the kernel part has no races. It's only when using the rate plugin that snd_pcm_delay() will try to update the pointer and race with writei.
So unless you use the rate plugin, you'll never notice this race.
Hello,
Le mercredi 14 novembre 2012 21:49:36, Trent Piepho a écrit :
You claim that the Gstreamer ALSA sink plugin accesses alsa-lib from two threads simultaneously. Could you elaborate on how this can happen, maybe it is easy to fix?
gstreamer has no lock around the call to snd_pcm_delay(). So it can race with snd_pcm_wait() or snd_pcm_writei(), which are called in another thread. There is a lock around the block of code calling wait()/writei(), but this lock isn't used for calling delay().
Use non-blocking I/O and poll(). Then the snd_pcm_write() calls will not sleep and can be interlocked with snd_pcm_delay() calls.
It seems that they didn't know there would be a problem.
Eh? It is rather the exception for a library to be thread-safe when using the same object in multiple threads.
It looks more like "they" did not pay attention to what they were doing.
________________________________________ From: alsa-devel-bounces@alsa-project.org [alsa-devel-bounces@alsa-project.org] on behalf of Rémi Denis-Courmont [remi@remlab.net] Sent: Wednesday, November 14, 2012 3:03 PM To: alsa-devel@alsa-project.org Cc: Trent Piepho Subject: Re: [alsa-devel] Races in alsa-lib with threads
Hello,
Le mercredi 14 novembre 2012 21:49:36, Trent Piepho a écrit :
You claim that the Gstreamer ALSA sink plugin accesses alsa-lib from two threads simultaneously. Could you elaborate on how this can happen, maybe it is easy to fix?
gstreamer has no lock around the call to snd_pcm_delay(). So it can race with snd_pcm_wait() or snd_pcm_writei(), which are called in another thread. There is a lock around the block of code calling wait()/writei(), but this lock isn't used for calling delay().
Use non-blocking I/O and poll(). Then the snd_pcm_write() calls will not sleep and can be interlocked with snd_pcm_delay() calls.
It seems that they didn't know there would be a problem.
Eh? It is rather the exception for a library to be thread-safe when using the same object in multiple threads.
It looks more like "they" did not pay attention to what they were doing.
-- Rémi Denis-Courmont http://www.remlab.net/ _______________________________________________ Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
HI Rémi,
We are using non-blocking mode with snd_pcm_wait() and snd_pcm_writei(). We have a workable solution. Thanks for the advice.
Best Regards.
Rob
On Wed, Nov 14, 2012 at 3:03 PM, Rémi Denis-Courmont remi@remlab.net wrote:
Le mercredi 14 novembre 2012 21:49:36, Trent Piepho a écrit :
You claim that the Gstreamer ALSA sink plugin accesses alsa-lib from two threads simultaneously. Could you elaborate on how this can happen, maybe it is easy to fix?
gstreamer has no lock around the call to snd_pcm_delay(). So it can race with snd_pcm_wait() or snd_pcm_writei(), which are called in another thread. There is a lock around the block of code calling wait()/writei(), but this lock isn't used for calling delay().
Use non-blocking I/O and poll(). Then the snd_pcm_write() calls will not sleep and can be interlocked with snd_pcm_delay() calls.
gstreamer uses non-blocking I/O. But uses snd_pcm_wait rather than poll directly. I wonder if poll() is entirely correct when ALSA plugins are in the chain?
"the snd_pcm_write() calls ...can be interlocked with snd_pcm_delay() calls".
It's still a race to call writei() and delay() at the same time, even if writei() is non-blocking. But only if the rate plugin is being used. Of course the actual race is very small, and doesn't include the time spent doing resampling or mixing in plugins which are still part of the writei() call in non-blocking mode. It's basically like the BKL around every ALSA call.
It seems that they didn't know there would be a problem.
Eh? It is rather the exception for a library to be thread-safe when using the same object in multiple threads.
It's ok to call kernel syscalls on the same fd at the same time. The kernel used to have the BKL for this. Now it has fine grained locking around the actual critical sections. So if ALSA is called thread safe, it doesn't seem that unreasonable to think one can do this.
It looks more like "they" did not pay attention to what they were doing.
I think the point is that this bug was in gstreamer. gstreamer is commonly used by many people. The bug has been there for years. Many programmers have looked at the code. Yet no one noticed it or tracked it down until now. If it's so obvious why did it take so long?
You aren't aware of this problem in other multi-threaded code, but that doesn't mean it doesn't exist. You weren't aware of the problem in gstreamer and yet it exists.
Le mercredi 14 novembre 2012 22:54:15, Trent Piepho a écrit :
gstreamer uses non-blocking I/O. But uses snd_pcm_wait rather than poll directly. I wonder if poll() is entirely correct when ALSA plugins are in the chain?
"the snd_pcm_write() calls ...can be interlocked with snd_pcm_delay() calls".
It's still a race to call writei() and delay() at the same time, even if writei() is non-blocking. But only if the rate plugin is being used. Of course the actual race is very small, and doesn't include the time spent doing resampling or mixing in plugins which are still part of the writei() call in non-blocking mode. It's basically like the BKL around every ALSA call.
Of course, you will need a lock around many ALSA calls, at least all potentially concurrent call. But that's a great improvement over holding a lock while sleeping inside a blocking I/O operation.
It seems that they didn't know there would be a problem.
Eh? It is rather the exception for a library to be thread-safe when using the same object in multiple threads.
It's ok to call kernel syscalls on the same fd at the same time.
Yes and no. Typically the kernel protects itself against undefined behaviour induced by user space. But it does not protect user space against their own undefined behaviour. The file descriptors table is a very good example of that.
In fact, calling snd_pcm_delay() and snd_pcm_write() concurrently is just PLAIN STUPID. The delay measurement may or may not include the impeding write operation, or maybe only a part of it. Essentially, the delay value is not much more than random garbage.
Hiding a mutex inside alsa-lib will NOT fix this intrinsic problem: Instead of having data races in memory, you will have logical races and gstreamer would still be buggy in some ways.
The kernel used to have the BKL for this. Now it has fine grained locking around the actual critical sections. So if ALSA is called thread safe, it doesn't seem that unreasonable to think one can do this.
Thread-safe means you can enter the library functions from multiple threads. Thread-safe never meant that you can enter the library functions from multiple threads with the SAME DATA.
See also Lennart's and Kay's example library...
It looks more like "they" did not pay attention to what they were doing.
I think the point is that this bug was in gstreamer. gstreamer is commonly used by many people. The bug has been there for years. Many programmers have looked at the code. Yet no one noticed it or tracked it down until now. If it's so obvious why did it take so long?
You aren't aware of this problem in other multi-threaded code, but that doesn't mean it doesn't exist. You weren't aware of the problem in gstreamer and yet it exists.
Hey, I don't use gstreamer. VLC has interlocked its ALSA calls for many years and I did not blame alsa-lib for not doing it internally. In fact, I realized that this was not a problem that ALSA could nor should solve.
Also, all audio APIs are like that. Even PulseAudio requires the application to acquire locks explicitly.
From: alsa-devel-bounces@alsa-project.org [alsa-devel-bounces@alsa-project.org] on behalf of Rémi Denis-Courmont [remi@remlab.net] Sent: Wednesday, November 14, 2012 4:19 PM To: Trent Piepho Cc: alsa-devel@alsa-project.org Subject: Re: [alsa-devel] Races in alsa-lib with threads
Le mercredi 14 novembre 2012 22:54:15, Trent Piepho a écrit :
gstreamer uses non-blocking I/O. But uses snd_pcm_wait rather than poll directly. I wonder if poll() is entirely correct when ALSA plugins are in the chain?
"the snd_pcm_write() calls ...can be interlocked with snd_pcm_delay() calls".
It's still a race to call writei() and delay() at the same time, even if writei() is non-blocking. But only if the rate plugin is being used. Of course the actual race is very small, and doesn't include the time spent doing resampling or mixing in plugins which are still part of the writei() call in non-blocking mode. It's basically like the BKL around every ALSA call.
Of course, you will need a lock around many ALSA calls, at least all potentially concurrent call. But that's a great improvement over holding a lock while sleeping inside a blocking I/O operation.
It seems that they didn't know there would be a problem.
Eh? It is rather the exception for a library to be thread-safe when using the same object in multiple threads.
It's ok to call kernel syscalls on the same fd at the same time.
Yes and no. Typically the kernel protects itself against undefined behaviour induced by user space. But it does not protect user space against their own undefined behaviour. The file descriptors table is a very good example of that.
In fact, calling snd_pcm_delay() and snd_pcm_write() concurrently is just PLAIN STUPID. The delay measurement may or may not include the impeding write operation, or maybe only a part of it. Essentially, the delay value is not much more than random garbage.
Hiding a mutex inside alsa-lib will NOT fix this intrinsic problem: Instead of having data races in memory, you will have logical races and gstreamer would still be buggy in some ways.
The kernel used to have the BKL for this. Now it has fine grained locking around the actual critical sections. So if ALSA is called thread safe, it doesn't seem that unreasonable to think one can do this.
Thread-safe means you can enter the library functions from multiple threads. Thread-safe never meant that you can enter the library functions from multiple threads with the SAME DATA.
See also Lennart's and Kay's example library...
It looks more like "they" did not pay attention to what they were doing.
I think the point is that this bug was in gstreamer. gstreamer is commonly used by many people. The bug has been there for years. Many programmers have looked at the code. Yet no one noticed it or tracked it down until now. If it's so obvious why did it take so long?
You aren't aware of this problem in other multi-threaded code, but that doesn't mean it doesn't exist. You weren't aware of the problem in gstreamer and yet it exists.
Hey, I don't use gstreamer. VLC has interlocked its ALSA calls for many years and I did not blame alsa-lib for not doing it internally. In fact, I realized that this was not a problem that ALSA could nor should solve.
Also, all audio APIs are like that. Even PulseAudio requires the application to acquire locks explicitly.
-- Rémi Denis-Courmont http://www.remlab.net/ _______________________________________________ Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
Hi Remi,
We have a solution now that we are testing where the ALSA calls reside is a single thread within the GStreamer ALSA Sink plugin. Once we tested it for a bit, we will submit it to the GStreamer folks for approval. Now that the ALSA Wiki has been updated with Jarloslav's caveat about ALSA thread safety, newbies should no longer misuse that ALSA Lib API. Thanks so much for your insight and for Takashi's and Jarloslav's help as well. Sorry to take up so much of your time. ;-)
Best Regards,
Rob
At Wed, 14 Nov 2012 14:49:36 -0500, Trent Piepho wrote:
On Wed, Nov 14, 2012 at 3:02 AM, David Henningsson david.henningsson@canonical.com wrote:
On 11/13/2012 08:41 PM, Trent Piepho wrote:
On Tue, Nov 13, 2012 at 8:50 AM, Takashi Iwai tiwai@suse.de wrote:
At Tue, 13 Nov 2012 13:39:24 +0000, Krakora Robert-B42341 wrote:
The way that the GStreamer ALSA Sink Plugin is using ALSA Lib assumes that it is thread safe. The fix crafted by one of Trent's colleagues (attached) seems to be the way to go. However, I don't know how it would impact other functionality within ALSA Lib.
No, sorry, we don't want to introduce pthread_lock in alsa-lib PCM stuff.
The problem with introduction serialization outside alsa-lib is that you must now serialize entire ALSA calls. The locks must be held for far too long.
Consider snd_pcm_writei(), most of the time is usually spent blocked waiting for a period to elapse. It is perfectly ok to call snd_pcm_delay() during this time. But if one isn't allowed to make any other pcm calls during snd_pcm_writei() then this can't be done.
It's a pretty big problem. Most code using blocking mode is going to write another period as soon as the writei call returns from the last write. It will spend almost all its time inside snd_pcm_writei() and thus will always be holding the app's pcm stream lock. As soon as it releases the lock it grabs it again for the next write. Another thread trying to call snd_pcm_delay() will virtually NEVER get a chance. Not only is it unnecessary stopped from getting the delay while another thread is blocked inside writei(), but it won't get a chance to call it between writei() calls either.
But there doesn't need to be a conflict, since the actual critical section that needs locking is very small, far smaller than the time spent sleeping inside writei() or wait(). How can just the critical section be protected without placing the locking inside alsa-lib?
Hmm, but the great majority of programs are not interested in accessing alsa-lib from multiple threads in the way that's currently unsafe. That includes programs who are very dependent on low latency. I would not like to see all these programs to suffer from the overhead of adding locks here and there, even if those locks are small.
Maybe they just don't know it's unsafe and don't hit a race often enough to notice?
Maybe, but many multi-threaded codes are carefully holding locks already in the caller side.
You claim that the Gstreamer ALSA sink plugin accesses alsa-lib from two threads simultaneously. Could you elaborate on how this can happen, maybe it is easy to fix?
gstreamer has no lock around the call to snd_pcm_delay(). So it can race with snd_pcm_wait() or snd_pcm_writei(), which are called in another thread. There is a lock around the block of code calling wait()/writei(), but this lock isn't used for calling delay(). It seems that they didn't know there would be a problem.
Actually the atomic lock things found in alsa-lib code is no proper lock for avoiding this kind of race. It's equivalent with seq_lock() in kernel, and it's used to make snd_pcm_status() consistent. It doesn't protect against the data corruption due to concurrent accesses.
A part of concurrent access problems in the rate plugin can be fixed by a patch like below. In essence, it avoids updating the internal hw_ptr value but just calculates the current hw_ptr and returns. Most of other changes in pcm_local.h are only refactoring. But this is obviously just a bandaid on the spot, it doesn't solve the whole problems at all. So I don't buy this.
One modest solution would be to add a bit flag SND_PCM_THREAD_SAFE to snd_pcm_open(), and activate pthread mutex locks conditionally only with this bit set. This isn't designed for any performance-wise codes, and it's just for protecting concurrent accesses, so should be called like SND_PCM_THREAD_SAFE_FOR_DUMMIES...
Takashi
--- diff --git a/src/pcm/pcm_local.h b/src/pcm/pcm_local.h index 8cf7c3d..af4695b 100644 --- a/src/pcm/pcm_local.h +++ b/src/pcm/pcm_local.h @@ -420,10 +420,15 @@ static inline int snd_pcm_check_error(snd_pcm_t *pcm, int err) return err; }
-static inline snd_pcm_uframes_t snd_pcm_mmap_playback_avail(snd_pcm_t *pcm) +static inline snd_pcm_uframes_t +_snd_pcm_calc_avail(snd_pcm_t *pcm, snd_pcm_uframes_t hw_ptr, + snd_pcm_uframes_t appl_ptr) { snd_pcm_sframes_t avail; - avail = *pcm->hw.ptr + pcm->buffer_size - *pcm->appl.ptr; + + avail = hw_ptr - appl_ptr; + if (pcm->stream == SND_PCM_STREAM_PLAYBACK) + avail += pcm->buffer_size; if (avail < 0) avail += pcm->boundary; else if ((snd_pcm_uframes_t) avail >= pcm->boundary) @@ -431,44 +436,22 @@ static inline snd_pcm_uframes_t snd_pcm_mmap_playback_avail(snd_pcm_t *pcm) return avail; }
-static inline snd_pcm_uframes_t snd_pcm_mmap_capture_avail(snd_pcm_t *pcm) -{ - snd_pcm_sframes_t avail; - avail = *pcm->hw.ptr - *pcm->appl.ptr; - if (avail < 0) - avail += pcm->boundary; - return avail; -} - static inline snd_pcm_uframes_t snd_pcm_mmap_avail(snd_pcm_t *pcm) { - if (pcm->stream == SND_PCM_STREAM_PLAYBACK) - return snd_pcm_mmap_playback_avail(pcm); - else - return snd_pcm_mmap_capture_avail(pcm); + return _snd_pcm_calc_avail(pcm, *pcm->hw.ptr, *pcm->appl.ptr); }
-static inline snd_pcm_sframes_t snd_pcm_mmap_playback_hw_avail(snd_pcm_t *pcm) -{ - return pcm->buffer_size - snd_pcm_mmap_playback_avail(pcm); -} - -static inline snd_pcm_sframes_t snd_pcm_mmap_capture_hw_avail(snd_pcm_t *pcm) -{ - return pcm->buffer_size - snd_pcm_mmap_capture_avail(pcm); -} +#define snd_pcm_mmap_playback_avail snd_pcm_mmap_avail +#define snd_pcm_mmap_capture_avail snd_pcm_mmap_avail
static inline snd_pcm_sframes_t snd_pcm_mmap_hw_avail(snd_pcm_t *pcm) { - snd_pcm_sframes_t avail; - avail = *pcm->hw.ptr - *pcm->appl.ptr; - if (pcm->stream == SND_PCM_STREAM_PLAYBACK) - avail += pcm->buffer_size; - if (avail < 0) - avail += pcm->boundary; - return pcm->buffer_size - avail; + return pcm->buffer_size - snd_pcm_mmap_avail(pcm); }
+#define snd_pcm_mmap_playback_hw_avail snd_pcm_mmap_hw_avail +#define snd_pcm_mmap_capture_hw_avail snd_pcm_mmap_hw_avail + static inline const snd_pcm_channel_area_t *snd_pcm_mmap_areas(snd_pcm_t *pcm) { if (pcm->stopped_areas && diff --git a/src/pcm/pcm_rate.c b/src/pcm/pcm_rate.c index 54a3e67..340a4d3 100644 --- a/src/pcm/pcm_rate.c +++ b/src/pcm/pcm_rate.c @@ -613,21 +613,28 @@ static inline snd_pcm_sframes_t snd_pcm_rate_move_applptr(snd_pcm_t *pcm, snd_pc return diff; }
-static inline void snd_pcm_rate_sync_hwptr(snd_pcm_t *pcm) +static inline snd_pcm_uframes_t snd_pcm_rate_get_hwptr(snd_pcm_t *pcm) { snd_pcm_rate_t *rate = pcm->private_data; snd_pcm_uframes_t slave_hw_ptr = *rate->gen.slave->hw.ptr;
if (pcm->stream != SND_PCM_STREAM_PLAYBACK) - return; + return rate->hw_ptr; /* FIXME: boundary overlap of slave hw_ptr isn't evaluated here! * e.g. if slave rate is small... */ - rate->hw_ptr = - (slave_hw_ptr / rate->gen.slave->period_size) * pcm->period_size + + return (slave_hw_ptr / rate->gen.slave->period_size) * pcm->period_size + rate->ops.input_frames(rate->obj, slave_hw_ptr % rate->gen.slave->period_size); }
+static inline void snd_pcm_rate_sync_hwptr(snd_pcm_t *pcm) +{ + snd_pcm_rate_t *rate = pcm->private_data; + + if (pcm->stream == SND_PCM_STREAM_PLAYBACK) + rate->hw_ptr = snd_pcm_rate_get_hwptr(pcm); +} + static int snd_pcm_rate_hwsync(snd_pcm_t *pcm) { snd_pcm_rate_t *rate = pcm->private_data; @@ -642,11 +649,11 @@ static int snd_pcm_rate_hwsync(snd_pcm_t *pcm)
static int snd_pcm_rate_delay(snd_pcm_t *pcm, snd_pcm_sframes_t *delayp) { - snd_pcm_rate_hwsync(pcm); - if (pcm->stream == SND_PCM_STREAM_PLAYBACK) - *delayp = snd_pcm_mmap_playback_hw_avail(pcm); - else - *delayp = snd_pcm_mmap_capture_hw_avail(pcm); + snd_pcm_rate_t *rate = pcm->private_data; + + snd_pcm_hwsync(rate->gen.slave); + *delayp = _snd_pcm_calc_avail(pcm, snd_pcm_rate_get_hwptr(pcm), + *pcm->appl.ptr); return 0; }
From: Takashi Iwai [tiwai@suse.de] Sent: Thursday, November 15, 2012 2:39 AM To: Trent Piepho Cc: David Henningsson; alsa-devel@alsa-project.org; Krakora Robert-B42341 Subject: Re: [alsa-devel] Races in alsa-lib with threads
At Wed, 14 Nov 2012 14:49:36 -0500, Trent Piepho wrote:
On Wed, Nov 14, 2012 at 3:02 AM, David Henningsson david.henningsson@canonical.com wrote:
On 11/13/2012 08:41 PM, Trent Piepho wrote:
On Tue, Nov 13, 2012 at 8:50 AM, Takashi Iwai tiwai@suse.de wrote:
At Tue, 13 Nov 2012 13:39:24 +0000, Krakora Robert-B42341 wrote:
The way that the GStreamer ALSA Sink Plugin is using ALSA Lib assumes that it is thread safe. The fix crafted by one of Trent's colleagues (attached) seems to be the way to go. However, I don't know how it would impact other functionality within ALSA Lib.
No, sorry, we don't want to introduce pthread_lock in alsa-lib PCM stuff.
The problem with introduction serialization outside alsa-lib is that you must now serialize entire ALSA calls. The locks must be held for far too long.
Consider snd_pcm_writei(), most of the time is usually spent blocked waiting for a period to elapse. It is perfectly ok to call snd_pcm_delay() during this time. But if one isn't allowed to make any other pcm calls during snd_pcm_writei() then this can't be done.
It's a pretty big problem. Most code using blocking mode is going to write another period as soon as the writei call returns from the last write. It will spend almost all its time inside snd_pcm_writei() and thus will always be holding the app's pcm stream lock. As soon as it releases the lock it grabs it again for the next write. Another thread trying to call snd_pcm_delay() will virtually NEVER get a chance. Not only is it unnecessary stopped from getting the delay while another thread is blocked inside writei(), but it won't get a chance to call it between writei() calls either.
But there doesn't need to be a conflict, since the actual critical section that needs locking is very small, far smaller than the time spent sleeping inside writei() or wait(). How can just the critical section be protected without placing the locking inside alsa-lib?
Hmm, but the great majority of programs are not interested in accessing alsa-lib from multiple threads in the way that's currently unsafe. That includes programs who are very dependent on low latency. I would not like to see all these programs to suffer from the overhead of adding locks here and there, even if those locks are small.
Maybe they just don't know it's unsafe and don't hit a race often enough to notice?
Maybe, but many multi-threaded codes are carefully holding locks already in the caller side.
You claim that the Gstreamer ALSA sink plugin accesses alsa-lib from two threads simultaneously. Could you elaborate on how this can happen, maybe it is easy to fix?
gstreamer has no lock around the call to snd_pcm_delay(). So it can race with snd_pcm_wait() or snd_pcm_writei(), which are called in another thread. There is a lock around the block of code calling wait()/writei(), but this lock isn't used for calling delay(). It seems that they didn't know there would be a problem.
Actually the atomic lock things found in alsa-lib code is no proper lock for avoiding this kind of race. It's equivalent with seq_lock() in kernel, and it's used to make snd_pcm_status() consistent. It doesn't protect against the data corruption due to concurrent accesses.
A part of concurrent access problems in the rate plugin can be fixed by a patch like below. In essence, it avoids updating the internal hw_ptr value but just calculates the current hw_ptr and returns. Most of other changes in pcm_local.h are only refactoring. But this is obviously just a bandaid on the spot, it doesn't solve the whole problems at all. So I don't buy this.
One modest solution would be to add a bit flag SND_PCM_THREAD_SAFE to snd_pcm_open(), and activate pthread mutex locks conditionally only with this bit set. This isn't designed for any performance-wise codes, and it's just for protecting concurrent accesses, so should be called like SND_PCM_THREAD_SAFE_FOR_DUMMIES...
Takashi
--- diff --git a/src/pcm/pcm_local.h b/src/pcm/pcm_local.h index 8cf7c3d..af4695b 100644 --- a/src/pcm/pcm_local.h +++ b/src/pcm/pcm_local.h @@ -420,10 +420,15 @@ static inline int snd_pcm_check_error(snd_pcm_t *pcm, int err) return err; }
-static inline snd_pcm_uframes_t snd_pcm_mmap_playback_avail(snd_pcm_t *pcm) +static inline snd_pcm_uframes_t +_snd_pcm_calc_avail(snd_pcm_t *pcm, snd_pcm_uframes_t hw_ptr, + snd_pcm_uframes_t appl_ptr) { snd_pcm_sframes_t avail; - avail = *pcm->hw.ptr + pcm->buffer_size - *pcm->appl.ptr; + + avail = hw_ptr - appl_ptr; + if (pcm->stream == SND_PCM_STREAM_PLAYBACK) + avail += pcm->buffer_size; if (avail < 0) avail += pcm->boundary; else if ((snd_pcm_uframes_t) avail >= pcm->boundary) @@ -431,44 +436,22 @@ static inline snd_pcm_uframes_t snd_pcm_mmap_playback_avail(snd_pcm_t *pcm) return avail; }
-static inline snd_pcm_uframes_t snd_pcm_mmap_capture_avail(snd_pcm_t *pcm) -{ - snd_pcm_sframes_t avail; - avail = *pcm->hw.ptr - *pcm->appl.ptr; - if (avail < 0) - avail += pcm->boundary; - return avail; -} - static inline snd_pcm_uframes_t snd_pcm_mmap_avail(snd_pcm_t *pcm) { - if (pcm->stream == SND_PCM_STREAM_PLAYBACK) - return snd_pcm_mmap_playback_avail(pcm); - else - return snd_pcm_mmap_capture_avail(pcm); + return _snd_pcm_calc_avail(pcm, *pcm->hw.ptr, *pcm->appl.ptr); }
-static inline snd_pcm_sframes_t snd_pcm_mmap_playback_hw_avail(snd_pcm_t *pcm) -{ - return pcm->buffer_size - snd_pcm_mmap_playback_avail(pcm); -} - -static inline snd_pcm_sframes_t snd_pcm_mmap_capture_hw_avail(snd_pcm_t *pcm) -{ - return pcm->buffer_size - snd_pcm_mmap_capture_avail(pcm); -} +#define snd_pcm_mmap_playback_avail snd_pcm_mmap_avail +#define snd_pcm_mmap_capture_avail snd_pcm_mmap_avail
static inline snd_pcm_sframes_t snd_pcm_mmap_hw_avail(snd_pcm_t *pcm) { - snd_pcm_sframes_t avail; - avail = *pcm->hw.ptr - *pcm->appl.ptr; - if (pcm->stream == SND_PCM_STREAM_PLAYBACK) - avail += pcm->buffer_size; - if (avail < 0) - avail += pcm->boundary; - return pcm->buffer_size - avail; + return pcm->buffer_size - snd_pcm_mmap_avail(pcm); }
+#define snd_pcm_mmap_playback_hw_avail snd_pcm_mmap_hw_avail +#define snd_pcm_mmap_capture_hw_avail snd_pcm_mmap_hw_avail + static inline const snd_pcm_channel_area_t *snd_pcm_mmap_areas(snd_pcm_t *pcm) { if (pcm->stopped_areas && diff --git a/src/pcm/pcm_rate.c b/src/pcm/pcm_rate.c index 54a3e67..340a4d3 100644 --- a/src/pcm/pcm_rate.c +++ b/src/pcm/pcm_rate.c @@ -613,21 +613,28 @@ static inline snd_pcm_sframes_t snd_pcm_rate_move_applptr(snd_pcm_t *pcm, snd_pc return diff; }
-static inline void snd_pcm_rate_sync_hwptr(snd_pcm_t *pcm) +static inline snd_pcm_uframes_t snd_pcm_rate_get_hwptr(snd_pcm_t *pcm) { snd_pcm_rate_t *rate = pcm->private_data; snd_pcm_uframes_t slave_hw_ptr = *rate->gen.slave->hw.ptr;
if (pcm->stream != SND_PCM_STREAM_PLAYBACK) - return; + return rate->hw_ptr; /* FIXME: boundary overlap of slave hw_ptr isn't evaluated here! * e.g. if slave rate is small... */ - rate->hw_ptr = - (slave_hw_ptr / rate->gen.slave->period_size) * pcm->period_size + + return (slave_hw_ptr / rate->gen.slave->period_size) * pcm->period_size + rate->ops.input_frames(rate->obj, slave_hw_ptr % rate->gen.slave->period_size); }
+static inline void snd_pcm_rate_sync_hwptr(snd_pcm_t *pcm) +{ + snd_pcm_rate_t *rate = pcm->private_data; + + if (pcm->stream == SND_PCM_STREAM_PLAYBACK) + rate->hw_ptr = snd_pcm_rate_get_hwptr(pcm); +} + static int snd_pcm_rate_hwsync(snd_pcm_t *pcm) { snd_pcm_rate_t *rate = pcm->private_data; @@ -642,11 +649,11 @@ static int snd_pcm_rate_hwsync(snd_pcm_t *pcm)
static int snd_pcm_rate_delay(snd_pcm_t *pcm, snd_pcm_sframes_t *delayp) { - snd_pcm_rate_hwsync(pcm); - if (pcm->stream == SND_PCM_STREAM_PLAYBACK) - *delayp = snd_pcm_mmap_playback_hw_avail(pcm); - else - *delayp = snd_pcm_mmap_capture_hw_avail(pcm); + snd_pcm_rate_t *rate = pcm->private_data; + + snd_pcm_hwsync(rate->gen.slave); + *delayp = _snd_pcm_calc_avail(pcm, snd_pcm_rate_get_hwptr(pcm), + *pcm->appl.ptr); return 0; }
Hi Takashi,
I believe that we have a workable solution that does not break the paradigm established by ALSA conforming to calls made exclusively in a single thread or in multiple threads with the using application providing locking (less efficient). There is a disconnect here with developers using ALSA in their applications and not fully understanding the paradigm or the ramifications of using it incorrectly in a multithreaded environment. I have seen old threads (no pun intended) where thread safety and ALSA has come up before and I am sorry for bringing it up yet again. I believe that Jarloslav's added caveat to the Wiki should help. As always, thanks for your quick and detail analysis.
Best Regards,
Rob Krakora
participants (7)
-
Clemens Ladisch -
David Henningsson -
Jaroslav Kysela -
Krakora Robert-B42341 -
Rémi Denis-Courmont -
Takashi Iwai -
Trent Piepho