From mboxrd@z Thu Jan 1 00:00:00 1970 From: Takashi Iwai Subject: Re: snd_pcsp locking mess Date: Thu, 29 May 2008 08:03:26 +0200 Message-ID: References: <20080518172258.D0DFB108060@picon.linux-foundation.org> <4830737B.70108@aknet.ru> <4831B260.1060802@aknet.ru> <4835D774.3070306@aknet.ru> <483C10E7.7070203@aknet.ru> <483C477D.2000207@aknet.ru> <483DBBC0.1080001@aknet.ru> 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 (mx2.suse.de [195.135.220.15]) by alsa0.perex.cz (Postfix) with ESMTP id D2E4E247FA for ; Thu, 29 May 2008 08:03:27 +0200 (CEST) In-Reply-To: <483DBBC0.1080001@aknet.ru> 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: Stas Sergeev Cc: alsa-devel@alsa-project.org List-Id: alsa-devel@alsa-project.org At Thu, 29 May 2008 00:08:32 +0400, Stas Sergeev wrote: > > Hello. > > Takashi Iwai wrote: > > OTOH, the update of the whole PCM via snd_pcm_period_elapsed() is a > > slow path. Especially in the case of hrtimer, it brings the lock > > mess, too. > Unless the entire callback is done in > a softirq, as it currently is, no? > > > Issuing softirq at each hrtimer callback isn't optimal from > > performance perspective. For RT-kernels, it doesn't matter, but for > > normal kernels, it does. > Oh. But in that case I guess it would > really be better if just snd_pcm_period_elapsed() > would raise a softirq. That's something > you proposed in the past IIRC. > Is this acceptable to have that change > not in the driver, or do you think it > is snd-pcsp-specific, and no other driver > will benefit? It would be good in general. However, better to be the next step after changing pcsp. > Btw, could you please point me to where > to read about the softirq expense? I > thought they are very cheap, almost zero > overhead. One big disadvantage of the softirq is its (possible) timing inaccuracy. (Note that this is about the normal non-preempt kernel.) The thing like pcsp driver does, flipping the bits at a high rate, isn't for softirq per se. > And yes, if the sotfirqs are expensive, > then indeed I see why you propose all > these tricks you do... But if they are > not and we can keep the entire callback > in a softirq as it currently is, then > there might be a simpler solutions. > > >> the chip->substream_lock for that (why?), > > Because the pointer callback and the hrtimer callback can race about > I asked only why have you chosen > chip->substream_lock - IIRC it was > intended not for this. > > >>> + pcsp_stop_playing(chip); > >> This will set chip->timer_active to 0, > >> but who knows if the callback on another > >> CPU have already passed the check or not? > > If it passed, then we simply wait until the next hrtimer callback > > finished. That's what hrtimer_cancel() does. So, after that, it is > > guaranteed that neither hrtimer and tasklet remains. > Yes, but the state doesn't seem to be > guaranteed. Running the handler after > the pcsp_stop_playing(chip) called, looks > nasty. This is there because pcsp_stop_playing() doesn't sync. Both hrtimer_cancel() and tasklet_kill() are basically just for sync, not for killing the object. > The timer will be in a different > mode. Nothing bad will likely to happen, > but... esp with the nforce workaround, > where the callback is supposed to be > called even amount of times. Doesn't > look very good to me, even if nothing > bad can happen... Well, it can be done in another away if you are so nervous about it. Simply add chip->timer_running atomic_t and reset it when the callback finished with HRTIMER_NORESTART. In another side, wait until chip->timer_running becomes zero. But, it's just an uneeded addition of the code. > > Because the substream lock isn't necessary! It just makes things more > > complicated -- if we take substream lock, AB/BA deadlocks occur again. > Even if the callback is called within > the softirq? The runtime callbacks, pointer and trigger, are called already inside they substream lock held by the PCM core. Thus it doesn't matter whether it's in a hard or a softirq. > >> And > >> if right now it is barely usefull, then > >> why it can't be improved to simply not > >> require a second lock that guards a > >> substream itself? > > Because it won't be simpler, as you already tried. > I haven't tried that. :) > I have tried the snd_pcm_stream_lock(), > and yes, its nasty. But to me its nastiness > is only because it needs another lock to > protect the substream itself. No, you don't need to protect the substream at all. Remember that hrtimer callback requires only the DMA-buffer pointer during its operation. No other thing is needed from the substream. This can be guaranteed with a simpler logic. > So it is > barely usefull. But it can be improved > (or an alternative provided), the way the > second lock is not needed. Then I will > need only that - a single lock, nothing > more. This might not be even difficult to > do at all. Well, my version is almost i8259_lock only. As mentioned, chip->substream_lock can be removed safely with a barrier. Takashi