Re: snd_pcsp locking mess

[Takashi Iwai <tiwai@suse.de>]

At Sun, 18 May 2008 22:20:43 +0400,
Stas Sergeev wrote:
> 
> bugme-daemon@bugzilla.kernel.org wrote:
> > ------- Comment #17 from tiwai@suse.de  2008-05-18 10:22 -------
> > 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.

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

> > The current lock mess may come from the PCM trigger (STOP) called from
> > snd_pcm_period_elapsed().  So, putting snd_pcm_period_elapsed() out of hrtimer
> > lock is anyway necessary.  A tasklet for snd_pcm_period_elapsed() is needed for
> > this reason, too.
> 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.

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.

A typical solution for this kind of problem is to use a global lock on
the top.  But, in your case, we can't -- because hrtimer doesn't want
such (of course).  That's why snd_pcm_period_elapsed() is better put
out to a tasklet in your case.  Also, this will gives two advantages:
no spinlock mess and the reduction of too long codepath in hrtimer
callback.

> If you
> need to drop it, then you either accept
> the race or need a second lock, and the
> troubles ensue.


Takashi