[alsa-devel] Clean up of PCM spinlocks
Takashi Iwai
tiwai at suse.de
Tue Mar 13 16:00:55 CET 2012
At Tue, 13 Mar 2012 15:24:11 +0100,
Jaroslav Kysela wrote:
>
> Date 13.3.2012 11:57, Takashi Iwai wrote:
> > 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.
>
> What do you mean by this? I noticed that you switched the behaviour for
> the stream_lock():
>
> 1) self_group spinlock is used for single (non-linked) stream
> - same as before
> 2) group.lock spinlock is used for the linked streams
> - the self_group spinlock is used only in the group action
> - the question is why? it seems useless, the
> stream_lock() functions use the allocated group.lock
Oh yeah, right, this is utterly useless!
I just thought of the mutex case, but in this case, the lock is
avoided anyway.
> I don't think that this patch will work in the way we expect.
>
> I believe we should use only group.lock spinlock for the linked streams
> without using the self_group spinlock to make locking work like this:
>
> stream_lock(current)
> group_action(current) {
> for s from all_group_substreams {
> if (s != current)
> stream_lock(s);
> }
> .... job ....
> .... group substreams unlock ....
> }
> stream_unlock(current)
Well, if we think of a single lock, the code would be much
simplified than above.
The reviewsed patch is below. I left the single-stream version as is
since it's used explicitly in the drain() -- the drain action is
applied to each substream although it's linked.
Also, the changes in snd_pcm_link() to use GFP_KERNEL is splitted out
of this patch.
thanks,
Takashi
---
From 4ad4cd8120958fa9ec037f21da6001ec7768577d Mon Sep 17 00:00:00 2001
From: Takashi Iwai <tiwai at suse.de>
Date: Mon, 12 Mar 2012 18:30:13 +0100
Subject: [PATCH v2] 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
nested spinlocks, we hold a single lock, substream->group->lock. It
points to the self_group.lock when the stream is unlinked. When the
stream is linked, it points to its group's common lock.
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.
Signed-off-by: Takashi Iwai <tiwai at suse.de>
---
include/sound/pcm.h | 12 +++++------
sound/core/pcm_native.c | 51 +++++++++--------------------------------------
2 files changed, 15 insertions(+), 48 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..aff5187 100644
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -701,23 +701,20 @@ struct action_ops {
/*
* this functions is core for handling of linked stream
* Note: the stream state might be changed also on failure
- * Note2: call with calling stream lock + link lock
+ * Note2: call with calling stream lock
*/
static int snd_pcm_action_group(struct action_ops *ops,
struct snd_pcm_substream *substream,
- int state, int do_lock)
+ int state)
{
struct snd_pcm_substream *s = NULL;
struct snd_pcm_substream *s1;
int res = 0;
snd_pcm_group_for_each_entry(s, substream) {
- if (do_lock && s != substream)
- spin_lock_nested(&s->self_group.lock,
- SINGLE_DEPTH_NESTING);
res = ops->pre_action(s, state);
if (res < 0)
- goto _unlock;
+ return res;
}
snd_pcm_group_for_each_entry(s, substream) {
res = ops->do_action(s, state);
@@ -729,23 +726,12 @@ static int snd_pcm_action_group(struct action_ops *ops,
ops->undo_action(s1, state);
}
}
- s = NULL; /* unlock all */
- goto _unlock;
+ return res;
}
}
snd_pcm_group_for_each_entry(s, substream) {
ops->post_action(s, state);
}
- _unlock:
- if (do_lock) {
- /* unlock streams */
- snd_pcm_group_for_each_entry(s1, substream) {
- if (s1 != substream)
- spin_unlock(&s1->self_group.lock);
- if (s1 == s) /* end */
- break;
- }
- }
return res;
}
@@ -779,13 +765,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);
+ res = snd_pcm_action_group(ops, substream, state);
} else {
res = snd_pcm_action_single(ops, substream, state);
}
@@ -801,19 +781,9 @@ static int snd_pcm_action_lock_irq(struct action_ops *ops,
{
int res;
- read_lock_irq(&snd_pcm_link_rwlock);
- 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_lock_irq(substream);
+ res = snd_pcm_action(ops, substream, state);
+ snd_pcm_stream_unlock_irq(substream);
return res;
}
@@ -826,10 +796,7 @@ static int snd_pcm_action_nonatomic(struct action_ops *ops,
int res;
down_read(&snd_pcm_link_rwsem);
- if (snd_pcm_stream_linked(substream))
- res = snd_pcm_action_group(ops, substream, state, 0);
- else
- res = snd_pcm_action_single(ops, substream, state);
+ res = snd_pcm_action(ops, substream, state);
up_read(&snd_pcm_link_rwsem);
return res;
}
--
1.7.9.2
More information about the Alsa-devel
mailing list