Hi Thomas,
On 10 July 2014 07:04, Frederic Weisbecker fweisbec@gmail.com wrote:
On Wed, Jul 09, 2014 at 11:30:41PM +0200, Thomas Gleixner wrote:
On Wed, 9 Jul 2014, Viresh Kumar wrote:
So your patch series drops active hrtimer checks after adding it, according to your subject line.
Quite useeul to drop something after adding it, right?
I meant "hrtimer" by "it". Will fix it in case this patchset is still required.
As hrtimer_start*() never fails, hrtimer_active() is guaranteed to return '1'. So, there is no point calling hrtimer_active().
Wrong as usual.
I cross-checked this with Frederic and Preeti before reaching out to you, to make sure its not 'obviously stupid'. And still couldn't get it right. :(
It's a common pattern that short timeouts are given which lead to immediate expiry so the extra round through schedule is even more pointless than the extra check.
Just wanted to confirm it again, you are talking about CPU being interrupted by clockevent device's interrupt right after hrtimer_start*() returns and before calling hrtimer_active()?
It may be a common pattern but it's not obvious at all as is in the code except for timers gurus.
It looks like error handling while it's actually an optimization.
Also what about this pattern when it's used in interrupt or interrupt-disabled code? In this case the handler is not going to fire right away, unless it's enqueued on another CPU for unpinned timers.
For example this code in tick_nohz_stop_sched_tick():
hrtimer_start(&ts->sched_timer, expires, HRTIMER_MODE_ABS_PINNED); /* Check, if the timer was already in the past */ if (hrtimer_active(&ts->sched_timer)) goto out;It's not clear what this is handling. Concurrent immediate callback expiration from another CPU? But the timer is pinned local so it can't execute right away between hrtimer_start() and hrtimer_active() check...
Actually I was concerned about other cases as well.
- Timeouts
I do agree that an extra check is better than an extra round of schedule(). But this is already achieved without calling hrtimer_active(), isn't it?
All these timeout hrtimers have hrtimer_wakeup() as there handler (as these are initialized with: hrtimer_init_sleeper()).
And on expiration hrtimer_wakeup() does this: t->task = NULL;
So would this extra call to hrtimer_active() make any difference?
- Process-context: sched changes
I am not sure if scheduler routines: start_bandwidth_timer() and start_dl_timer() would get called *only* with interrupts disabled.
But, it doesn't look obvious that the optimization Thomas mentioned earlier is relevant here as well. These might be added here for error checking.
I might be wrong here as I don't have any understanding of this code and so sorry in advance.
Note: My tree is monitored by kbuild-bot and these changes are under testing for over a week now. And I haven't received any reports of the WARN() firing in __hrtimer_start_range_ns().. Probably these short timeouts aren't getting hit at all by bot's tests.
-- viresh