[PATCH 0/2] tick: NO_HZ_FULL: get rid of unnecessary interruption

[PATCH 0/2] tick: NO_HZ_FULL: get rid of unnecessary interruption

[Viresh Kumar]

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(-)

-- 
2.0.0.rc2

[PATCH 1/2] hrtimer: reprogram event for expires=KTIME_MAX in hrtimer_force_reprogram()

[Viresh Kumar]

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);
 }
 
 /*
-- 
2.0.0.rc2

[PATCH 2/2] tick: SHUTDOWN event-dev if no events are required for KTIME_MAX

[Viresh Kumar]

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;
 }
 
 /**
-- 
2.0.0.rc2

Re: [PATCH 1/2] hrtimer: reprogram event for expires=KTIME_MAX in hrtimer_force_reprogram()

[Preeti U Murthy]

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);
>  }
> 
>  /*
> 

Re: [PATCH 1/2] hrtimer: reprogram event for expires=KTIME_MAX in hrtimer_force_reprogram()

[Viresh Kumar]

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

Re: [PATCH 2/2] tick: SHUTDOWN event-dev if no events are required for KTIME_MAX

[Thomas Gleixner]

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

Re: [PATCH 2/2] tick: SHUTDOWN event-dev if no events are required for KTIME_MAX

[Thomas Gleixner]

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

Re: [PATCH 1/2] hrtimer: reprogram event for expires=KTIME_MAX in hrtimer_force_reprogram()

[Preeti U Murthy]

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
> 

Re: [PATCH 2/2] tick: SHUTDOWN event-dev if no events are required for KTIME_MAX

[Viresh Kumar]

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:
>
> 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.

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

Re: [PATCH 1/2] hrtimer: reprogram event for expires=KTIME_MAX in hrtimer_force_reprogram()

[Viresh Kumar]

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..

Re: [PATCH 2/2] tick: SHUTDOWN event-dev if no events are required for KTIME_MAX

[Viresh Kumar]

On 10 May 2014 16:31, Thomas Gleixner <tglx@linutronix.de> wrote:
> 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();

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

Re: [PATCH 2/2] tick: SHUTDOWN event-dev if no events are required for KTIME_MAX

[Thomas Gleixner]

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:
> >
> > 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();
> 
> 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

Re: [PATCH 1/2] hrtimer: reprogram event for expires=KTIME_MAX in hrtimer_force_reprogram()

[Preeti U Murthy]

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
>