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? Btw, could you please point me to where to read about the softirq expense? I thought they are very cheap, almost zero overhead. 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. 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...
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?
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. 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.