[PATCH - alsa-lib] pcm/ioplug: Add mutex in snd_pcm_ioplug_hw_ptr_update

[PATCH - alsa-lib] pcm/ioplug: Add mutex in snd_pcm_ioplug_hw_ptr_update

[Vishal Agrawal]

[-- Attachment #1: Type: text/plain, Size: 380 bytes --]

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

[-- Attachment #2: 0001-pcm-ioplug-Add-mutex-in-snd_pcm_ioplug_hw_ptr_update.patch --]
[-- Type: text/x-patch, Size: 1400 bytes --]

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


[-- Attachment #3: Type: text/plain, Size: 0 bytes --]

Re: [PATCH - alsa-lib] pcm/ioplug: Add mutex in snd_pcm_ioplug_hw_ptr_update

[Takashi Iwai]

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
> 

Re: [PATCH - alsa-lib] pcm/ioplug: Add mutex in snd_pcm_ioplug_hw_ptr_update

[Vishal Agrawal]

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
> >
>

Re: [PATCH - alsa-lib] pcm/ioplug: Add mutex in snd_pcm_ioplug_hw_ptr_update

[Takashi Iwai]

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)>]
> 

Re: [PATCH - alsa-lib] pcm/ioplug: Add mutex in snd_pcm_ioplug_hw_ptr_update

[Vishal Agrawal]

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)>]
> >
>

Re: [PATCH - alsa-lib] pcm/ioplug: Add mutex in snd_pcm_ioplug_hw_ptr_update

[Takashi Iwai]

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)>]
> 

Re: [PATCH - alsa-lib] pcm/ioplug: Add mutex in snd_pcm_ioplug_hw_ptr_update

[Vishal Agrawal]

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
> 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)>]
> >
>