[alsa-devel] snd_pcsp locking mess

[Takashi Iwai]

At Mon, 19 May 2008 21:01:20 +0400,
Stas Sergeev wrote:
> 
> Hello.
> 
> Takashi Iwai wrote:
> >>> Anyway, what I meant is that the part touching I/O ports could be a faster path
> >>> than softirq.  And the hrtimer code can use a bit simpler locks.  The hrtimer
> >>> callback needs just one thing about DMA buffer - the buffer is allocated and
> >>> the address doesn't change during the callback.  So, we'd need the lock for the
> >>> hr callback and for the PCM prepare, hwparams and free callbacks, i.e. to
> >>> assure that the hrtimer callback is finished when these PCM callbacks may
> >>> change the DMA buffer.  Thus this lock doesn't have to be PCM substream lock.
> >> I wonder if it would be better to introduce
> >> such a lock in an alsa core rather than in a
> >> driver. The same way as snd_pcm_stream_lock().
> > I don't think it's good to introduce yet another lock in the PCM
> > core.  More lock, more complex.  Even now the lock handling is too
> > complex (and might be buggy in some cases) in ALSA PCM core.
> Then I might be missing something.
> Right now I touch runtime->dma_area[]
> in a callback. You say it have to be
> protected, but not with alsa lock, but
> with my own lock. But I do not touch
> it ever in any other place. Everything
> else happens in an alsa core. So how
> would I possibly protect it with some
> lock private to snd-pcsp?

The only thing that we make sure is that runtime->dma_area is present
and not changed during the hrtimer callback.  Since this callback is
called only when the stream is started and running, the only places we
check are callbacks that may change the DMA buffer, namely, hw_params
and hw_free.  These should sync with hrtimer callback so that it's no
longer called before actually changing the buffer.

In this scenario, the trigger-stop could be asynchronous.  Thus, the
prepare PCM callback should sync also with the hrtimer callback to
assure that the pending callback is finished.

> > I think, however, it'd be worth to consider snd_pcm_period_elapsed()
> > to be softirq in PCM core so that the driver doesn't need
> > initializations.  This will also reduce the spin unlock/re-lock
> > procedure in IRQ handler around snd_pcm_perild_elapsed().
> I guess it will help, but such a solution
> has a tendency of being classified as a
> "stupid hack", which you've already seen
> recently. :)

No, it's totally different.  The purpose of moving
snd_pcm_period_elapsed() to a tasklet is completely valid, just like
what we did for bottom-halves -- splitting fast and slow paths.
Using a tasklet for starting the stream is really a dirty hack.

> There really must be other ways, that do
> not carry such a risk.
> 
> >> Wouldn't it be better to just make
> >> snd_pcm_period_elapsed() to not take
> >> the stream lock? There is a requirement
> >> right now to drop the stream lock before
> >> calling this. I still beleive this is
> >> the root of all the headache.
> > No.  The work done by snd_pcm_period_elapsed() touches critical
> > sections and is delicated enough to protect by a lock.
> I didn't mean it should run without a lock.
> But it shouldn't take it itself, allowing
> the caller to _not drop_ it before calling.
> Is this acceptable?

Not yet.  Some callbacks are supposed to be called outside the lock.
But, yes, it's worth to consider in future.

> > The root of all the headache is the order of the spinlock.  It's just
> > one lock in the PCM; and you must take it somewhere to protect.
> Certainly, I take it. But the unfortunate
> policy is that I have to drop it before
> calling snd_pcm_period_elapsed(). Can we
> change that?

The substream lock can be removed from hrtimer callback gracefully.
So, you have no longer the issue with this lock/unlock procedure.
In hrtimer callback, you just need to have the DMA-buffer.  That can
be protected without the substream lock, too, I believe.

> Of course, that will mean more changes
> than just to not take the lock in
> snd_pcm_period_elapsed(). Most likely
> the snd_pcm_stream_lock() and all its
> users will have to be slightly adjusted,
> but I think its pretty much a trivial
> changes, which will lead to a much cleaner
> code. And I remember the last time we
> discussed this, you admitted (some of)
> the other drivers do get this part wrong
> too. So fixing it properly once and for
> all might be a good idea as you are at
> it again.

I don't agree here unless any code is shown :)


thanks,

Takashi