* [PATCH] pcm_hw: Respect LIBASOUND_THREAD_SAFE env var @ 2016-09-01 6:06 David Henningsson 2016-09-01 6:51 ` Takashi Iwai 0 siblings, 1 reply; 7+ messages in thread From: David Henningsson @ 2016-09-01 6:06 UTC (permalink / raw) To: perex, tiwai, alsa-devel; +Cc: samuel.thibault, 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] pcm_hw: Respect LIBASOUND_THREAD_SAFE env var 2016-09-01 6:06 [PATCH] pcm_hw: Respect LIBASOUND_THREAD_SAFE env var David Henningsson @ 2016-09-01 6:51 ` Takashi Iwai 2016-09-01 12:28 ` Takashi Iwai 0 siblings, 1 reply; 7+ messages in thread From: Takashi Iwai @ 2016-09-01 6:51 UTC (permalink / raw) To: David Henningsson; +Cc: samuel.thibault, alsa-devel 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 > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] pcm_hw: Respect LIBASOUND_THREAD_SAFE env var 2016-09-01 6:51 ` Takashi Iwai @ 2016-09-01 12:28 ` Takashi Iwai 2016-09-01 19:40 ` David Henningsson 0 siblings, 1 reply; 7+ messages in thread From: Takashi Iwai @ 2016-09-01 12:28 UTC (permalink / raw) To: David Henningsson; +Cc: samuel.thibault, alsa-devel 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] pcm_hw: Respect LIBASOUND_THREAD_SAFE env var 2016-09-01 12:28 ` Takashi Iwai @ 2016-09-01 19:40 ` David Henningsson 2016-09-02 6:55 ` Takashi Iwai 0 siblings, 1 reply; 7+ messages in thread From: David Henningsson @ 2016-09-01 19:40 UTC (permalink / raw) To: Takashi Iwai; +Cc: samuel.thibault, alsa-devel 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 */ > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] pcm_hw: Respect LIBASOUND_THREAD_SAFE env var 2016-09-01 19:40 ` David Henningsson @ 2016-09-02 6:55 ` Takashi Iwai 2016-09-02 8:19 ` Takashi Iwai 0 siblings, 1 reply; 7+ messages in thread From: Takashi Iwai @ 2016-09-02 6:55 UTC (permalink / raw) To: David Henningsson; +Cc: samuel.thibault, alsa-devel 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 */ > > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] pcm_hw: Respect LIBASOUND_THREAD_SAFE env var 2016-09-02 6:55 ` Takashi Iwai @ 2016-09-02 8:19 ` Takashi Iwai 2016-09-02 8:21 ` Samuel Thibault 0 siblings, 1 reply; 7+ messages in thread From: Takashi Iwai @ 2016-09-02 8:19 UTC (permalink / raw) To: David Henningsson; +Cc: samuel.thibault, alsa-devel 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] pcm_hw: Respect LIBASOUND_THREAD_SAFE env var 2016-09-02 8:19 ` Takashi Iwai @ 2016-09-02 8:21 ` Samuel Thibault 0 siblings, 0 replies; 7+ messages in thread From: Samuel Thibault @ 2016-09-02 8:21 UTC (permalink / raw) To: Takashi Iwai; +Cc: alsa-devel, David Henningsson 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2016-09-02 8:21 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-09-01 6:06 [PATCH] pcm_hw: Respect LIBASOUND_THREAD_SAFE env var David Henningsson 2016-09-01 6:51 ` Takashi Iwai 2016-09-01 12:28 ` Takashi Iwai 2016-09-01 19:40 ` David Henningsson 2016-09-02 6:55 ` Takashi Iwai 2016-09-02 8:19 ` Takashi Iwai 2016-09-02 8:21 ` Samuel Thibault
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).