From mboxrd@z Thu Jan 1 00:00:00 1970 From: Vishal Agrawal Subject: Re: [PATCH - alsa-lib] pcm/ioplug: Add mutex in snd_pcm_ioplug_hw_ptr_update Date: Tue, 29 Oct 2013 19:02:20 +0530 Message-ID: References: Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail-wg0-f52.google.com (mail-wg0-f52.google.com [74.125.82.52]) by alsa0.perex.cz (Postfix) with ESMTP id CCDA1261B1B for ; Tue, 29 Oct 2013 14:32:20 +0100 (CET) Received: by mail-wg0-f52.google.com with SMTP id k14so3434986wgh.7 for ; Tue, 29 Oct 2013 06:32:20 -0700 (PDT) In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: alsa-devel-bounces@alsa-project.org Sender: alsa-devel-bounces@alsa-project.org To: Takashi Iwai Cc: alsa-devel@alsa-project.org List-Id: alsa-devel@alsa-project.org 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 wrote: > At Tue, 29 Oct 2013 17:23:30 +0530, > Vishal Agrawal wrote: > > > > [1 ] > > 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 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 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 ] > > > > > > > > > > > > [2 > 0001-pcm-ioplug-Add-mutex-in-snd_pcm_ioplug_hw_ptr_update.patch > > > > > ] > > > > > > From b826b1f28c50bd4a414cf50751be4ac74b4e4174 Mon Sep 17 00:00:00 > > > 2001 > > > > > > From: Vishal Agrawal > > > > > > 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 > > > > > > > > > > > > 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 > > > > > > #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 ] > > > > > > > > > [2 ] > > >