From mboxrd@z Thu Jan 1 00:00:00 1970 From: Takashi Iwai Subject: Re: Clean up of PCM spinlocks Date: Tue, 13 Mar 2012 17:05:03 +0100 Message-ID: References: <4F5F588B.40107@perex.cz> <4F5F64D4.5080206@perex.cz> <4F5F6BDF.5030601@perex.cz> Mime-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mx2.suse.de (cantor2.suse.de [195.135.220.15]) by alsa0.perex.cz (Postfix) with ESMTP id 9442D104170 for ; Tue, 13 Mar 2012 17:05:05 +0100 (CET) In-Reply-To: <4F5F6BDF.5030601@perex.cz> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: alsa-devel-bounces@alsa-project.org Errors-To: alsa-devel-bounces@alsa-project.org To: Jaroslav Kysela Cc: alsa-devel@alsa-project.org List-Id: alsa-devel@alsa-project.org At Tue, 13 Mar 2012 16:46:39 +0100, Jaroslav Kysela wrote: > > Date 13.3.2012 16:23, Takashi Iwai wrote: > > 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 > >>>>> 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. > > I see. The problem is that the concurrent but linked streams can take > different first locks for the group locking: > > time 0 time 1 > -------------------------------- > lock stream 0 -> group lock > lock stream 1 -> group lock > > In this case the condition current != s is not enough. I think that we > have to strictly separate situations when the group lock can be > activated and use different locking scheme there - for example using a > master lock per linked substreams. In my revised patch, group->lock is actually the master lock of the group. This is the only lock applied to the substream. When a stream is linked, it's done in the global write_lock to guarantee that stream is unlocked. Then group->lock is replaced with a real group lock from self_group.lock. So, there is no race among this move. > It seems to me that your proposed patch just removes the spinlocks for > linked substreams which can cause inconsistencies for these streams. Not really. It just uses a single lock for linked streams. There are no longer stream locks once when the stream is linked. This is of course a drawback -- it disallows the concurrent access to linked stream members in some operations. How this is regarded as a serious drawback, is a question of operation styles. If any apps are accessing many streams as a linked stream in realtime manner, and access to each without mmap, then the performance may be worsen. But, this scenario is rather pretty rare, I guess. > Or just a quick idea - what about to track the the group action > collisions? We can use a spinlock protected counter and serialize these > requests in another way (if counter is above one - the CPU should wait > until it becomes one). Well... this sounds way too complex, I'm afraid. thanks, Takashi