[alsa-devel] Clean up of PCM spinlocks
Takashi Iwai
tiwai at suse.de
Tue Mar 13 11:57:50 CET 2012
Hi,
below is a patch I posted to LKML as the follow up for the lockdep
report from Dave Jones. It's a resurrected patch from what I worked
on quite ago. Although this fix is suboptimal, I see no better way.
If this looks OK, I'm going to queue it as a 3.4 material.
thanks,
Takashi
===
From: Takashi Iwai <tiwai at suse.de>
Subject: [PATCH] ALSA: pcm - Simplify linked PCM substream spinlocks
The spinlocks held for PCM substreams have been always messy. For the
better concurrent accessibility, we took always the substream's lock
first. When substreams are linked, we need a group-wide lock, but
this should be applied outside substream's lock. So, we unlock the
substream's lock first, then do locks twice.
This scheme is known to be problematic with lockdep. Maybe because
the nested lock isn't marked properly, and partly because the lock and
unlock sequences are different (A/B -> A/B).
This patch tries to simplify the scheme. Instead of holding the
substream's lock, we take the group lock always at first. A drawback
of this is that the access to the individual substream in a same group
won't be allowed any longer. But, the code looks much easier, and
likely less buggy.
Note that the group lock is identical with the substream's lock
initially before the substream is linked with others. So, in the case
of non-linked PCM, the new code behaves exactly same as before.
Signed-off-by: Takashi Iwai <tiwai at suse.de>
---
include/sound/pcm.h | 12 ++++++------
sound/core/pcm_native.c | 36 ++++++++++++++----------------------
2 files changed, 20 insertions(+), 28 deletions(-)
diff --git a/include/sound/pcm.h b/include/sound/pcm.h
index 0cf91b2..d634dcc 100644
--- a/include/sound/pcm.h
+++ b/include/sound/pcm.h
@@ -531,36 +531,36 @@ static inline int snd_pcm_stream_linked(struct snd_pcm_substream *substream)
static inline void snd_pcm_stream_lock(struct snd_pcm_substream *substream)
{
read_lock(&snd_pcm_link_rwlock);
- spin_lock(&substream->self_group.lock);
+ spin_lock(&substream->group->lock);
}
static inline void snd_pcm_stream_unlock(struct snd_pcm_substream *substream)
{
- spin_unlock(&substream->self_group.lock);
+ spin_unlock(&substream->group->lock);
read_unlock(&snd_pcm_link_rwlock);
}
static inline void snd_pcm_stream_lock_irq(struct snd_pcm_substream *substream)
{
read_lock_irq(&snd_pcm_link_rwlock);
- spin_lock(&substream->self_group.lock);
+ spin_lock(&substream->group->lock);
}
static inline void snd_pcm_stream_unlock_irq(struct snd_pcm_substream *substream)
{
- spin_unlock(&substream->self_group.lock);
+ spin_unlock(&substream->group->lock);
read_unlock_irq(&snd_pcm_link_rwlock);
}
#define snd_pcm_stream_lock_irqsave(substream, flags) \
do { \
read_lock_irqsave(&snd_pcm_link_rwlock, (flags)); \
- spin_lock(&substream->self_group.lock); \
+ spin_lock(&substream->group->lock); \
} while (0)
#define snd_pcm_stream_unlock_irqrestore(substream, flags) \
do { \
- spin_unlock(&substream->self_group.lock); \
+ spin_unlock(&substream->group->lock); \
read_unlock_irqrestore(&snd_pcm_link_rwlock, (flags)); \
} while (0)
diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
index 25ed9fe..c5f28ed 100644
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -712,7 +712,7 @@ static int snd_pcm_action_group(struct action_ops *ops,
int res = 0;
snd_pcm_group_for_each_entry(s, substream) {
- if (do_lock && s != substream)
+ if (do_lock)
spin_lock_nested(&s->self_group.lock,
SINGLE_DEPTH_NESTING);
res = ops->pre_action(s, state);
@@ -740,8 +740,7 @@ static int snd_pcm_action_group(struct action_ops *ops,
if (do_lock) {
/* unlock streams */
snd_pcm_group_for_each_entry(s1, substream) {
- if (s1 != substream)
- spin_unlock(&s1->self_group.lock);
+ spin_unlock(&s1->self_group.lock);
if (s1 == s) /* end */
break;
}
@@ -779,13 +778,7 @@ static int snd_pcm_action(struct action_ops *ops,
int res;
if (snd_pcm_stream_linked(substream)) {
- if (!spin_trylock(&substream->group->lock)) {
- spin_unlock(&substream->self_group.lock);
- spin_lock(&substream->group->lock);
- spin_lock(&substream->self_group.lock);
- }
res = snd_pcm_action_group(ops, substream, state, 1);
- spin_unlock(&substream->group->lock);
} else {
res = snd_pcm_action_single(ops, substream, state);
}
@@ -801,19 +794,13 @@ static int snd_pcm_action_lock_irq(struct action_ops *ops,
{
int res;
- read_lock_irq(&snd_pcm_link_rwlock);
+ snd_pcm_stream_lock_irq(substream);
if (snd_pcm_stream_linked(substream)) {
- spin_lock(&substream->group->lock);
- spin_lock(&substream->self_group.lock);
res = snd_pcm_action_group(ops, substream, state, 1);
- spin_unlock(&substream->self_group.lock);
- spin_unlock(&substream->group->lock);
} else {
- spin_lock(&substream->self_group.lock);
res = snd_pcm_action_single(ops, substream, state);
- spin_unlock(&substream->self_group.lock);
}
- read_unlock_irq(&snd_pcm_link_rwlock);
+ snd_pcm_stream_unlock_irq(substream);
return res;
}
@@ -1586,12 +1573,18 @@ static int snd_pcm_link(struct snd_pcm_substream *substream, int fd)
struct file *file;
struct snd_pcm_file *pcm_file;
struct snd_pcm_substream *substream1;
+ struct snd_pcm_group *group;
file = snd_pcm_file_fd(fd);
if (!file)
return -EBADFD;
pcm_file = file->private_data;
substream1 = pcm_file->substream;
+ group = kmalloc(sizeof(*group), GFP_KERNEL);
+ if (!group) {
+ res = -ENOMEM;
+ goto _nolock;
+ }
down_write(&snd_pcm_link_rwsem);
write_lock_irq(&snd_pcm_link_rwlock);
if (substream->runtime->status->state == SNDRV_PCM_STATE_OPEN ||
@@ -1604,11 +1597,7 @@ static int snd_pcm_link(struct snd_pcm_substream *substream, int fd)
goto _end;
}
if (!snd_pcm_stream_linked(substream)) {
- substream->group = kmalloc(sizeof(struct snd_pcm_group), GFP_ATOMIC);
- if (substream->group == NULL) {
- res = -ENOMEM;
- goto _end;
- }
+ substream->group = group;
spin_lock_init(&substream->group->lock);
INIT_LIST_HEAD(&substream->group->substreams);
list_add_tail(&substream->link_list, &substream->group->substreams);
@@ -1620,7 +1609,10 @@ static int snd_pcm_link(struct snd_pcm_substream *substream, int fd)
_end:
write_unlock_irq(&snd_pcm_link_rwlock);
up_write(&snd_pcm_link_rwsem);
+ _nolock:
fput(file);
+ if (res < 0)
+ kfree(group);
return res;
}
--
1.7.9.2
More information about the Alsa-devel
mailing list