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

2014-05-12 Thread Thomas Gleixner

On Mon, 12 May 2014, Viresh Kumar wrote:

> On 10 May 2014 16:31, Thomas Gleixner  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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


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

2014-05-12 Thread Viresh Kumar
On 10 May 2014 16:31, Thomas Gleixner  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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


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

2014-05-12 Thread Viresh Kumar
On 10 May 2014 16:31, Thomas Gleixner t...@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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


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

2014-05-12 Thread Thomas Gleixner

On Mon, 12 May 2014, Viresh Kumar wrote:

 On 10 May 2014 16:31, Thomas Gleixner t...@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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


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

2014-05-11 Thread Viresh Kumar
Thanks for blasting me off, it might be very helpful going forward :)

On 10 May 2014 01:39, Thomas Gleixner  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 

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

2014-05-11 Thread Viresh Kumar
Thanks for blasting me off, it might be very helpful going forward :)

On 10 May 2014 01:39, Thomas Gleixner t...@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 

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

2014-05-10 Thread 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


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

2014-05-10 Thread 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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


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

2014-05-09 Thread 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 
> ---
>  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 intfeatures;
>   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 

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

2014-05-09 Thread 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 
---
 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 intfeatures;
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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


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

2014-05-09 Thread 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.ku...@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 intfeatures;
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

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


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

2014-05-09 Thread 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.ku...@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 intfeatures;
   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