At Thu, 29 May 2008 00:08:32 +0400, Stas Sergeev wrote:
Hello.
Takashi Iwai wrote:
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.
Unless the entire callback is done in a softirq, as it currently is, no?
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.
Oh. But in that case I guess it would 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.
Btw, could you please point me to where to read about the softirq expense? I thought they are very cheap, almost zero overhead.
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.
And yes, if the sotfirqs are expensive, then indeed I see why you propose all these tricks you do... But if they are not and we can keep the entire callback in a softirq as it currently is, then there might be a simpler solutions.
the chip->substream_lock for that (why?),
Because the pointer callback and the hrtimer callback can race about
I asked only why have you chosen chip->substream_lock - IIRC it was intended not for this.
- 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.
Yes, but the state doesn't seem to be guaranteed. Running the handler after the pcsp_stop_playing(chip) called, looks nasty.
This is there because pcsp_stop_playing() doesn't sync. Both hrtimer_cancel() and tasklet_kill() are basically just for sync, not for killing the object.
The timer will be in a different mode. Nothing bad will likely to happen, but... esp with the nforce workaround, where the callback is supposed to be called even amount of times. Doesn't look very good to me, even if nothing bad can happen...
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.
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.
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.
I haven't tried that. :) I have tried the snd_pcm_stream_lock(), 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. Remember that hrtimer callback requires only the DMA-buffer pointer during its operation. No other thing is needed from the substream. This can be guaranteed with a simpler logic.
So it is barely usefull. But it can be improved (or an alternative provided), the way the second lock is not needed. Then I will need only that - a single lock, nothing more. This might not be even difficult to do at all.
Well, my version is almost i8259_lock only. As mentioned, chip->substream_lock can be removed safely with a barrier.
Takashi