[alsa-devel] Clean up of PCM spinlocks
Jaroslav Kysela
perex at perex.cz
Tue Mar 13 16:16:36 CET 2012
Date 13.3.2012 16:00, Takashi Iwai wrote:
> 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.
I meant use group->lock in the group_action, see patch bellow.
> Also, the changes in snd_pcm_link() to use GFP_KERNEL is splitted out
> of this patch.
Maybe, we can reuse the self_group container for the linked streams, too?
In this case, the dynamic allocation of new group structure can be removed.
Jaroslav
diff --git a/include/sound/pcm.h b/include/sound/pcm.h
index 1d58d79..301ed22 100644
--- a/include/sound/pcm.h
+++ b/include/sound/pcm.h
@@ -535,36 +535,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..5096c83 100644
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -713,7 +713,7 @@ static int snd_pcm_action_group(struct action_ops *ops,
snd_pcm_group_for_each_entry(s, substream) {
if (do_lock && s != substream)
- spin_lock_nested(&s->self_group.lock,
+ spin_lock_nested(&s->group->lock,
SINGLE_DEPTH_NESTING);
res = ops->pre_action(s, state);
if (res < 0)
@@ -741,7 +741,7 @@ static int snd_pcm_action_group(struct action_ops *ops,
/* unlock streams */
snd_pcm_group_for_each_entry(s1, substream) {
if (s1 != substream)
- spin_unlock(&s1->self_group.lock);
+ spin_unlock(&s1->group->lock);
if (s1 == s) /* end */
break;
}
@@ -779,13 +779,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 +795,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;
}
--
Jaroslav Kysela <perex at perex.cz>
Linux Kernel Sound Maintainer
ALSA Project; Red Hat, Inc.
More information about the Alsa-devel
mailing list