[alsa-devel] Clean up of PCM spinlocks

[Takashi Iwai]

At Tue, 13 Mar 2012 16:16:36 +0100,
Jaroslav Kysela wrote:
> 
> 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.

Hmm, I don't think this works.  From linked stream members,
group->lock points to the very same lock instance.  So,
snd_pcm_action_group() would dead-lock the same lock with your code.

> > 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.

A substream can be unlinked at any time, thus it can't belong to
the substream instance.  The point of substream->group is to detach
the instance from the substream.  self_group is just an optimization
for unlinked streams.

The GFP_KERNEL thing is a thing of optimization, too: whether to call
kmalloc() superfluously in the case of errors or not.  As seen in the
patch below, it's no big matter...


thanks,

Takashi

---
From: Takashi Iwai <tiwai at suse.de>
Subject: [PATCH] ALSA: pcm - Avoid GFP_ATOMIC in snd_pcm_link()

GFP_ATOMIC is used in snd_pcm_link() just because the kmalloc is
called inside a lock.  Since this function isn't too critical for
speed and is rarely called in practice, better to allocate the chunk
at first before spinlock and free it in error paths, so that
GFP_KERNEL can be used.

Signed-off-by: Takashi Iwai <tiwai at suse.de>
---
 sound/core/pcm_native.c |   15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
index aff5187..afa4b83 100644
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -1553,12 +1553,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 ||
@@ -1571,11 +1577,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);
@@ -1587,7 +1589,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