Re: snd_pcsp locking mess

[Takashi Iwai <tiwai@suse.de>]

At Thu, 21 Aug 2008 12:06:07 +0400,
Stas Sergeev wrote:
> 
> Hello.
> 
> Takashi Iwai wrote:
> > Maybe codes tell better than words.  The below is my untested patch.
> I finally got around to have a
> look at the patch and gave it
> some testing.
> I think it is the best solution
> for what we currently have, but
> I see a few simplifications to
> it if some simple changes to the
> alsa core are done.
> 
> 1. We can have only one sync point
> instead of many, and that should
> be the stop callback. It is not
> possible right now because the stop
> callback can be asynchronous. This
> can be solved by providing the separate
> callback to be called from snd_pcm_period_elapsed().
> I already proposed this a few years
> ago and made a patch, but it was
> rejected in favour of the current
> excessive locking we have in pcsp.
> But now as we are at simplifying
> it, I wonder if that idea can be
> re-evaluated. Note that the change
> was not intrusive at all. The new
> callback was optional. If it is
> not provided, then the old logic
> applies.

I don't remember exactly, but adding a new callback doesn't sound so
perfect.  Well, it's just one pointer, but it's added all over
places.

Anyway, could you repost it?  Then we can discuss about it more
concretely.


> 2.
> >>> +/*
> >>> > > + * 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).
> local_irq_disable() can be avoided
> as well by introducing the separate
> async stop callback.

It'll be nice, then.


> 3. The AB/BA locking can be avoided
> without the use of the softirq I think.
> Right now snd_pcm_period_elapsed()
> takes the pcm_stream_lock() because
> it calls the stop callback which needs
> that lock. But, having the separate
> stop callback, snd_pcm_period_elapsed()
> may probably just take some other lock
> for its internal needs, and take the
> pcm_stream_lock() only if the separate
> stop callback is not provided.
> 
> The above changes look fairly simple,
> and I even had the patch for 1 already.
> The changes to snd-pcsp will became
> much simpler with them I beleive.
> And having the possibility to provide
> a separate callback for the IRQ context
> looks like the really must-have thing
> to me from the very beginning. I even
> remember the old OSS callbacks which
> were passed the flag to indicate whether
> or not they are called from an IRQ
> context. I prefer the separate callback
> to the flag, but in alsa there is currently
> neither.

Well, the IRQ context and locking are different things.
A known problem with PCM substream lock is that it's to be used for
multiple bound streams as well.  My concern is whether this can be
still avoided well by your changes.

But, in general, I'd love to clean up the PCM stuff, especially these
lock messes.  Let's cook a stew a bit more.


thanks,

Takashi