From mboxrd@z Thu Jan 1 00:00:00 1970 From: Takashi Iwai Subject: Re: snd_pcsp locking mess Date: Wed, 28 May 2008 12:13:44 +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> 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 A5F11245CB for ; Wed, 28 May 2008 12:13:45 +0200 (CEST) In-Reply-To: <483C477D.2000207@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 Tue, 27 May 2008 21:40:13 +0400, Stas Sergeev wrote: > > Hello. > > Takashi Iwai wrote: > > Well, let's take a look back how the callbacks are called. > > The callbacks that are called during PCM running are the following: > > > > - hrtimer callback gets called from hrtimer: > > This updates the pcsp port and updates the PCM pointer. > > > > - PCM pointer callback: > > This returns the current PCM pointer > > > > So, actually between these calls, we have only one thing to protect - > > the PCM pointer. The DMA-buffer readiness is necessary only for > > hrtimer callback. > OK. > > > And, in addition, we need to call somewhere snd_pcm_period_elapsed() > > when the period is finished. This can be done in a tasklet. > What is the difference between calling > everything in softirq (current solution) > and calling snd_pcm_period_elapsed() in > a tasklet, with the rest in a hardirq > context? What will this solve? Think just like the old-good bottomhalf. The register flip with hrtimer callback is definitely a fast path, so it should be called inside the hard irq. 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. Thus it's better to put this into a softirq. 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. > > Now, remember that hrtimer is started only via trigger start > > callback. That is, hrtimer callback isn called first after PCM is > > actually started; hence the DMA buffer is ready when it's called. > > > > Then, if we assure that the DMA buffer isn't changed and the PCM > > substream still exists during hrtimer callback (and/or its execution > > finishes), hrtimer callback can safely run without *any* lock. > OK so how do you protect the PCM pointer > after all? Even in your example you use > the chip->substream_lock for that (why?), Because the pointer callback and the hrtimer callback can race about chip->playback_ptr. The lock is simply to protect it. Don't take it as a PCM substream lock. It can be implemened even without a lock, e.g. using a simple barrier, though. I was just too lazy. > so it doesn't look like you run a callback > without any lock at all. You don't need a lock for the PCM callback itself. The pointer callback is called inside the PCM substream lock. > > This is pretty easy. We just need to add appropriate calls to > > synchronize the finish of hrtimer (and tasklet) at each place that can > > change the DMA buffers. > I wonder if this is SMP-safe (comments > below). > > > +/* > > + * Force to stop and sync the stream > > + */ > > +void pcsp_sync_stop(struct snd_pcsp *chip) > > +{ > > + local_irq_disable(); > So what will happen if the timer callback > is executing on another CPU at that point? That doesn't matter, because this irqsave is needed just for i8253_lock in pcsp_stop_playing() (as it doesn't save irq). > > + 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. > > + local_irq_enable(); > > + hrtimer_cancel(&chip->timer); > > + tasklet_kill(&pcsp_pcm_tasklet); > Ouch, IIRC all three are discouraged to > use... Really? Any pointer? > I (hopefully) now understand the logic > you are trying to explain, and I don't > think it is completely trouble-free (I > may be wrong of course, but certainly > it won't be straightforward for every > reader why is that code correct). > So the question is still why not to > simply use snd_pcm_stream_lock(). Because the substream lock isn't necessary! It just makes things more complicated -- if we take substream lock, AB/BA deadlocks occur again. > 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. Takashi