[alsa-devel] [PATCH - alsa-lib] pcm/ioplug: Add mutex in snd_pcm_ioplug_hw_ptr_update
Function snd_pcm_ioplug_hw_ptr_update can be called from many threads, we need to protect the hw_ptr. I came across this issue with GStreamer and Jackd. Function will be called from the thread doing snd_pcm_write and another would be to get the playback position for the track bar. Change in itself is very minimum, but it took me a lot of time to debug. Thanks, Vishal Agrawal
At Tue, 29 Oct 2013 15:52:16 +0530, Vishal Agrawal wrote:
Function snd_pcm_ioplug_hw_ptr_update can be called from many threads, we need to protect the hw_ptr. I came across this issue with GStreamer and Jackd. Function will be called from the thread doing snd_pcm_write and another would be to get the playback position for the track bar.
Change in itself is very minimum, but it took me a lot of time to debug.
We'd like to avoid mutex in the alsa-lib code as much as possible. In which code paths did you get the race? In general, the code paths involved with hw_ptr_update() are known to be thread-unsafe, so it's the responsibility of the caller side to protect the race. thanks, Takashi
Thanks, Vishal Agrawal [1.2 <text/html; ISO-8859-1 (quoted-printable)>]
[2 0001-pcm-ioplug-Add-mutex-in-snd_pcm_ioplug_hw_ptr_update.patch <text/x-patch; US-ASCII (base64)>] From b826b1f28c50bd4a414cf50751be4ac74b4e4174 Mon Sep 17 00:00:00 2001 From: Vishal Agrawal <visagrawal@gmail.com> Date: Mon, 28 Oct 2013 18:01:14 +0530 Subject: [PATCH - alsa-lib] pcm/ioplug: Add mutex in snd_pcm_ioplug_hw_ptr_update
snd_pcm_ioplug_hw_ptr_update() can be called from different threads, it needs to be protected by mutex
Signed-off-by: Vishal Agrawal <visagrawal@gmail.com>
diff --git a/src/pcm/pcm_ioplug.c b/src/pcm/pcm_ioplug.c index a90c844..23dbee3 100644 --- a/src/pcm/pcm_ioplug.c +++ b/src/pcm/pcm_ioplug.c @@ -26,6 +26,7 @@ * */
+#include <pthread.h> #include "pcm_local.h" #include "pcm_ioplug.h" #include "pcm_ext_parm.h" @@ -47,12 +48,15 @@ typedef struct snd_pcm_ioplug_priv { snd_htimestamp_t trigger_tstamp; } ioplug_priv_t;
+static pthread_mutex_t hw_ptr_mutex = PTHREAD_MUTEX_INITIALIZER; + /* update the hw pointer */ static void snd_pcm_ioplug_hw_ptr_update(snd_pcm_t *pcm) { ioplug_priv_t *io = pcm->private_data; snd_pcm_sframes_t hw;
+ pthread_mutex_lock(&hw_ptr_mutex); hw = io->data->callback->pointer(io->data); if (hw >= 0) { unsigned int delta; @@ -64,6 +68,7 @@ static void snd_pcm_ioplug_hw_ptr_update(snd_pcm_t *pcm) io->last_hw = hw; } else io->data->state = SNDRV_PCM_STATE_XRUN; + pthread_mutex_unlock(&hw_ptr_mutex); }
static int snd_pcm_ioplug_info(snd_pcm_t *pcm, snd_pcm_info_t *info) -- 1.7.9.5
Hi Takashi, Thanks for the reply. I saw the race between "delay" and "avail_update" function.
From GStreamer stack these two function gets called from different threads.
Caller is calling two different function here it doesn't know about race condition. And this race condition doesn't happen with HW plug-in, Its only with ioplug plug-in. Thanks, Vishal Agrawal On Tue, Oct 29, 2013 at 4:12 PM, Takashi Iwai <tiwai@suse.de> wrote:
At Tue, 29 Oct 2013 15:52:16 +0530, Vishal Agrawal wrote:
Function snd_pcm_ioplug_hw_ptr_update can be called from many threads, we need to protect the hw_ptr. I came across this issue with GStreamer and Jackd. Function will be called from the thread doing snd_pcm_write and another would be to get the playback position for the track bar.
Change in itself is very minimum, but it took me a lot of time to debug.
We'd like to avoid mutex in the alsa-lib code as much as possible. In which code paths did you get the race? In general, the code paths involved with hw_ptr_update() are known to be thread-unsafe, so it's the responsibility of the caller side to protect the race.
thanks,
Takashi
Thanks, Vishal Agrawal [1.2 <text/html; ISO-8859-1 (quoted-printable)>]
[2 0001-pcm-ioplug-Add-mutex-in-snd_pcm_ioplug_hw_ptr_update.patch
<text/x-patch; US-ASCII (base64)>]
From b826b1f28c50bd4a414cf50751be4ac74b4e4174 Mon Sep 17 00:00:00 2001 From: Vishal Agrawal <visagrawal@gmail.com> Date: Mon, 28 Oct 2013 18:01:14 +0530 Subject: [PATCH - alsa-lib] pcm/ioplug: Add mutex in snd_pcm_ioplug_hw_ptr_update
snd_pcm_ioplug_hw_ptr_update() can be called from different threads, it needs to be protected by mutex
Signed-off-by: Vishal Agrawal <visagrawal@gmail.com>
diff --git a/src/pcm/pcm_ioplug.c b/src/pcm/pcm_ioplug.c index a90c844..23dbee3 100644 --- a/src/pcm/pcm_ioplug.c +++ b/src/pcm/pcm_ioplug.c @@ -26,6 +26,7 @@ * */
+#include <pthread.h> #include "pcm_local.h" #include "pcm_ioplug.h" #include "pcm_ext_parm.h" @@ -47,12 +48,15 @@ typedef struct snd_pcm_ioplug_priv { snd_htimestamp_t trigger_tstamp; } ioplug_priv_t;
+static pthread_mutex_t hw_ptr_mutex = PTHREAD_MUTEX_INITIALIZER; + /* update the hw pointer */ static void snd_pcm_ioplug_hw_ptr_update(snd_pcm_t *pcm) { ioplug_priv_t *io = pcm->private_data; snd_pcm_sframes_t hw;
+ pthread_mutex_lock(&hw_ptr_mutex); hw = io->data->callback->pointer(io->data); if (hw >= 0) { unsigned int delta; @@ -64,6 +68,7 @@ static void snd_pcm_ioplug_hw_ptr_update(snd_pcm_t *pcm) io->last_hw = hw; } else io->data->state = SNDRV_PCM_STATE_XRUN; + pthread_mutex_unlock(&hw_ptr_mutex); }
static int snd_pcm_ioplug_info(snd_pcm_t *pcm, snd_pcm_info_t *info) -- 1.7.9.5
At Tue, 29 Oct 2013 17:09:38 +0530, Vishal Agrawal wrote:
Hi Takashi,
Thanks for the reply. I saw the race between "delay" and "avail_update" function.
From GStreamer stack these two function gets called from different threads.
Caller is calling two different function here it doesn't know about race condition. And this race condition doesn't happen with HW plug-in, Its only with ioplug plug-in.
It doesn't justify to put the hack in ioplug plugin. The similar problem may happen in every plugin; i.e. calling these two in different threads aren't guaranteed to work without protection in the caller side. Takashi
Thanks, Vishal Agrawal
On Tue, Oct 29, 2013 at 4:12 PM, Takashi Iwai <tiwai@suse.de> wrote:
At Tue, 29 Oct 2013 15:52:16 +0530, Vishal Agrawal wrote:
Function snd_pcm_ioplug_hw_ptr_update can be called from many threads, we need to protect the hw_ptr. I came across this issue with GStreamer and Jackd. Function will be called from the thread doing snd_pcm_write and another would be to get the playback position for the track bar.
Change in itself is very minimum, but it took me a lot of time to debug.
We'd like to avoid mutex in the alsa-lib code as much as possible. In which code paths did you get the race? In general, the code paths involved with hw_ptr_update() are known to be thread-unsafe, so it's the responsibility of the caller side to protect the race.
thanks,
Takashi
Thanks, Vishal Agrawal [1.2 <text/html; ISO-8859-1 (quoted-printable)>]
[2 0001-pcm-ioplug-Add-mutex-in-snd_pcm_ioplug_hw_ptr_update.patch
<text/x-patch; US-ASCII (base64)>]
From b826b1f28c50bd4a414cf50751be4ac74b4e4174 Mon Sep 17 00:00:00 2001 From: Vishal Agrawal <visagrawal@gmail.com> Date: Mon, 28 Oct 2013 18:01:14 +0530 Subject: [PATCH - alsa-lib] pcm/ioplug: Add mutex in snd_pcm_ioplug_hw_ptr_update
snd_pcm_ioplug_hw_ptr_update() can be called from different threads, it needs to be protected by mutex
Signed-off-by: Vishal Agrawal <visagrawal@gmail.com>
diff --git a/src/pcm/pcm_ioplug.c b/src/pcm/pcm_ioplug.c index a90c844..23dbee3 100644 --- a/src/pcm/pcm_ioplug.c +++ b/src/pcm/pcm_ioplug.c @@ -26,6 +26,7 @@ * */
+#include <pthread.h> #include "pcm_local.h" #include "pcm_ioplug.h" #include "pcm_ext_parm.h" @@ -47,12 +48,15 @@ typedef struct snd_pcm_ioplug_priv { snd_htimestamp_t trigger_tstamp; } ioplug_priv_t;
+static pthread_mutex_t hw_ptr_mutex = PTHREAD_MUTEX_INITIALIZER; + /* update the hw pointer */ static void snd_pcm_ioplug_hw_ptr_update(snd_pcm_t *pcm) { ioplug_priv_t *io = pcm->private_data; snd_pcm_sframes_t hw;
+ pthread_mutex_lock(&hw_ptr_mutex); hw = io->data->callback->pointer(io->data); if (hw >= 0) { unsigned int delta; @@ -64,6 +68,7 @@ static void snd_pcm_ioplug_hw_ptr_update(snd_pcm_t *pcm) io->last_hw = hw; } else io->data->state = SNDRV_PCM_STATE_XRUN; + pthread_mutex_unlock(&hw_ptr_mutex); }
static int snd_pcm_ioplug_info(snd_pcm_t *pcm, snd_pcm_info_t *info) -- 1.7.9.5
[2 <text/html; ISO-8859-1 (quoted-printable)>]
Hi Takashi, Thanks again. So basically it means ALSA APIs are not thread safe? In the http://www.alsa-project.org/main/index.php/SMP_Design its mentioned that - "The snd_*_open() calls are thread safe. The use of returned handles must be serialized in the application using own locking scheme. Standalone (not handle related) functions in alsa-lib should be fully thread safe." I could not understand what are "Standalone (not handle related) functions"? Can you please give me an example? My understanding was apart from snd_*_open() and snd_*_close() all other functions are thread safe. Thanks, Vishal Agrawal On Tue, Oct 29, 2013 at 5:20 PM, Takashi Iwai <tiwai@suse.de> wrote:
At Tue, 29 Oct 2013 17:09:38 +0530, Vishal Agrawal wrote:
Hi Takashi,
Thanks for the reply. I saw the race between "delay" and "avail_update" function.
From GStreamer stack these two function gets called from different
threads.
Caller is calling two different function here it doesn't know about race condition. And this race condition doesn't happen with HW plug-in, Its only with ioplug plug-in.
It doesn't justify to put the hack in ioplug plugin.
The similar problem may happen in every plugin; i.e. calling these two in different threads aren't guaranteed to work without protection in the caller side.
Takashi
Thanks, Vishal Agrawal
On Tue, Oct 29, 2013 at 4:12 PM, Takashi Iwai <tiwai@suse.de> wrote:
At Tue, 29 Oct 2013 15:52:16 +0530, Vishal Agrawal wrote:
Function snd_pcm_ioplug_hw_ptr_update can be called from many
threads, we
need to protect the hw_ptr. I came across this issue with GStreamer and Jackd. Function will be called from the thread doing snd_pcm_write and another would be to get the playback position for the track bar.
Change in itself is very minimum, but it took me a lot of time to debug.
We'd like to avoid mutex in the alsa-lib code as much as possible. In which code paths did you get the race? In general, the code paths involved with hw_ptr_update() are known to be thread-unsafe, so it's the responsibility of the caller side to protect the race.
thanks,
Takashi
Thanks, Vishal Agrawal [1.2 <text/html; ISO-8859-1 (quoted-printable)>]
[2 0001-pcm-ioplug-Add-mutex-in-snd_pcm_ioplug_hw_ptr_update.patch
<text/x-patch; US-ASCII (base64)>]
From b826b1f28c50bd4a414cf50751be4ac74b4e4174 Mon Sep 17 00:00:00 2001 From: Vishal Agrawal <visagrawal@gmail.com> Date: Mon, 28 Oct 2013 18:01:14 +0530 Subject: [PATCH - alsa-lib] pcm/ioplug: Add mutex in snd_pcm_ioplug_hw_ptr_update
snd_pcm_ioplug_hw_ptr_update() can be called from different threads, it needs to be protected by mutex
Signed-off-by: Vishal Agrawal <visagrawal@gmail.com>
diff --git a/src/pcm/pcm_ioplug.c b/src/pcm/pcm_ioplug.c index a90c844..23dbee3 100644 --- a/src/pcm/pcm_ioplug.c +++ b/src/pcm/pcm_ioplug.c @@ -26,6 +26,7 @@ * */
+#include <pthread.h> #include "pcm_local.h" #include "pcm_ioplug.h" #include "pcm_ext_parm.h" @@ -47,12 +48,15 @@ typedef struct snd_pcm_ioplug_priv { snd_htimestamp_t trigger_tstamp; } ioplug_priv_t;
+static pthread_mutex_t hw_ptr_mutex = PTHREAD_MUTEX_INITIALIZER; + /* update the hw pointer */ static void snd_pcm_ioplug_hw_ptr_update(snd_pcm_t *pcm) { ioplug_priv_t *io = pcm->private_data; snd_pcm_sframes_t hw;
+ pthread_mutex_lock(&hw_ptr_mutex); hw = io->data->callback->pointer(io->data); if (hw >= 0) { unsigned int delta; @@ -64,6 +68,7 @@ static void snd_pcm_ioplug_hw_ptr_update(snd_pcm_t *pcm) io->last_hw = hw; } else io->data->state = SNDRV_PCM_STATE_XRUN; + pthread_mutex_unlock(&hw_ptr_mutex); }
static int snd_pcm_ioplug_info(snd_pcm_t *pcm, snd_pcm_info_t *info) -- 1.7.9.5
[2 <text/html; ISO-8859-1 (quoted-printable)>]
At Tue, 29 Oct 2013 17:23:30 +0530, Vishal Agrawal wrote:
[1 <text/plain; ISO-8859-1 (7bit)>] Hi Takashi,
Thanks again. So basically it means ALSA APIs are not thread safe?
In the http://www.alsa-project.org/main/index.php/SMP_Design its mentioned that - "The snd_*_open() calls are thread safe. The use of returned handles must be serialized in the application using own locking scheme. Standalone (not handle related) functions in alsa-lib should be fully thread safe."
I could not understand what are "Standalone (not handle related) functions"? Can you please give me an example? My understanding was apart from snd_*_open() and snd_*_close() all other functions are thread safe.
The functions that are not handle-related are the functions that don't have arguments with a handle pointer like snd_pcm_t. All these functions are thread unsafe. Takashi
Thanks, Vishal Agrawal
On Tue, Oct 29, 2013 at 5:20 PM, Takashi Iwai <tiwai@suse.de> wrote:
At Tue, 29 Oct 2013 17:09:38 +0530, Vishal Agrawal wrote:
Hi Takashi,
Thanks for the reply. I saw the race between "delay" and "avail_update" function.
From GStreamer stack these two function gets called from different
threads.
Caller is calling two different function here it doesn't know about race condition. And this race condition doesn't happen with HW plug-in, Its only with ioplug plug-in.
It doesn't justify to put the hack in ioplug plugin.
The similar problem may happen in every plugin; i.e. calling these two in different threads aren't guaranteed to work without protection in the caller side.
Takashi
Thanks, Vishal Agrawal
On Tue, Oct 29, 2013 at 4:12 PM, Takashi Iwai <tiwai@suse.de> wrote:
At Tue, 29 Oct 2013 15:52:16 +0530, Vishal Agrawal wrote:
Function snd_pcm_ioplug_hw_ptr_update can be called from many
threads, we
need to protect the hw_ptr. I came across this issue with GStreamer and Jackd. Function will be called from the thread doing snd_pcm_write and another would be to get the playback position for the track bar.
Change in itself is very minimum, but it took me a lot of time to debug.
We'd like to avoid mutex in the alsa-lib code as much as possible. In which code paths did you get the race? In general, the code paths involved with hw_ptr_update() are known to be thread-unsafe, so it's the responsibility of the caller side to protect the race.
thanks,
Takashi
Thanks, Vishal Agrawal [1.2 <text/html; ISO-8859-1 (quoted-printable)>]
[2 0001-pcm-ioplug-Add-mutex-in-snd_pcm_ioplug_hw_ptr_update.patch
<text/x-patch; US-ASCII (base64)>]
From b826b1f28c50bd4a414cf50751be4ac74b4e4174 Mon Sep 17 00:00:00 2001 From: Vishal Agrawal <visagrawal@gmail.com> Date: Mon, 28 Oct 2013 18:01:14 +0530 Subject: [PATCH - alsa-lib] pcm/ioplug: Add mutex in snd_pcm_ioplug_hw_ptr_update
snd_pcm_ioplug_hw_ptr_update() can be called from different threads, it needs to be protected by mutex
Signed-off-by: Vishal Agrawal <visagrawal@gmail.com>
diff --git a/src/pcm/pcm_ioplug.c b/src/pcm/pcm_ioplug.c index a90c844..23dbee3 100644 --- a/src/pcm/pcm_ioplug.c +++ b/src/pcm/pcm_ioplug.c @@ -26,6 +26,7 @@ * */
+#include <pthread.h> #include "pcm_local.h" #include "pcm_ioplug.h" #include "pcm_ext_parm.h" @@ -47,12 +48,15 @@ typedef struct snd_pcm_ioplug_priv { snd_htimestamp_t trigger_tstamp; } ioplug_priv_t;
+static pthread_mutex_t hw_ptr_mutex = PTHREAD_MUTEX_INITIALIZER; + /* update the hw pointer */ static void snd_pcm_ioplug_hw_ptr_update(snd_pcm_t *pcm) { ioplug_priv_t *io = pcm->private_data; snd_pcm_sframes_t hw;
+ pthread_mutex_lock(&hw_ptr_mutex); hw = io->data->callback->pointer(io->data); if (hw >= 0) { unsigned int delta; @@ -64,6 +68,7 @@ static void snd_pcm_ioplug_hw_ptr_update(snd_pcm_t *pcm) io->last_hw = hw; } else io->data->state = SNDRV_PCM_STATE_XRUN; + pthread_mutex_unlock(&hw_ptr_mutex); }
static int snd_pcm_ioplug_info(snd_pcm_t *pcm, snd_pcm_info_t *info) -- 1.7.9.5
[2 <text/html; ISO-8859-1 (quoted-printable)>]
[2 <text/html; ISO-8859-1 (quoted-printable)>]
I checked the latest GStreamer code they have taken care of this. Thanks for your time Takashi. On Tue, Oct 29, 2013 at 6:11 PM, Takashi Iwai <tiwai@suse.de> wrote:
At Tue, 29 Oct 2013 17:23:30 +0530, Vishal Agrawal wrote:
[1 <text/plain; ISO-8859-1 (7bit)>] Hi Takashi,
Thanks again. So basically it means ALSA APIs are not thread safe?
In the http://www.alsa-project.org/main/index.php/SMP_Design its
mentioned
that - "The snd_*_open() calls are thread safe. The use of returned handles must be serialized in the application using own locking scheme. Standalone (not handle related) functions in alsa-lib should be fully thread safe."
I could not understand what are "Standalone (not handle related) functions"? Can you please give me an example? My understanding was apart from snd_*_open() and snd_*_close() all other functions are thread safe.
The functions that are not handle-related are the functions that don't have arguments with a handle pointer like snd_pcm_t. All these functions are thread unsafe.
Takashi
Thanks, Vishal Agrawal
On Tue, Oct 29, 2013 at 5:20 PM, Takashi Iwai <tiwai@suse.de> wrote:
At Tue, 29 Oct 2013 17:09:38 +0530, Vishal Agrawal wrote:
Hi Takashi,
Thanks for the reply. I saw the race between "delay" and "avail_update" function.
From GStreamer stack these two function gets called from different
threads.
Caller is calling two different function here it doesn't know about
condition. And this race condition doesn't happen with HW plug-in, Its only with ioplug plug-in.
It doesn't justify to put the hack in ioplug plugin.
The similar problem may happen in every plugin; i.e. calling these two in different threads aren't guaranteed to work without protection in the caller side.
Takashi
Thanks, Vishal Agrawal
On Tue, Oct 29, 2013 at 4:12 PM, Takashi Iwai <tiwai@suse.de> wrote:
At Tue, 29 Oct 2013 15:52:16 +0530, Vishal Agrawal wrote:
Function snd_pcm_ioplug_hw_ptr_update can be called from many
threads, we
need to protect the hw_ptr. I came across this issue with GStreamer and Jackd. Function will be called from the thread doing snd_pcm_write and another would be to get the playback position for the track bar.
Change in itself is very minimum, but it took me a lot of time to debug.
We'd like to avoid mutex in the alsa-lib code as much as possible. In which code paths did you get the race? In general, the code
race paths
involved with hw_ptr_update() are known to be thread-unsafe, so it's the responsibility of the caller side to protect the race.
thanks,
Takashi
Thanks, Vishal Agrawal [1.2 <text/html; ISO-8859-1 (quoted-printable)>]
[2
0001-pcm-ioplug-Add-mutex-in-snd_pcm_ioplug_hw_ptr_update.patch <text/x-patch; US-ASCII (base64)>]
From b826b1f28c50bd4a414cf50751be4ac74b4e4174 Mon Sep 17 00:00:00 2001 From: Vishal Agrawal <visagrawal@gmail.com> Date: Mon, 28 Oct 2013 18:01:14 +0530 Subject: [PATCH - alsa-lib] pcm/ioplug: Add mutex in snd_pcm_ioplug_hw_ptr_update
snd_pcm_ioplug_hw_ptr_update() can be called from different threads, it needs to be protected by mutex
Signed-off-by: Vishal Agrawal <visagrawal@gmail.com>
diff --git a/src/pcm/pcm_ioplug.c b/src/pcm/pcm_ioplug.c index a90c844..23dbee3 100644 --- a/src/pcm/pcm_ioplug.c +++ b/src/pcm/pcm_ioplug.c @@ -26,6 +26,7 @@ * */
+#include <pthread.h> #include "pcm_local.h" #include "pcm_ioplug.h" #include "pcm_ext_parm.h" @@ -47,12 +48,15 @@ typedef struct snd_pcm_ioplug_priv { snd_htimestamp_t trigger_tstamp; } ioplug_priv_t;
+static pthread_mutex_t hw_ptr_mutex = PTHREAD_MUTEX_INITIALIZER; + /* update the hw pointer */ static void snd_pcm_ioplug_hw_ptr_update(snd_pcm_t *pcm) { ioplug_priv_t *io = pcm->private_data; snd_pcm_sframes_t hw;
+ pthread_mutex_lock(&hw_ptr_mutex); hw = io->data->callback->pointer(io->data); if (hw >= 0) { unsigned int delta; @@ -64,6 +68,7 @@ static void snd_pcm_ioplug_hw_ptr_update(snd_pcm_t *pcm) io->last_hw = hw; } else io->data->state = SNDRV_PCM_STATE_XRUN; + pthread_mutex_unlock(&hw_ptr_mutex); }
static int snd_pcm_ioplug_info(snd_pcm_t *pcm, snd_pcm_info_t *info) -- 1.7.9.5
[2 <text/html; ISO-8859-1 (quoted-printable)>]
[2 <text/html; ISO-8859-1 (quoted-printable)>]
participants (2)
-
Takashi Iwai -
Vishal Agrawal