[PATCH] pcm_hw: Respect LIBASOUND_THREAD_SAFE env var

[PATCH] pcm_hw: Respect LIBASOUND_THREAD_SAFE env var

[David Henningsson]

Without this patch, there no way to revert to the old
behaviour with pcm_hw, so currently there will always
be some extra locking.

This seems to be an oversight?

Reported-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
Signed-off-by: David Henningsson <diwic@ubuntu.com>
---
 src/pcm/pcm_hw.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/pcm/pcm_hw.c b/src/pcm/pcm_hw.c
index 3a5634c..e6a8a4f 100644
--- a/src/pcm/pcm_hw.c
+++ b/src/pcm/pcm_hw.c
@@ -1514,7 +1514,8 @@ int snd_pcm_hw_open_fd(snd_pcm_t **pcmp, const char *name,
 	pcm->poll_events = info.stream == SND_PCM_STREAM_PLAYBACK ? POLLOUT : POLLIN;
 	pcm->tstamp_type = tstamp_type;
 #ifdef THREAD_SAFE_API
-	pcm->thread_safe = 1;
+	if (pcm->thread_safe != -1)
+		pcm->thread_safe = 1;
 #endif
 
 	ret = snd_pcm_hw_mmap_status(pcm);
-- 
1.9.1

Re: [PATCH] pcm_hw: Respect LIBASOUND_THREAD_SAFE env var

[Takashi Iwai]

On Thu, 01 Sep 2016 08:06:50 +0200,
David Henningsson wrote:
> 
> Without this patch, there no way to revert to the old
> behaviour with pcm_hw, so currently there will always
> be some extra locking.
> 
> This seems to be an oversight?

thread_safe=1 means that the plugin needs no locking, so it means hw
PCM never takes a lock by itself.  Only other plugins do, and it's
suppressed when $LIBASOUND_THREAD_SAFE=0 is passed.

The flag needs a better description or a better name...


Takashi


> Reported-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
> Signed-off-by: David Henningsson <diwic@ubuntu.com>
> ---
>  src/pcm/pcm_hw.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/src/pcm/pcm_hw.c b/src/pcm/pcm_hw.c
> index 3a5634c..e6a8a4f 100644
> --- a/src/pcm/pcm_hw.c
> +++ b/src/pcm/pcm_hw.c
> @@ -1514,7 +1514,8 @@ int snd_pcm_hw_open_fd(snd_pcm_t **pcmp, const char *name,
>  	pcm->poll_events = info.stream == SND_PCM_STREAM_PLAYBACK ? POLLOUT : POLLIN;
>  	pcm->tstamp_type = tstamp_type;
>  #ifdef THREAD_SAFE_API
> -	pcm->thread_safe = 1;
> +	if (pcm->thread_safe != -1)
> +		pcm->thread_safe = 1;
>  #endif
>  
>  	ret = snd_pcm_hw_mmap_status(pcm);
> -- 
> 1.9.1
> 

Re: [PATCH] pcm_hw: Respect LIBASOUND_THREAD_SAFE env var

[Takashi Iwai]

On Thu, 01 Sep 2016 08:51:14 +0200,
Takashi Iwai wrote:
> 
> On Thu, 01 Sep 2016 08:06:50 +0200,
> David Henningsson wrote:
> > 
> > Without this patch, there no way to revert to the old
> > behaviour with pcm_hw, so currently there will always
> > be some extra locking.
> > 
> > This seems to be an oversight?
> 
> thread_safe=1 means that the plugin needs no locking, so it means hw
> PCM never takes a lock by itself.  Only other plugins do, and it's
> suppressed when $LIBASOUND_THREAD_SAFE=0 is passed.
> 
> The flag needs a better description or a better name...

So here we go, let's decrypt the monkey code.
Let me know if this is good enough.


Takashi

-- 8< --
From: Takashi Iwai <tiwai@suse.de>
Subject: [PATCH] pcm: Better understandable locking code

The newly added locking code seems to have confused quite a few
people, as "thread_safe=1" may be considered as if the thread-safety
lock has to be turned on.  (It meant that the plugin _is_ thread-safe,
i.e. it needs no extra locking.)

For avoiding such a misunderstanding, this commit renames the relevant
pcm fields and give more comments to explain what is for what.
The former single pcm->thread_safe flag is now split to two boolean
flags, pcm->need_lock and pcm->lock_enabled.  It consumes a few more
bytes, but this would be (hopefully) better understandable.

No functional change by this commit.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 src/pcm/pcm.c       | 16 +++++++++++-----
 src/pcm/pcm_hw.c    |  2 +-
 src/pcm/pcm_local.h | 27 ++++++++++++++++++++++-----
 3 files changed, 34 insertions(+), 11 deletions(-)

diff --git a/src/pcm/pcm.c b/src/pcm/pcm.c
index f8323999343e..c96ae1c4641a 100644
--- a/src/pcm/pcm.c
+++ b/src/pcm/pcm.c
@@ -2545,14 +2545,20 @@ int snd_pcm_new(snd_pcm_t **pcmp, snd_pcm_type_t type, const char *name,
 	INIT_LIST_HEAD(&pcm->async_handlers);
 #ifdef THREAD_SAFE_API
 	pthread_mutex_init(&pcm->lock, NULL);
+	/* use locking as default;
+	 * each plugin may suppress this in its open call
+	 */
+	pcm->need_lock = 1;
 	{
-		static int default_thread_safe = -1;
-		if (default_thread_safe < 0) {
+		/* set lock_enabled field depending on $LIBASOUND_THREAD_SAFE;
+		 * it's evaluated only once at the first open for consistency
+		 */
+		static int do_lock_enable = -1;
+		if (do_lock_enable < 0) {
 			char *p = getenv("LIBASOUND_THREAD_SAFE");
-			default_thread_safe = !p || *p != '0';
+			do_lock_enable = !p || *p != '0';
 		}
-		if (!default_thread_safe)
-			pcm->thread_safe = -1; /* force to disable */
+		pcm->lock_enabled = do_lock_enable;
 	}
 #endif
 	*pcmp = pcm;
diff --git a/src/pcm/pcm_hw.c b/src/pcm/pcm_hw.c
index 3a5634c1d39a..56e88b6bf6c0 100644
--- a/src/pcm/pcm_hw.c
+++ b/src/pcm/pcm_hw.c
@@ -1514,7 +1514,7 @@ int snd_pcm_hw_open_fd(snd_pcm_t **pcmp, const char *name,
 	pcm->poll_events = info.stream == SND_PCM_STREAM_PLAYBACK ? POLLOUT : POLLIN;
 	pcm->tstamp_type = tstamp_type;
 #ifdef THREAD_SAFE_API
-	pcm->thread_safe = 1;
+	pcm->need_lock = 0;	/* hw plugin is thread-safe */
 #endif
 
 	ret = snd_pcm_hw_mmap_status(pcm);
diff --git a/src/pcm/pcm_local.h b/src/pcm/pcm_local.h
index bb7964d7833e..bba2f15ac463 100644
--- a/src/pcm/pcm_local.h
+++ b/src/pcm/pcm_local.h
@@ -244,7 +244,12 @@ struct _snd_pcm {
 	void *private_data;
 	struct list_head async_handlers;
 #ifdef THREAD_SAFE_API
-	int thread_safe;
+	int need_lock;		/* true = this PCM (plugin) is thread-unsafe,
+				 * thus it needs a lock.
+				 */
+	int lock_enabled;	/* thread-safety lock is enabled on the system;
+				 * it's set depending on $LIBASOUND_THREAD_SAFE.
+				 */
 	pthread_mutex_t lock;
 #endif
 };
@@ -1085,24 +1090,36 @@ static inline void sw_set_period_event(snd_pcm_sw_params_t *params, int val)
 #define PCMINABORT(pcm) (((pcm)->mode & SND_PCM_ABORT) != 0)
 
 #ifdef THREAD_SAFE_API
+/*
+ * __snd_pcm_lock() and __snd_pcm_unlock() are used to lock/unlock the plugin
+ * forcibly even if it's declared as thread-safe.  It's needed only for some
+ * codes that are thread-unsafe per design (e.g. snd_pcm_nonblock()).
+ *
+ * OTOH, snd_pcm_lock() and snd_pcm_unlock() are used to lock/unlock the plugin
+ * in normal situations.  They do lock/unlock only when the plugin is
+ * thread-unsafe.
+ *
+ * Both __snd_pcm_lock() and snd_pcm_lock() (and their unlocks) wouldn't do
+ * any action when the whole locking is disabled via $LIBASOUND_THREAD_SAFE=0.
+ */
 static inline void __snd_pcm_lock(snd_pcm_t *pcm)
 {
-	if (pcm->thread_safe >= 0)
+	if (pcm->lock_enabled)
 		pthread_mutex_lock(&pcm->lock);
 }
 static inline void __snd_pcm_unlock(snd_pcm_t *pcm)
 {
-	if (pcm->thread_safe >= 0)
+	if (pcm->lock_enabled)
 		pthread_mutex_unlock(&pcm->lock);
 }
 static inline void snd_pcm_lock(snd_pcm_t *pcm)
 {
-	if (!pcm->thread_safe)
+	if (pcm->lock_enabled && pcm->need_lock)
 		pthread_mutex_lock(&pcm->lock);
 }
 static inline void snd_pcm_unlock(snd_pcm_t *pcm)
 {
-	if (!pcm->thread_safe)
+	if (pcm->lock_enabled && pcm->need_lock)
 		pthread_mutex_unlock(&pcm->lock);
 }
 #else /* THREAD_SAFE_API */
-- 
2.9.3

Re: [PATCH] pcm_hw: Respect LIBASOUND_THREAD_SAFE env var

[David Henningsson]

On 2016-09-01 14:28, Takashi Iwai wrote:
> On Thu, 01 Sep 2016 08:51:14 +0200,
> Takashi Iwai wrote:
>>
>> On Thu, 01 Sep 2016 08:06:50 +0200,
>> David Henningsson wrote:
>>>
>>> Without this patch, there no way to revert to the old
>>> behaviour with pcm_hw, so currently there will always
>>> be some extra locking.
>>>
>>> This seems to be an oversight?
>>
>> thread_safe=1 means that the plugin needs no locking, so it means hw
>> PCM never takes a lock by itself.  Only other plugins do, and it's
>> suppressed when $LIBASOUND_THREAD_SAFE=0 is passed.
>>
>> The flag needs a better description or a better name...
>
> So here we go, let's decrypt the monkey code.
> Let me know if this is good enough.

This is better, indeed. It also fixes the bug I sent a patch for 
(Respect LIBASOUND_THREAD_SAFE env var).

It's perhaps a bit inconsistent that "true" is -1 for lock_enabled and 1 
for need_lock, but that's just a nitpick.

Since many calls to __snd_pcm_lock has the comment "forced lock" perhaps 
it can be renamed to snd_pcm_lock_forced, and snd_pcm_lock can perhaps 
be snd_pcm_lock_if_needed?

>
>
> Takashi
>
> -- 8< --
> From: Takashi Iwai <tiwai@suse.de>
> Subject: [PATCH] pcm: Better understandable locking code
>
> The newly added locking code seems to have confused quite a few
> people, as "thread_safe=1" may be considered as if the thread-safety
> lock has to be turned on.  (It meant that the plugin _is_ thread-safe,
> i.e. it needs no extra locking.)
>
> For avoiding such a misunderstanding, this commit renames the relevant
> pcm fields and give more comments to explain what is for what.
> The former single pcm->thread_safe flag is now split to two boolean
> flags, pcm->need_lock and pcm->lock_enabled.  It consumes a few more
> bytes, but this would be (hopefully) better understandable.
>
> No functional change by this commit.
>
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ---
>  src/pcm/pcm.c       | 16 +++++++++++-----
>  src/pcm/pcm_hw.c    |  2 +-
>  src/pcm/pcm_local.h | 27 ++++++++++++++++++++++-----
>  3 files changed, 34 insertions(+), 11 deletions(-)
>
> diff --git a/src/pcm/pcm.c b/src/pcm/pcm.c
> index f8323999343e..c96ae1c4641a 100644
> --- a/src/pcm/pcm.c
> +++ b/src/pcm/pcm.c
> @@ -2545,14 +2545,20 @@ int snd_pcm_new(snd_pcm_t **pcmp, snd_pcm_type_t type, const char *name,
>  	INIT_LIST_HEAD(&pcm->async_handlers);
>  #ifdef THREAD_SAFE_API
>  	pthread_mutex_init(&pcm->lock, NULL);
> +	/* use locking as default;
> +	 * each plugin may suppress this in its open call
> +	 */
> +	pcm->need_lock = 1;
>  	{
> -		static int default_thread_safe = -1;
> -		if (default_thread_safe < 0) {
> +		/* set lock_enabled field depending on $LIBASOUND_THREAD_SAFE;
> +		 * it's evaluated only once at the first open for consistency
> +		 */
> +		static int do_lock_enable = -1;
> +		if (do_lock_enable < 0) {
>  			char *p = getenv("LIBASOUND_THREAD_SAFE");
> -			default_thread_safe = !p || *p != '0';
> +			do_lock_enable = !p || *p != '0';
>  		}
> -		if (!default_thread_safe)
> -			pcm->thread_safe = -1; /* force to disable */
> +		pcm->lock_enabled = do_lock_enable;
>  	}
>  #endif
>  	*pcmp = pcm;
> diff --git a/src/pcm/pcm_hw.c b/src/pcm/pcm_hw.c
> index 3a5634c1d39a..56e88b6bf6c0 100644
> --- a/src/pcm/pcm_hw.c
> +++ b/src/pcm/pcm_hw.c
> @@ -1514,7 +1514,7 @@ int snd_pcm_hw_open_fd(snd_pcm_t **pcmp, const char *name,
>  	pcm->poll_events = info.stream == SND_PCM_STREAM_PLAYBACK ? POLLOUT : POLLIN;
>  	pcm->tstamp_type = tstamp_type;
>  #ifdef THREAD_SAFE_API
> -	pcm->thread_safe = 1;
> +	pcm->need_lock = 0;	/* hw plugin is thread-safe */
>  #endif
>
>  	ret = snd_pcm_hw_mmap_status(pcm);
> diff --git a/src/pcm/pcm_local.h b/src/pcm/pcm_local.h
> index bb7964d7833e..bba2f15ac463 100644
> --- a/src/pcm/pcm_local.h
> +++ b/src/pcm/pcm_local.h
> @@ -244,7 +244,12 @@ struct _snd_pcm {
>  	void *private_data;
>  	struct list_head async_handlers;
>  #ifdef THREAD_SAFE_API
> -	int thread_safe;
> +	int need_lock;		/* true = this PCM (plugin) is thread-unsafe,
> +				 * thus it needs a lock.
> +				 */
> +	int lock_enabled;	/* thread-safety lock is enabled on the system;
> +				 * it's set depending on $LIBASOUND_THREAD_SAFE.
> +				 */
>  	pthread_mutex_t lock;
>  #endif
>  };
> @@ -1085,24 +1090,36 @@ static inline void sw_set_period_event(snd_pcm_sw_params_t *params, int val)
>  #define PCMINABORT(pcm) (((pcm)->mode & SND_PCM_ABORT) != 0)
>
>  #ifdef THREAD_SAFE_API
> +/*
> + * __snd_pcm_lock() and __snd_pcm_unlock() are used to lock/unlock the plugin
> + * forcibly even if it's declared as thread-safe.  It's needed only for some
> + * codes that are thread-unsafe per design (e.g. snd_pcm_nonblock()).
> + *
> + * OTOH, snd_pcm_lock() and snd_pcm_unlock() are used to lock/unlock the plugin
> + * in normal situations.  They do lock/unlock only when the plugin is
> + * thread-unsafe.
> + *
> + * Both __snd_pcm_lock() and snd_pcm_lock() (and their unlocks) wouldn't do
> + * any action when the whole locking is disabled via $LIBASOUND_THREAD_SAFE=0.
> + */
>  static inline void __snd_pcm_lock(snd_pcm_t *pcm)
>  {
> -	if (pcm->thread_safe >= 0)
> +	if (pcm->lock_enabled)
>  		pthread_mutex_lock(&pcm->lock);
>  }
>  static inline void __snd_pcm_unlock(snd_pcm_t *pcm)
>  {
> -	if (pcm->thread_safe >= 0)
> +	if (pcm->lock_enabled)
>  		pthread_mutex_unlock(&pcm->lock);
>  }
>  static inline void snd_pcm_lock(snd_pcm_t *pcm)
>  {
> -	if (!pcm->thread_safe)
> +	if (pcm->lock_enabled && pcm->need_lock)
>  		pthread_mutex_lock(&pcm->lock);
>  }
>  static inline void snd_pcm_unlock(snd_pcm_t *pcm)
>  {
> -	if (!pcm->thread_safe)
> +	if (pcm->lock_enabled && pcm->need_lock)
>  		pthread_mutex_unlock(&pcm->lock);
>  }
>  #else /* THREAD_SAFE_API */
>

Re: [PATCH] pcm_hw: Respect LIBASOUND_THREAD_SAFE env var

[Takashi Iwai]

On Thu, 01 Sep 2016 21:40:50 +0200,
David Henningsson wrote:
> 
> On 2016-09-01 14:28, Takashi Iwai wrote:
> > On Thu, 01 Sep 2016 08:51:14 +0200,
> > Takashi Iwai wrote:
> >>
> >> On Thu, 01 Sep 2016 08:06:50 +0200,
> >> David Henningsson wrote:
> >>>
> >>> Without this patch, there no way to revert to the old
> >>> behaviour with pcm_hw, so currently there will always
> >>> be some extra locking.
> >>>
> >>> This seems to be an oversight?
> >>
> >> thread_safe=1 means that the plugin needs no locking, so it means hw
> >> PCM never takes a lock by itself.  Only other plugins do, and it's
> >> suppressed when $LIBASOUND_THREAD_SAFE=0 is passed.
> >>
> >> The flag needs a better description or a better name...
> >
> > So here we go, let's decrypt the monkey code.
> > Let me know if this is good enough.
> 
> This is better, indeed. It also fixes the bug I sent a patch for
> (Respect LIBASOUND_THREAD_SAFE env var).

The behavior doesn't change.  The old code treated the flag as
tristate while now having two booleans instead.  That's the only
difference.

> It's perhaps a bit inconsistent that "true" is -1 for lock_enabled and
> 1 for need_lock, but that's just a nitpick.

No, -1 is set just to indicate the uninitialized state.  At the first
call, it's replaced with either 0 or 1.

> Since many calls to __snd_pcm_lock has the comment "forced lock"
> perhaps it can be renamed to snd_pcm_lock_forced, and snd_pcm_lock can
> perhaps be snd_pcm_lock_if_needed?

I don't think it matters much.  They are internal function calls,
after all.  And the __ variant shouldn't be called blindly, only with
care.


Takashi

> >
> >
> > Takashi
> >
> > -- 8< --
> > From: Takashi Iwai <tiwai@suse.de>
> > Subject: [PATCH] pcm: Better understandable locking code
> >
> > The newly added locking code seems to have confused quite a few
> > people, as "thread_safe=1" may be considered as if the thread-safety
> > lock has to be turned on.  (It meant that the plugin _is_ thread-safe,
> > i.e. it needs no extra locking.)
> >
> > For avoiding such a misunderstanding, this commit renames the relevant
> > pcm fields and give more comments to explain what is for what.
> > The former single pcm->thread_safe flag is now split to two boolean
> > flags, pcm->need_lock and pcm->lock_enabled.  It consumes a few more
> > bytes, but this would be (hopefully) better understandable.
> >
> > No functional change by this commit.
> >
> > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > ---
> >  src/pcm/pcm.c       | 16 +++++++++++-----
> >  src/pcm/pcm_hw.c    |  2 +-
> >  src/pcm/pcm_local.h | 27 ++++++++++++++++++++++-----
> >  3 files changed, 34 insertions(+), 11 deletions(-)
> >
> > diff --git a/src/pcm/pcm.c b/src/pcm/pcm.c
> > index f8323999343e..c96ae1c4641a 100644
> > --- a/src/pcm/pcm.c
> > +++ b/src/pcm/pcm.c
> > @@ -2545,14 +2545,20 @@ int snd_pcm_new(snd_pcm_t **pcmp, snd_pcm_type_t type, const char *name,
> >  	INIT_LIST_HEAD(&pcm->async_handlers);
> >  #ifdef THREAD_SAFE_API
> >  	pthread_mutex_init(&pcm->lock, NULL);
> > +	/* use locking as default;
> > +	 * each plugin may suppress this in its open call
> > +	 */
> > +	pcm->need_lock = 1;
> >  	{
> > -		static int default_thread_safe = -1;
> > -		if (default_thread_safe < 0) {
> > +		/* set lock_enabled field depending on $LIBASOUND_THREAD_SAFE;
> > +		 * it's evaluated only once at the first open for consistency
> > +		 */
> > +		static int do_lock_enable = -1;
> > +		if (do_lock_enable < 0) {
> >  			char *p = getenv("LIBASOUND_THREAD_SAFE");
> > -			default_thread_safe = !p || *p != '0';
> > +			do_lock_enable = !p || *p != '0';
> >  		}
> > -		if (!default_thread_safe)
> > -			pcm->thread_safe = -1; /* force to disable */
> > +		pcm->lock_enabled = do_lock_enable;
> >  	}
> >  #endif
> >  	*pcmp = pcm;
> > diff --git a/src/pcm/pcm_hw.c b/src/pcm/pcm_hw.c
> > index 3a5634c1d39a..56e88b6bf6c0 100644
> > --- a/src/pcm/pcm_hw.c
> > +++ b/src/pcm/pcm_hw.c
> > @@ -1514,7 +1514,7 @@ int snd_pcm_hw_open_fd(snd_pcm_t **pcmp, const char *name,
> >  	pcm->poll_events = info.stream == SND_PCM_STREAM_PLAYBACK ? POLLOUT : POLLIN;
> >  	pcm->tstamp_type = tstamp_type;
> >  #ifdef THREAD_SAFE_API
> > -	pcm->thread_safe = 1;
> > +	pcm->need_lock = 0;	/* hw plugin is thread-safe */
> >  #endif
> >
> >  	ret = snd_pcm_hw_mmap_status(pcm);
> > diff --git a/src/pcm/pcm_local.h b/src/pcm/pcm_local.h
> > index bb7964d7833e..bba2f15ac463 100644
> > --- a/src/pcm/pcm_local.h
> > +++ b/src/pcm/pcm_local.h
> > @@ -244,7 +244,12 @@ struct _snd_pcm {
> >  	void *private_data;
> >  	struct list_head async_handlers;
> >  #ifdef THREAD_SAFE_API
> > -	int thread_safe;
> > +	int need_lock;		/* true = this PCM (plugin) is thread-unsafe,
> > +				 * thus it needs a lock.
> > +				 */
> > +	int lock_enabled;	/* thread-safety lock is enabled on the system;
> > +				 * it's set depending on $LIBASOUND_THREAD_SAFE.
> > +				 */
> >  	pthread_mutex_t lock;
> >  #endif
> >  };
> > @@ -1085,24 +1090,36 @@ static inline void sw_set_period_event(snd_pcm_sw_params_t *params, int val)
> >  #define PCMINABORT(pcm) (((pcm)->mode & SND_PCM_ABORT) != 0)
> >
> >  #ifdef THREAD_SAFE_API
> > +/*
> > + * __snd_pcm_lock() and __snd_pcm_unlock() are used to lock/unlock the plugin
> > + * forcibly even if it's declared as thread-safe.  It's needed only for some
> > + * codes that are thread-unsafe per design (e.g. snd_pcm_nonblock()).
> > + *
> > + * OTOH, snd_pcm_lock() and snd_pcm_unlock() are used to lock/unlock the plugin
> > + * in normal situations.  They do lock/unlock only when the plugin is
> > + * thread-unsafe.
> > + *
> > + * Both __snd_pcm_lock() and snd_pcm_lock() (and their unlocks) wouldn't do
> > + * any action when the whole locking is disabled via $LIBASOUND_THREAD_SAFE=0.
> > + */
> >  static inline void __snd_pcm_lock(snd_pcm_t *pcm)
> >  {
> > -	if (pcm->thread_safe >= 0)
> > +	if (pcm->lock_enabled)
> >  		pthread_mutex_lock(&pcm->lock);
> >  }
> >  static inline void __snd_pcm_unlock(snd_pcm_t *pcm)
> >  {
> > -	if (pcm->thread_safe >= 0)
> > +	if (pcm->lock_enabled)
> >  		pthread_mutex_unlock(&pcm->lock);
> >  }
> >  static inline void snd_pcm_lock(snd_pcm_t *pcm)
> >  {
> > -	if (!pcm->thread_safe)
> > +	if (pcm->lock_enabled && pcm->need_lock)
> >  		pthread_mutex_lock(&pcm->lock);
> >  }
> >  static inline void snd_pcm_unlock(snd_pcm_t *pcm)
> >  {
> > -	if (!pcm->thread_safe)
> > +	if (pcm->lock_enabled && pcm->need_lock)
> >  		pthread_mutex_unlock(&pcm->lock);
> >  }
> >  #else /* THREAD_SAFE_API */
> >
> 

Re: [PATCH] pcm_hw: Respect LIBASOUND_THREAD_SAFE env var

[Takashi Iwai]

On Fri, 02 Sep 2016 08:55:39 +0200,
Takashi Iwai wrote:
> 
> On Thu, 01 Sep 2016 21:40:50 +0200,
> David Henningsson wrote:
> > 
> > On 2016-09-01 14:28, Takashi Iwai wrote:
> > > On Thu, 01 Sep 2016 08:51:14 +0200,
> > > Takashi Iwai wrote:
> > >>
> > >> On Thu, 01 Sep 2016 08:06:50 +0200,
> > >> David Henningsson wrote:
> > >>>
> > >>> Without this patch, there no way to revert to the old
> > >>> behaviour with pcm_hw, so currently there will always
> > >>> be some extra locking.
> > >>>
> > >>> This seems to be an oversight?
> > >>
> > >> thread_safe=1 means that the plugin needs no locking, so it means hw
> > >> PCM never takes a lock by itself.  Only other plugins do, and it's
> > >> suppressed when $LIBASOUND_THREAD_SAFE=0 is passed.
> > >>
> > >> The flag needs a better description or a better name...
> > >
> > > So here we go, let's decrypt the monkey code.
> > > Let me know if this is good enough.
> > 
> > This is better, indeed. It also fixes the bug I sent a patch for
> > (Respect LIBASOUND_THREAD_SAFE env var).
> 
> The behavior doesn't change.  The old code treated the flag as
> tristate while now having two booleans instead.  That's the only
> difference.
> 
> > It's perhaps a bit inconsistent that "true" is -1 for lock_enabled and
> > 1 for need_lock, but that's just a nitpick.
> 
> No, -1 is set just to indicate the uninitialized state.  At the first
> call, it's replaced with either 0 or 1.

Hopefully the revised one below is clearer wrt it.


Takashi

-- 8< --
From: Takashi Iwai <tiwai@suse.de>
Subject: [PATCH v2] pcm: Better understandable locking code

The newly added locking code seems to have confused quite a few
people, as "thread_safe=1" may be considered as if the thread-safety
lock has to be turned on.  (It meant that the plugin _is_ thread-safe,
i.e. it needs no extra locking.)

For avoiding such a misunderstanding, this commit renames the relevant
pcm fields and give more comments to explain what is for what.
The former single pcm->thread_safe flag is now split to two boolean
flags, pcm->need_lock and pcm->lock_enabled.  It consumes a few more
bytes, but this would be (hopefully) better understandable.

No functional change by this commit.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 src/pcm/pcm.c       | 16 +++++++++++-----
 src/pcm/pcm_hw.c    |  2 +-
 src/pcm/pcm_local.h | 27 ++++++++++++++++++++++-----
 3 files changed, 34 insertions(+), 11 deletions(-)

diff --git a/src/pcm/pcm.c b/src/pcm/pcm.c
index f8323999343e..cd87bc759ded 100644
--- a/src/pcm/pcm.c
+++ b/src/pcm/pcm.c
@@ -2545,14 +2545,20 @@ int snd_pcm_new(snd_pcm_t **pcmp, snd_pcm_type_t type, const char *name,
 	INIT_LIST_HEAD(&pcm->async_handlers);
 #ifdef THREAD_SAFE_API
 	pthread_mutex_init(&pcm->lock, NULL);
+	/* use locking as default;
+	 * each plugin may suppress this in its open call
+	 */
+	pcm->need_lock = 1;
 	{
-		static int default_thread_safe = -1;
-		if (default_thread_safe < 0) {
+		/* set lock_enabled field depending on $LIBASOUND_THREAD_SAFE */
+		static int do_lock_enable = -1; /* uninitialized */
+
+		/* evaluate env var only once at the first open for consistency */
+		if (do_lock_enable == -1) {
 			char *p = getenv("LIBASOUND_THREAD_SAFE");
-			default_thread_safe = !p || *p != '0';
+			do_lock_enable = !p || *p != '0';
 		}
-		if (!default_thread_safe)
-			pcm->thread_safe = -1; /* force to disable */
+		pcm->lock_enabled = do_lock_enable;
 	}
 #endif
 	*pcmp = pcm;
diff --git a/src/pcm/pcm_hw.c b/src/pcm/pcm_hw.c
index 3a5634c1d39a..56e88b6bf6c0 100644
--- a/src/pcm/pcm_hw.c
+++ b/src/pcm/pcm_hw.c
@@ -1514,7 +1514,7 @@ int snd_pcm_hw_open_fd(snd_pcm_t **pcmp, const char *name,
 	pcm->poll_events = info.stream == SND_PCM_STREAM_PLAYBACK ? POLLOUT : POLLIN;
 	pcm->tstamp_type = tstamp_type;
 #ifdef THREAD_SAFE_API
-	pcm->thread_safe = 1;
+	pcm->need_lock = 0;	/* hw plugin is thread-safe */
 #endif
 
 	ret = snd_pcm_hw_mmap_status(pcm);
diff --git a/src/pcm/pcm_local.h b/src/pcm/pcm_local.h
index bb7964d7833e..bba2f15ac463 100644
--- a/src/pcm/pcm_local.h
+++ b/src/pcm/pcm_local.h
@@ -244,7 +244,12 @@ struct _snd_pcm {
 	void *private_data;
 	struct list_head async_handlers;
 #ifdef THREAD_SAFE_API
-	int thread_safe;
+	int need_lock;		/* true = this PCM (plugin) is thread-unsafe,
+				 * thus it needs a lock.
+				 */
+	int lock_enabled;	/* thread-safety lock is enabled on the system;
+				 * it's set depending on $LIBASOUND_THREAD_SAFE.
+				 */
 	pthread_mutex_t lock;
 #endif
 };
@@ -1085,24 +1090,36 @@ static inline void sw_set_period_event(snd_pcm_sw_params_t *params, int val)
 #define PCMINABORT(pcm) (((pcm)->mode & SND_PCM_ABORT) != 0)
 
 #ifdef THREAD_SAFE_API
+/*
+ * __snd_pcm_lock() and __snd_pcm_unlock() are used to lock/unlock the plugin
+ * forcibly even if it's declared as thread-safe.  It's needed only for some
+ * codes that are thread-unsafe per design (e.g. snd_pcm_nonblock()).
+ *
+ * OTOH, snd_pcm_lock() and snd_pcm_unlock() are used to lock/unlock the plugin
+ * in normal situations.  They do lock/unlock only when the plugin is
+ * thread-unsafe.
+ *
+ * Both __snd_pcm_lock() and snd_pcm_lock() (and their unlocks) wouldn't do
+ * any action when the whole locking is disabled via $LIBASOUND_THREAD_SAFE=0.
+ */
 static inline void __snd_pcm_lock(snd_pcm_t *pcm)
 {
-	if (pcm->thread_safe >= 0)
+	if (pcm->lock_enabled)
 		pthread_mutex_lock(&pcm->lock);
 }
 static inline void __snd_pcm_unlock(snd_pcm_t *pcm)
 {
-	if (pcm->thread_safe >= 0)
+	if (pcm->lock_enabled)
 		pthread_mutex_unlock(&pcm->lock);
 }
 static inline void snd_pcm_lock(snd_pcm_t *pcm)
 {
-	if (!pcm->thread_safe)
+	if (pcm->lock_enabled && pcm->need_lock)
 		pthread_mutex_lock(&pcm->lock);
 }
 static inline void snd_pcm_unlock(snd_pcm_t *pcm)
 {
-	if (!pcm->thread_safe)
+	if (pcm->lock_enabled && pcm->need_lock)
 		pthread_mutex_unlock(&pcm->lock);
 }
 #else /* THREAD_SAFE_API */
-- 
2.9.3

Re: [PATCH] pcm_hw: Respect LIBASOUND_THREAD_SAFE env var

[Samuel Thibault]

Hello,

Takashi Iwai, on Fri 02 Sep 2016 10:19:16 +0200, wrote:
> Subject: [PATCH v2] pcm: Better understandable locking code

Way clearer, thanks!

Samuel