Re: snd_pcsp locking mess

[Takashi Iwai <tiwai@suse.de>]

At Thu, 29 May 2008 21:07:55 +0400,
Stas Sergeev wrote:
> 
> Hello.
> 
> Takashi Iwai wrote:
> >> 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.
> But why do you need both changes?
> If you are going to change snd_pcm_period_elapsed()
> itself, then how would the snd-pcsp
> change be justified?

snd-pcsp needs anyway the change to HRTIMER_IRQSAFE again.

And, moving snd_pcm_period_elapsed() to a tasklet isn't needed for
RT kernels.  It's basically a good thing, but just not mandatory like
snd-pcsp.

> > 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.
> OK, I see.
> 
> > 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.
> But for some reasons it looks safer
> to me...

A spinlock is like a gun: you can protect yourself with it, and you
can shoot others with it :)

> >>> 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.
> The difference is that in a softirq,
> the hrtimer code doesn't take a lock.

But it may cause automatically latencies for beep controls.
The latency isn't a big issue for snd_pcm_period_elapsed() (unless you
want realtime thingy with snd-pcsp), but for beep controls, it does.

> The AB/BA problem was because of the
> hrtimer' lock, which is not there
> with the softirq callback. So what
> exactly A and B do you mean here?

That's what I meant for, too.  Note that in the above text, I presume
that the hrtimer is called in HRTIMER_IRQSAFE.

> >> 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.
> But that was suggested by you in the
> past. :)

Yeah, that's my failure, obviously.  I just thought your hrtimer
callback must be constantly called and hard to sync.

> But adding a syncronization
> point should be better I agree.
> I'll see if some minimal change is
> possible that will look sane to you
> and wont make me ask 100 questions
> about its safety at the same time. :)

OK.


thanks,

Takashi