On 23 May 2014 03:07, Kevin Hilman khilman@linaro.org wrote:
Overall a much better changelog, but some more comments below...
Thanks for spending more time on these :)
A clockevent device can be stopped, or its events can be masked, if next event is expected at KTIME_MAX, i.e. no events are required for very long time.
The wording here is the wrong way around IMO. The way it's worded, it sounds like it's already the case. You're trying to make the case that this *should* happen, so make sure it's worded.
Yeah, I thought of s/can/should in my first line while writing this but still did the mistake. Yes, what you are saying makes a lot of sense :)
This normally happens with NO_HZ_FULL (when tick-sched timer is removed and we don't have any more timers scheduled for long), not sure if it can happen otherwise.
This isn't specific to NO_HZ_FULL, but will happen in regular tickless idle (NO_HZ_IDLE) also.
Copying below from my reply to Frederic, he said the same thing :)
How exactly? I would have seen these fake interrupts in normal boot if that's the case as we go in and out of idle again and again.. The reason I can think of why it isn't happening for NO_HZ is: clockevent devices are normally part of CPU block (Atleast on ARM), and when CPU goes into some power down state, the even device goes as well..
Though there is one way it can happen for NO_HZ_IDLE, probably Preeti or Daniel told me this earlier and I forgot to mention it. i.e. the case of external event device.
-- This is new now :)
I still wasn't sure about the above case, though it sounds logical. And probably you guys can tell me if that's the case for some of the platforms atleast. Because as I said, it isn't happening for me with Exynos (dual A15) as timers are part of CPU block..
Anyway, kernel shouldn't make any assumptions here and should always try to stop clockevent when not in use..
If we don't STOP clockevent device, we are guaranteed to receive atleast one fake interrupt, as hrtimer_force_reprogram() isn't reprogramming the event device.
I don't think "fake" is the right word. There's nothing fake about them. You might say unneeded or spurious instead.
Spurious is the word, yes ..
Depending on particular implementation of clockevent device, this can be a fake interrupt at tick-rate.. (This was observed atleast on one implementation: arm_arch_timer.c).
I don't follow this part (or see the relevance to the series.) Was the continued spurious interrupt because that clockevent didn't implement one-shot mode, so was left in periodic mode?
Hmm, I should have given more details :(
drivers/clocksource/arm_arch_timer.c (driver for Exynos) is probably emulating ONESHOT mode from Periodic. And so we get continuous 'spurious' interrupts every 5 ms :)
A simple solution to get this fixed could be: update hrtimer_force_reprogram()
s/simple/simple (yet hacky)/ :)
/me nods..
to always reprogram clockevent device and update clockevent drivers to STOP generating events (or delay it to max time), when 'expires' is set to KTIME_MAX. But the drawback here is that every clockevent driver has to be hacked for this particular case and its very easy for new ones to miss this.
Thomas suggested to add an optional mode: CLOCK_EVT_MODE_ONESHOT_STOPPED
However, Thomas suggested...
Grrrr .. I realize that my English is really poor :)
(lkml.org/lkml/2014/5/9/508) to get tihs fixed.
s/get this fixed/solve this problem/
hmm.. thanks for looking at so minute levels ..
This requires ->set_mode() callback to have capability to return 'success' or 'failure' for this new mode.
Why? (yes, I know the answer, but it should be described here.)
Specifically for this I mentioned *optional* in above line and though these two together would make sense. Anyway, they didn't.. Will mention that..
Currently its return type is 'void'.
To change return type of set_mode() Thomas suggested to implement another callback: ->set_dev_mode(), with return type 'int'. We can then convert clockevent drivers to use this interface instead of existing ->set_mode() and then finally remove ->set_mode()'s support.
This patchset first adds another callback with return capability, set_dev_mode(), then it migrates all drivers one by one and finally removes earlier callback set_mode() when nobody is using it.
This is looking much better,
Finally :)