On 07/08/2014 07:45 PM, Frederic Weisbecker wrote:
On Tue, Jul 08, 2014 at 07:08:46PM +0530, Viresh Kumar wrote:
On 8 July 2014 18:21, Frederic Weisbecker fweisbec@gmail.com wrote:
Makes sense, assuming I'm not missing something obvious.
Actually looking at the way it is written since the beginning, and the credentials of people that wrote it, I also have serious doubts about this stuff..
So be ready for a rant, I am :)
But you might want to add a WARN_ON_ONCE(!hrtimer_active(hrtimer)) on the end of these hrtimer_start common parts. In the end of __hrtimer_start_range_ns() maybe.
When I first read your reply, it looked a great idea. But thinking about it a bit more, it didn't look that great :)
Isn't it something like:
x = y; WARN_ON_ONCE(x != y);
Well, every state check eventually sums up to that. Now for a state check to be useful we must indeed keep some "reasonable distance" between it and the potential state write.
But then "reasonable distance" is left to personal appreciation :)
To me a function call is enough.
The best would be to check from all __hrtimer_start_range_ns() callers. So maybe __hrtimer_start_range_ns() could do the check and call __hrtimer_start_range_ns_internal().
i.e. we are adding a WARN where we are quite sure nothing can go wrong.
WARN()'s are probably good at places, where we don't expect things to happen in a certain way but they can still happen.
For example in the set_dev_mode() series, we had it on failure from ->set_dev_mode(), as platforms might try to return failures.
Having said that, I will still add this in a separate patch (the last one) just for testing. My trees are automated with kbuild bot and we can figure out if we really need to check for hrtimer_active().
This last one *shouldn't* be upstreamed :)
Let me know if you still want that WARN() to be there..
Well as you just shown above, you don't feel safe with that. That feeling alone proves that we need a state check.
Also it's very important to have one such that people are warned when they try things that go beyond our hrtimer active assumption.
Plus the check is mostly free.
Is it possible that between the time we start the hrtimer and we check for hrtimer_active(), we can get a timer interrupt which can dequeue the tick sched timer if it were to be in the past? In this case the hrtimer_active() can have a failed return value right?
Regards Preeti U Murthy