This happened for me during my NO_HZ_FULL testing on isolated CPU, but may happen otherwise as well.
When the last htimer of a CPU is cancelled (For example: for NO_HZ_FULL when expires == KTIME_MAX), we do not SHUTDOWN the event device. And because of that we will get interrupted unnecessarily on the isolated core as event device will interrupt as per the last value it is configured with.
The implementation of event device's driver may make it worse. For example: drivers/clocksource/arm_arch_timer.c disables the event device only on SHUTDOWN/UNUSED requests in set-mode. Otherwise, it will keep giving interrupts at tick interval even if hrtimer_interrupt() didn't reprogram tick (When expires == KTIME_MAX). And so we will keep getting interrupt every few milliseconds. To confirm this I added a small hack in hrtimer.c [1] and got these results [2] as soon as the CPU got isolated with NO_HZ_FULL and cpusets.
Probably the right solution to fix this is to disable the event device for such cases, i.e. expires == KTIME_MAX and restart it later when required. This will get rid of unnecessary interruption which can be avoided.
Tested over 3.15-rc4 on ARM Exynos5250 (Dual A15) board.
NOTE: Current implementation of NO_HZ_FULL has a limitation of 1 second and so we might never end up cancelling sched_timer. I was using Kevin's debug patch: https://lkml.org/lkml/2013/12/17/649, with which I am able to get expires to KTIME_MAX and so ended up cancelling hrtimer.
[1] http://pastebin.com/MhDdawVd [2] http://pastebin.com/5U5FBbTW
Viresh Kumar (2): hrtimer: reprogram event for expires=KTIME_MAX in hrtimer_force_reprogram() tick: SHUTDOWN event-dev if no events are required for KTIME_MAX
include/linux/clockchips.h | 1 + kernel/hrtimer.c | 3 +-- kernel/time/tick-oneshot.c | 14 +++++++++++++- 3 files changed, 15 insertions(+), 3 deletions(-)
In hrtimer_force_reprogram(), we are reprogramming event device only if the next timer event is before KTIME_MAX. But what if it is equal to KTIME_MAX? As we aren't reprogramming it again, it will be set to the last value it was, probably tick interval, i.e. few milliseconds.
And we will get a interrupt due to that, wouldn't have any hrtimers to service and return without doing much. But the implementation of event device's driver may make it more stupid. For example: drivers/clocksource/arm_arch_timer.c disables the event device only on SHUTDOWN/UNUSED requests in set-mode. Otherwise, it will keep giving interrupts at tick interval even if hrtimer_interrupt() didn't reprogram tick..
To get this fixed, lets reprogram event device even for KTIME_MAX, so that the timer is scheduled for long enough.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- kernel/hrtimer.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/kernel/hrtimer.c b/kernel/hrtimer.c index 6b715c0..b21085c 100644 --- a/kernel/hrtimer.c +++ b/kernel/hrtimer.c @@ -591,8 +591,7 @@ hrtimer_force_reprogram(struct hrtimer_cpu_base *cpu_base, int skip_equal) if (cpu_base->hang_detected) return;
- if (cpu_base->expires_next.tv64 != KTIME_MAX) - tick_program_event(cpu_base->expires_next, 1); + tick_program_event(cpu_base->expires_next, 1); }
/*
Hi Viresh,
On 05/09/2014 02:10 PM, Viresh Kumar wrote:
In hrtimer_force_reprogram(), we are reprogramming event device only if the next timer event is before KTIME_MAX. But what if it is equal to KTIME_MAX? As we aren't reprogramming it again, it will be set to the last value it was, probably tick interval, i.e. few milliseconds.
And we will get a interrupt due to that, wouldn't have any hrtimers to service and return without doing much. But the implementation of event device's driver may make it more stupid. For example: drivers/clocksource/arm_arch_timer.c disables the event device only on SHUTDOWN/UNUSED requests in set-mode. Otherwise, it will keep giving interrupts at tick interval even if hrtimer_interrupt() didn't reprogram tick..
To get this fixed, lets reprogram event device even for KTIME_MAX, so that the timer is scheduled for long enough.
I looked through the code in arm_arch_timer.c and I think the more fundamental problem lies in the timer handler there. Ideally even before calling the tick event handler the timer handler must be programming the tick device to fire at some __MAX__ time. Then irrespective of whether the core kernel deems it appropriate to program it or not, the max time by which a timer interrupt will get deferred is __MAX__ and one will not find anomalies like what you saw.
The reason this got exposed in NOHZ_FULL config is because in a normal NOHZ scenario when the cpu goes idle, and there are no pending timers in timer_list, even then tick_sched_timer gets cancelled. Precisely the scenario that you have described. But we don't get continuous interrupts then because the first time we get an interrupt, we queue the tick_sched_timer and program the tick device to the time of its expiry and therefore *push* the time at which your tick device should fire further. In case of NOHZ_FULL however I am presuming you will not queue the tick_sched_timer again unless there is more than one process in the runqueue. Therefore the tick device keeps firing since its counter remains in the expired state and is not pushed up.
Moreover from the core kernel's perspective also this does not look like the right thing to do. The core timer code cannot *shutdown* a clock device simply because there are no pending timers. Some arch may change their notion of *shutdown* to rendering the tick device unusable. Some archs may already do that. Hence I don't think we should take a drastic measure as to shutdown the clock device in case of no pending timers,
My suggestion is as pointed above to set the tick device to a KTIME_MAX equivalent before calling the timer interrupt event handler.
Regards Preeti U Murthy
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
kernel/hrtimer.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/kernel/hrtimer.c b/kernel/hrtimer.c index 6b715c0..b21085c 100644 --- a/kernel/hrtimer.c +++ b/kernel/hrtimer.c @@ -591,8 +591,7 @@ hrtimer_force_reprogram(struct hrtimer_cpu_base *cpu_base, int skip_equal) if (cpu_base->hang_detected) return;
- if (cpu_base->expires_next.tv64 != KTIME_MAX)
tick_program_event(cpu_base->expires_next, 1);
- tick_program_event(cpu_base->expires_next, 1);
}
/*
On 9 May 2014 16:04, Preeti U Murthy preeti@linux.vnet.ibm.com wrote:
On 05/09/2014 02:10 PM, Viresh Kumar wrote:
I looked through the code in arm_arch_timer.c and I think the more fundamental problem lies in the timer handler there. Ideally even before calling the tick event handler the timer handler must be programming the tick device to fire at some __MAX__ time.
Ideally, the device should have stopped events as we programmed it in ONESHOT mode. And should have waited for kernel to set it again..
But probably that device doesn't have a ONESHOT mode and is firing again and again. Anyway the real problem I was trying to solve wasn't infinite interrupts coming from event dev, but the first extra event that we should have got rid of .. It just happened that we got more problems on this particular board.
Then irrespective of whether the core kernel deems it appropriate to program it or not, the max time by which a timer interrupt will get deferred is __MAX__ and one will not find anomalies like what you saw.
We will still get a interrupt once the counter overflows. And that is bad too.
The reason this got exposed in NOHZ_FULL config is because in a normal NOHZ scenario when the cpu goes idle, and there are no pending timers in timer_list, even then tick_sched_timer gets cancelled. Precisely the scenario that you have described.
I haven't tried but it looks like this problem will exist there as well.. Who is disabling the event device in that case when tick_sched timer goes off ? The same question that is applicable in this case as well..
But we don't get continuous interrupts then because the first time we get an interrupt, we queue the tick_sched_timer and program the tick device to the time of its expiry and therefore *push* the time at which your tick device should fire further.
Probably not.. We don't get continuous interrupts because that's a special case for my platform. But I am quite sure you would be getting one extra interrupt after tick period, but because we didn't had anything to service hrtimer_interrupt() routine just returned and CPU went into idle.
Moreover from the core kernel's perspective also this does not look like the right thing to do. The core timer code cannot *shutdown* a clock device simply because there are no pending timers.
Why? To me it looks the right thing to do..
Some arch may change their notion of *shutdown* to rendering the tick device unusable. Some archs may already do that.
There is only one definition of 'Shutdown' for me which every platform must implement. Stop the event device to give any new events. that's it.
Hence I don't think we should take a drastic measure as to shutdown the clock device in case of no pending timers,
Sorry, I still don't agree :) .. We don't know when is the next time we need to use a service, so free it. What will we get by pushing it to a long long time ? What would we loose if we SHUTDOWN it now ?
My suggestion is as pointed above to set the tick device to a KTIME_MAX equivalent before calling the timer interrupt event handler.
This would still interrupt on overflow, so isn't the right idea.. Not currently as there are limitations, but later on with NO_HZ_FULL a core should be allowed to go into infinite isolation, unless the application running on it wants.. And this pushing to KTIME_MAX wouldn't work in that case..
Thanks for your review and the long chats we had about this problem since yesterday on IRC..
-- viresh
On 05/09/2014 04:27 PM, Viresh Kumar wrote:
On 9 May 2014 16:04, Preeti U Murthy preeti@linux.vnet.ibm.com wrote:
On 05/09/2014 02:10 PM, Viresh Kumar wrote:
I looked through the code in arm_arch_timer.c and I think the more fundamental problem lies in the timer handler there. Ideally even before calling the tick event handler the timer handler must be programming the tick device to fire at some __MAX__ time.
Ideally, the device should have stopped events as we programmed it in ONESHOT mode. And should have waited for kernel to set it again..
But probably that device doesn't have a ONESHOT mode and is firing again and again. Anyway the real problem I was trying to solve wasn't infinite interrupts coming from event dev, but the first extra event that we should have got rid of .. It just happened that we got more problems on this particular board.
So on a timer interrupt the tick device, irrespective of if it is in ONESHOT mode or not, is in an expired state. Thus it will continue to fire. What has ONESHOT mode got to do with this?
Then irrespective of whether the core kernel deems it appropriate to program it or not, the max time by which a timer interrupt will get deferred is __MAX__ and one will not find anomalies like what you saw.
We will still get a interrupt once the counter overflows. And that is bad too.
Hmm true.
The reason this got exposed in NOHZ_FULL config is because in a normal NOHZ scenario when the cpu goes idle, and there are no pending timers in timer_list, even then tick_sched_timer gets cancelled. Precisely the scenario that you have described.
I haven't tried but it looks like this problem will exist there as well.. Who is disabling the event device in that case when tick_sched timer goes off ? The same question that is applicable in this case as well..
But we don't get continuous interrupts then because the first time we get an interrupt, we queue the tick_sched_timer and program the tick device to the time of its expiry and therefore *push* the time at which your tick device should fire further.
Probably not.. We don't get continuous interrupts because that's a special case for my platform. But I am quite sure you would be getting one extra interrupt after tick period, but because we didn't had anything to service
Hmm? I didn't get this. Why would we? We ensure that if there are no pending timers in timer_list the tick_sched_timer is cancelled. We cannot get spurious interrupts when there are no pending timers in NOHZ mode.
hrtimer_interrupt() routine just returned and CPU went into idle.
Moreover from the core kernel's perspective also this does not look like the right thing to do. The core timer code cannot *shutdown* a clock device simply because there are no pending timers.
Why? To me it looks the right thing to do..
Some arch may change their notion of *shutdown* to rendering the tick device unusable. Some archs may already do that.
There is only one definition of 'Shutdown' for me which every platform must implement. Stop the event device to give any new events. that's it.
Hence I don't think we should take a drastic measure as to shutdown the clock device in case of no pending timers,
Sorry, I still don't agree :) .. We don't know when is the next time we need to use a service, so free it. What will we get by pushing it to a long long time ? What would we loose if we SHUTDOWN it now ?
My suggestion is as pointed above to set the tick device to a KTIME_MAX equivalent before calling the timer interrupt event handler.
This would still interrupt on overflow, so isn't the right idea.. Not currently as there are limitations, but later on with NO_HZ_FULL a core should be allowed to go into infinite isolation, unless the application running on it wants.. And this pushing to KTIME_MAX wouldn't work in that case..
Hmm yeah looking at the problem that you are trying to solve, that being completely disabling timer interrupts on cpus that are running just one process, it appears to me that setting the tick device in SHUTDOWN mode is the only way to do so. And you are right. We use SHUTDOWN mode to imply that the device can be switched off. Its upto the arch to react to it appropriately.
My concern is on powerpc today when we set the device to SHUTDOWN mode we set the decrementer to a MAX value. Which means we will get interrupts only spaced out more widely in time. But on NOHZ_FULL mode if you are looking at completely disabling tick_sched_timer as long as a single process runs then we might need to change the semantics here.
Regards Preeti U Murthy
Thanks for your review and the long chats we had about this problem since yesterday on IRC..
-- viresh
On 10 May 2014 21:47, Preeti U Murthy preeti@linux.vnet.ibm.com wrote:
On 05/09/2014 04:27 PM, Viresh Kumar wrote:
On 9 May 2014 16:04, Preeti U Murthy preeti@linux.vnet.ibm.com wrote:
Ideally, the device should have stopped events as we programmed it in ONESHOT mode. And should have waited for kernel to set it again..
But probably that device doesn't have a ONESHOT mode and is firing again and again. Anyway the real problem I was trying to solve wasn't infinite interrupts coming from event dev, but the first extra event that we should have got rid of .. It just happened that we got more problems on this particular board.
So on a timer interrupt the tick device, irrespective of if it is in ONESHOT mode or not, is in an expired state. Thus it will continue to fire. What has ONESHOT mode got to do with this?
So, the arch specific timer handler must be clearing it I suppose and it shouldn't have fired again after 5 ms as it is not reprogrammed.
Probably that's an implementation specific stuff.. I have seen timers which have two modes, periodic: they fire continuously and oneshot: they get disabled after firing and have to be reprogrammed.
The reason this got exposed in NOHZ_FULL config is because in a normal NOHZ scenario when the cpu goes idle, and there are no pending timers in timer_list, even then tick_sched_timer gets cancelled. Precisely the scenario that you have described.
I haven't tried but it looks like this problem will exist there as well.. Who is disabling the event device in that case when tick_sched timer goes off ? The same question that is applicable in this case as well..
But we don't get continuous interrupts then because the first time we get an interrupt, we queue the tick_sched_timer and program the tick device to the time of its expiry and therefore *push* the time at which your tick device should fire further.
Probably not.. We don't get continuous interrupts because that's a special case for my platform. But I am quite sure you would be getting one extra interrupt after tick period, but because we didn't had anything to service
Hmm? I didn't get this. Why would we? We ensure that if there are no pending timers in timer_list the tick_sched_timer is cancelled. We cannot get spurious interrupts when there are no pending timers in NOHZ mode.
Okay, there are no pending timers to fire and even we have disabled tick_sched_timer as well.. But the event dev isn't SHUTDOWN or reprogrammed. And so it must fire after tick interval? Exactly the same issue we are getting here in NO_HZ_FULL..
And the worst part is we aren't getting these interrupts in traces as well. Somebody probably need to revisit the trace_irq_handler_entry part as well to catch such problems.
Hmm yeah looking at the problem that you are trying to solve, that being completely disabling timer interrupts on cpus that are running just one process, it appears to me that setting the tick device in SHUTDOWN mode is the only way to do so. And you are right. We use SHUTDOWN mode to imply that the device can be switched off. Its upto the arch to react to it appropriately.
So, from the mail where tglx blasted me off, we have a better solution to implement now :)
My concern is on powerpc today when we set the device to SHUTDOWN mode we set the decrementer to a MAX value. Which means we will get interrupts only spaced out more widely in time. But on NOHZ_FULL mode if you are looking at completely disabling tick_sched_timer as long as a single process runs then we might need to change the semantics here.
Lets see if we can do some nice stuff with ONESHOT_STOPPED state..
On 05/12/2014 11:23 AM, Viresh Kumar wrote:
On 10 May 2014 21:47, Preeti U Murthy preeti@linux.vnet.ibm.com wrote:
On 05/09/2014 04:27 PM, Viresh Kumar wrote:
On 9 May 2014 16:04, Preeti U Murthy preeti@linux.vnet.ibm.com wrote:
Ideally, the device should have stopped events as we programmed it in ONESHOT mode. And should have waited for kernel to set it again..
But probably that device doesn't have a ONESHOT mode and is firing again and again. Anyway the real problem I was trying to solve wasn't infinite interrupts coming from event dev, but the first extra event that we should have got rid of .. It just happened that we got more problems on this particular board.
So on a timer interrupt the tick device, irrespective of if it is in ONESHOT mode or not, is in an expired state. Thus it will continue to fire. What has ONESHOT mode got to do with this?
So, the arch specific timer handler must be clearing it I suppose and it shouldn't have fired again after 5 ms as it is not reprogrammed.
Probably that's an implementation specific stuff.. I have seen timers which have two modes, periodic: they fire continuously and oneshot: they get disabled after firing and have to be reprogrammed.
Yes that is implementation specific. From a core kernel code's perspective, periodic mode is where the clock device will fire periodically; it resets itself on every expiry. Oneshot mode is where the clock device should explicitly be programmed to fire and can be programmed for any point in time unlike periodic where the clock device should be programmed only at periodic intervals. That is it. Beyond that the core kernel cannot assume anything more.
Its possible that in oneshot mode some implementations disable the clock device on expiry. Some might not. Hence the core code must make decisions which *will not break either of these implementations*. For example on PowerPC we do not disable the clock device. Instead we reset the clock device to fire after 4seconds by default on expiry unless there is some timer queued which sets the clock device to fire appropriately.
The point is the implementations have their reasons for implementing these modes and the core kernel code cannot be based on "Ideally the device should have stopped" assumptions.
The reason this got exposed in NOHZ_FULL config is because in a normal NOHZ scenario when the cpu goes idle, and there are no pending timers in timer_list, even then tick_sched_timer gets cancelled. Precisely the scenario that you have described.
I haven't tried but it looks like this problem will exist there as well.. Who is disabling the event device in that case when tick_sched timer goes off ? The same question that is applicable in this case as well..
But we don't get continuous interrupts then because the first time we get an interrupt, we queue the tick_sched_timer and program the tick device to the time of its expiry and therefore *push* the time at which your tick device should fire further.
Probably not.. We don't get continuous interrupts because that's a special case for my platform. But I am quite sure you would be getting one extra interrupt after tick period, but because we didn't had anything to service
Hmm? I didn't get this. Why would we? We ensure that if there are no pending timers in timer_list the tick_sched_timer is cancelled. We cannot get spurious interrupts when there are no pending timers in NOHZ mode.
Okay, there are no pending timers to fire and even we have disabled tick_sched_timer as well.. But the event dev isn't SHUTDOWN or reprogrammed. And so it must fire after tick interval? Exactly the same issue we are getting here in NO_HZ_FULL..
Not after tick interval. If there is a pending hrtimer, the event dev will fire at its expiry time. If there are no pending hrtimers as well as timers in timer_list, then its upto the arch to decide how it will handle "no pending timer events in the future". It can set the clock device to expire at some far off time for an example.
And the worst part is we aren't getting these interrupts in traces as well. Somebody probably need to revisit the trace_irq_handler_entry part as well to catch such problems.
Hmm yeah looking at the problem that you are trying to solve, that being completely disabling timer interrupts on cpus that are running just one process, it appears to me that setting the tick device in SHUTDOWN mode is the only way to do so. And you are right. We use SHUTDOWN mode to imply that the device can be switched off. Its upto the arch to react to it appropriately.
So, from the mail where tglx blasted me off, we have a better solution to implement now :)
My concern is on powerpc today when we set the device to SHUTDOWN mode we set the decrementer to a MAX value. Which means we will get interrupts only spaced out more widely in time. But on NOHZ_FULL mode if you are looking at completely disabling tick_sched_timer as long as a single process runs then we might need to change the semantics here.
Lets see if we can do some nice stuff with ONESHOT_STOPPED state..
Indeed. That is a very clean way to getting this done. We were trying to extrapolate the existing code to solve this problem. That was bound to snap :)
Regards Preeti U Murthy
When expires is set to KTIME_MAX in tick_program_event(), we are sure that there are no events enqueued for a very long time and so there is no point keeping event device running. We will get interrupted without any work to do many a times, for example when timer's counter overflows.
So, its better to SHUTDOWN the event device then and restart it ones we get a request for next event. For implementing this a new field 'last_mode' is added to 'struct clock_event_device' to keep track of last mode used.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- include/linux/clockchips.h | 1 + kernel/time/tick-oneshot.c | 14 +++++++++++++- 2 files changed, 14 insertions(+), 1 deletion(-)
diff --git a/include/linux/clockchips.h b/include/linux/clockchips.h index 2e4cb67..36a4ca6 100644 --- a/include/linux/clockchips.h +++ b/include/linux/clockchips.h @@ -105,6 +105,7 @@ struct clock_event_device { u32 mult; u32 shift; enum clock_event_mode mode; + enum clock_event_mode last_mode; unsigned int features; unsigned long retries;
diff --git a/kernel/time/tick-oneshot.c b/kernel/time/tick-oneshot.c index 8241090..9543815 100644 --- a/kernel/time/tick-oneshot.c +++ b/kernel/time/tick-oneshot.c @@ -27,8 +27,20 @@ int tick_program_event(ktime_t expires, int force) { struct clock_event_device *dev = __this_cpu_read(tick_cpu_device.evtdev); + int ret = 0;
- return clockevents_program_event(dev, expires, force); + /* Shut down event device if it is not required for long */ + if (unlikely(expires.tv64 == KTIME_MAX)) { + dev->last_mode = dev->mode; + clockevents_set_mode(dev, CLOCK_EVT_MODE_SHUTDOWN); + } else { + /* restore mode when restarting event dev */ + if (unlikely(dev->mode == CLOCK_EVT_MODE_SHUTDOWN)) + clockevents_set_mode(dev, dev->last_mode); + ret = clockevents_program_event(dev, expires, force); + } + + return ret; }
/**
On Fri, 9 May 2014, Viresh Kumar wrote:
When expires is set to KTIME_MAX in tick_program_event(), we are sure that there are no events enqueued for a very long time and so there is no point keeping event device running. We will get interrupted without any work to do many a times, for example when timer's counter overflows.
So, its better to SHUTDOWN the event device then and restart it ones we get a request for next event. For implementing this a new field 'last_mode' is added to 'struct clock_event_device' to keep track of last mode used.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
include/linux/clockchips.h | 1 + kernel/time/tick-oneshot.c | 14 +++++++++++++- 2 files changed, 14 insertions(+), 1 deletion(-)
diff --git a/include/linux/clockchips.h b/include/linux/clockchips.h index 2e4cb67..36a4ca6 100644 --- a/include/linux/clockchips.h +++ b/include/linux/clockchips.h @@ -105,6 +105,7 @@ struct clock_event_device { u32 mult; u32 shift; enum clock_event_mode mode;
- enum clock_event_mode last_mode; unsigned int features; unsigned long retries;
diff --git a/kernel/time/tick-oneshot.c b/kernel/time/tick-oneshot.c index 8241090..9543815 100644 --- a/kernel/time/tick-oneshot.c +++ b/kernel/time/tick-oneshot.c @@ -27,8 +27,20 @@ int tick_program_event(ktime_t expires, int force) { struct clock_event_device *dev = __this_cpu_read(tick_cpu_device.evtdev);
- int ret = 0;
- return clockevents_program_event(dev, expires, force);
- /* Shut down event device if it is not required for long */
- if (unlikely(expires.tv64 == KTIME_MAX)) {
dev->last_mode = dev->mode;clockevents_set_mode(dev, CLOCK_EVT_MODE_SHUTDOWN);
No, we are not doing a state change behind the scene and a magic restore. And I know at least one way to make this fall flat on its nose, because you are blindly doing dev->last_mode = dev->mode on every invocation. So if that gets called twice without a restore in between, the device is going to be in shutdown mode forever.
It's moronic anyway as the clock event device has the state CLOCK_EVT_MODE_ONESHOT if its active, otherwise we would not be in that code path.
But what's even worse: you just define that it's the best way for all implementations of clockevents to handle this.
It's definitley NOT. Some startup/shutdown implementations are rather complex, so that would burden them with rather big latencies and some of them will even outright break.
There is a world outside of YOUR favourite subarch.
We do not hijack stuff just because we can and it works on some machines. We think about it proper.
I prevented that the GPIO folks hijacked irq_startup in a disgusting way for solving the GPIO issues for the very same reason. So in the end we added a new OPT-In interface which solved the problem without breaking any existing code. And it made the code simpler and cleaner in the very end.
If we hijack some existing facility then we audit ALL implementation sites and document that we did so and why we are sure that it won't break stuff. It still might break some oddball case, but that's not a big issue.
In the clockevents case we do not even need a new interface, but this must be made OPT-in and not a flagday change for all users.
And no we are not going to abuse a feature flag for this. It's not a feature.
I'd rather have a new state for this, simply because it is NOT shutdown. It is in ONESHOT_STOPPED state. Whether a specific implementation will use the SHUTDOWN code for it or not does not matter.
That requires a full tree update of all implementations because most of them have a switch case for the mode. And adding a state will cause all of them which do not have a default clause to omit warnings because the mode is an enum for this very reason.
And even if all of them would have a default clause, you'd need a way to OPT-In, because some of the defaults have a BUG() in there. Again, no feature flag exclusion. See above.
So the right thing to do this is:
1A) Change the prototype of the set_mode callback to return int and fixup all users. Either add the missing default clause or remove the existing BUG()/ pr_err()/whatever handling in the existing default clause and return a UNIQUE error code.
I know I should have done that from the very beginning, but in hindsight one could have done everything better.
coccinelle is your friend (if you need help ask me or Julia Lawall). But it's going to be quite some manual work on top.
1B) Audit the changes and look at the implementations. If the patch is just adding the default clause or replacing some BUG/printk error handling goto #1C
If it looks like it needs some preparatory care or if you find bugs in a particular implementation, roll back the changes and do the bug fixes and preparatory changes first as separate patches.
Go back to #1A until the coccinelle patches are just squeaky clean.
1C) Add proper error handling for the various modes to the set_mode callback call sites, only two AFAIK.
2A) Add a new mode ONESHOT_STOPPED. That's safe now as all error handling will be done in the core code.
2B) Implement the ONESHOT_STOPPED logic and make sure all of the core code is aware of it.
And don't tell me it can't be done. I've done it I don't know how many times with interrupts, timers, locking and some more. It's hard work, but it's valuable and way better than the brainless "make it work for me" hackery.
You asked me yesterday about your other hrtimer patches. You know why I do not come around to review them? Because I have found way too much half baken stuff in your patches I reviewed so far. That forces me to go through all of them with a fine comb and I simply do not scale.
Alone reviewing this patch took me several couple of hours, because I had to think through the implications and stare into the code. And you know why? Because, first of all I do not trust your patches and secondly your changelogs (especially the one of the 1/2 patch) told me clearly, that this is "works for me" hackery.
So YOU forced me to spend time on looking at the consequences all over the place instead of YOU had looked in the first place and figured it out yourself.
Did you look at ALL implementations of clock events when you made that change? Definitely NOT.
I did. And found quite some of them which are going to be hurt. I also found some of them which are buggy.
Just get it. This is CORE code and it affects ALL of its users. You can play that "hack it into submission game" with a random driver, i.e at the end of the callchain, but core code is very very differrent.
There is always the risk to break something when you work on core code and nobody will rip your head off, if you break something because you did not notice the random oddity of some use site.
But breaking stuff wholesale by just not thinking about it carefully won't earn you any brownie points.
Vs. your other pending patches, I have no idea whether I have the time and the stomach to go through them before I vanish to Japan next weekend.
If there are urgent bugfixes, which are obvious or proper thought through and explained, please resend them.
Thanks,
tglx
On Fri, 9 May 2014, Thomas Gleixner wrote:
On Fri, 9 May 2014, Viresh Kumar wrote: So the right thing to do this is:
1A) Change the prototype of the set_mode callback to return int and fixup all users. Either add the missing default clause or remove the existing BUG()/ pr_err()/whatever handling in the existing default clause and return a UNIQUE error code.
I know I should have done that from the very beginning, but in hindsight one could have done everything better. coccinelle is your friend (if you need help ask me or Julia Lawall). But it's going to be quite some manual work on top.
There is even a better way to do that:
1) Create a new callback set_state() which has an int return value.
2) Make the callsites do
if (dev->set_state) { ret = dev->set_state(); handle_return_value(); } else dev->set_mode();
3) Convert implementations one by one to use the new callback
4) Remove the set_mode callback
5) Implement new features.
Thanks,
tglx
On 10 May 2014 16:31, Thomas Gleixner tglx@linutronix.de wrote:
There is even a better way to do that:
Create a new callback set_state() which has an int return value.
Make the callsites do
if (dev->set_state) { ret = dev->set_state(); handle_return_value(); } else dev->set_mode();
Do you want me to touch clock_event_mode as well? Otherwise we will pass mode into a function setting state..
Or we can do s/mode/state after all the work suggested by you is done ..
Or leave as is..
1 & 2, should be just 1-2 patches, I will try to send them as soon as possible. Once these get your nod, will start working on fixing all the clockevent drivers.
-- viresh
On Mon, 12 May 2014, Viresh Kumar wrote:
On 10 May 2014 16:31, Thomas Gleixner tglx@linutronix.de wrote:
There is even a better way to do that:
Create a new callback set_state() which has an int return value.
Make the callsites do
if (dev->set_state) { ret = dev->set_state(); handle_return_value(); } else dev->set_mode();
Do you want me to touch clock_event_mode as well? Otherwise we will pass mode into a function setting state..
Or we can do s/mode/state after all the work suggested by you is done ..
Or leave as is..
You can name the new callback set_dev_mode() :)
set_state() was just pulled out of the air for illustration.
Thanks,
tglx
Thanks for blasting me off, it might be very helpful going forward :)
On 10 May 2014 01:39, Thomas Gleixner tglx@linutronix.de wrote:
On Fri, 9 May 2014, Viresh Kumar wrote:
diff --git a/kernel/time/tick-oneshot.c b/kernel/time/tick-oneshot.c
int tick_program_event(ktime_t expires, int force) { struct clock_event_device *dev = __this_cpu_read(tick_cpu_device.evtdev);
int ret = 0;
return clockevents_program_event(dev, expires, force);
/* Shut down event device if it is not required for long */if (unlikely(expires.tv64 == KTIME_MAX)) {dev->last_mode = dev->mode;clockevents_set_mode(dev, CLOCK_EVT_MODE_SHUTDOWN);No, we are not doing a state change behind the scene and a magic restore. And I know at least one way to make this fall flat on its nose, because you are blindly doing dev->last_mode = dev->mode on every invocation. So if that gets called twice without a restore in between, the device is going to be in shutdown mode forever.
During my tests I had this as well:
if (unlikely(expires.tv64 == KTIME_MAX)) { + WARN_ON(dev->mode == CLOCK_EVT_MODE_SHUTDOWN);
But it never got to it and I thought it might never happen, so removed it. But yes, there should be some check here for that.
It's moronic anyway as the clock event device has the state CLOCK_EVT_MODE_ONESHOT if its active, otherwise we would not be in that code path.
Yeah, Missed that earlier.
But what's even worse: you just define that it's the best way for all implementations of clockevents to handle this.
It's definitley NOT. Some startup/shutdown implementations are rather complex, so that would burden them with rather big latencies and some of them will even outright break.
There is a world outside of YOUR favourite subarch.
:)
We do not hijack stuff just because we can and it works on some machines. We think about it proper.
Agreed..
If we hijack some existing facility then we audit ALL implementation sites and document that we did so and why we are sure that it won't break stuff. It still might break some oddball case, but that's not a big issue.
Because SHUTDOWN was an existing old API, I thought it will work without breaking stuff. Yes, I must have done some auditing or made this an RFC series atleast to get the discussion going forward..
In the clockevents case we do not even need a new interface, but this must be made OPT-in and not a flagday change for all users.
And no we are not going to abuse a feature flag for this. It's not a feature.
Okay.
I'd rather have a new state for this, simply because it is NOT shutdown. It is in ONESHOT_STOPPED state. Whether a specific implementation will use the SHUTDOWN code for it or not does not matter.
Correct.
That requires a full tree update of all implementations because most of them have a switch case for the mode. And adding a state will cause all of them which do not have a default clause to omit warnings because the mode is an enum for this very reason.
And even if all of them would have a default clause, you'd need a way to OPT-In, because some of the defaults have a BUG() in there. Again, no feature flag exclusion. See above.
Okay..
So the right thing to do this is:
1A) Change the prototype of the set_mode callback to return int and fixup all users. Either add the missing default clause or remove the existing BUG()/ pr_err()/whatever handling in the existing default clause and return a UNIQUE error code.
I know I should have done that from the very beginning, but in hindsight one could have done everything better. coccinelle is your friend (if you need help ask me or Julia Lawall). But it's going to be quite some manual work on top.
Sure.
1B) Audit the changes and look at the implementations. If the patch is just adding the default clause or replacing some BUG/printk error handling goto #1C
If it looks like it needs some preparatory care or if you find bugs in a particular implementation, roll back the changes and do the bug fixes and preparatory changes first as separate patches. Go back to #1A until the coccinelle patches are just squeaky clean.1C) Add proper error handling for the various modes to the set_mode callback call sites, only two AFAIK.
2A) Add a new mode ONESHOT_STOPPED. That's safe now as all error handling will be done in the core code.
2B) Implement the ONESHOT_STOPPED logic and make sure all of the core code is aware of it.
Okay..
And don't tell me it can't be done.
No way :)
I've done it I don't know how many times with interrupts, timers, locking and some more. It's hard work, but it's valuable and way better than the brainless "make it work for me" hackery.
I didn't mean that actually. I just pin pointed how badly things can go with an example of ARM's platform. But I never meant that it must get in as it "works for me" :) .. But yes, you got that impression and I need to make sure it doesn't happen again.
You asked me yesterday about your other hrtimer patches. You know why I do not come around to review them? Because I have found way too much half baken stuff in your patches I reviewed so far.
Hmm, that's bad. I thought most of them wouldn't make any difference functionally, and so wouldn't break anything. Sorry about that.
That forces me to go through all of them with a fine comb and I simply do not scale.
Alone reviewing this patch took me several couple of hours, because I had to think through the implications and stare into the code. And you know why? Because, first of all I do not trust your patches and
I will try my best to come over that :)
secondly your changelogs (especially the one of the 1/2 patch) told me clearly, that this is "works for me" hackery.
I really didn't meant that :(
So YOU forced me to spend time on looking at the consequences all over the place instead of YOU had looked in the first place and figured it out yourself.
Did you look at ALL implementations of clock events when you made that change? Definitely NOT.
No, I didn't ... Yeah, I should have handled it in a better way.. With some more study and work..
I did. And found quite some of them which are going to be hurt. I also found some of them which are buggy.
Just get it. This is CORE code and it affects ALL of its users. You can play that "hack it into submission game" with a random driver, i.e at the end of the callchain, but core code is very very differrent.
There is always the risk to break something when you work on core code and nobody will rip your head off, if you break something because you did not notice the random oddity of some use site.
But breaking stuff wholesale by just not thinking about it carefully won't earn you any brownie points.
Agreed.
Vs. your other pending patches, I have no idea whether I have the time and the stomach to go through them before I vanish to Japan next weekend.
If there are urgent bugfixes, which are obvious or proper thought through and explained, please resend them.
Only one as far as I remember and I already got a go ahead from you on that, will resend it.
Let me get your next mail in here as well:
There is even a better way to do that:
Create a new callback set_state() which has an int return value.
Make the callsites do
if (dev->set_state) { ret = dev->set_state(); handle_return_value(); } else dev->set_mode();
Convert implementations one by one to use the new callback
Remove the set_mode callback
Implement new features.
Yeah, this is obviously going to be far more easy as there is less risk of breaking things here :)
Again, sorry for the noise (Atleast the issue was real and important). I wanted to do it in a better way but thought the existing API should work smoothly..
I will do my best to earn your trust :)
Thanks.. Viresh
linaro-kernel@lists.linaro.org
-
Preeti U Murthy -
Thomas Gleixner -
Viresh Kumar