* [PATCH 0/2] clockevents: Hide CLOCK_EVT_STATE_* from rest of the kernel
@ 2015-05-20 9:06 Viresh Kumar
2015-05-20 9:06 ` [PATCH 1/2] clockevents: Move 'enum clock_event_state' to tick-internal.h Viresh Kumar
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Viresh Kumar @ 2015-05-20 9:06 UTC (permalink / raw)
To: Thomas Gleixner
Cc: linaro-kernel, linux-kernel, Ingo Molnar, Peter Zijlstra, Viresh Kumar
Hi Thomas,
Ingo suggested [1] to keep CLOCK_EVT_STATE_* symbols somewhere in
kernel/time/. We couldn't do it as bL_switcher code was using it
earlier. But that's fixed now. And so the first patch moves these
symbols to tick-internal.h.
Some of the drivers [2] need to verify state of the clockevent device
from their callbacks or interrupt handlers.
Because these symbols (defined by 'enum clock_event_state') will now be
internal to the core, we need some helpers to verify state of a
clockevent device.
One way out was to maintain the state in drivers as well, but that would
be unnecessary burden on them. And so the second patch introduces
helpers for these states.
Rebased-over: tip/timers/core (dependency on 8fff52fd5093 ("clockevents:
Introduce CLOCK_EVT_STATE_ONESHOT_STOPPED state"))
--
viresh
[1] https://lists.linaro.org/pipermail/linaro-kernel/2015-February/020292.html
[2] http://pastebin.com/374X18mv
Viresh Kumar (2):
clockevents: Move 'enum clock_event_state' to tick-internal.h
clockevents: Add helpers to verify state of a clockevent device
include/linux/clockchips.h | 34 ++++++++++++----------------------
kernel/time/clockevents.c | 31 +++++++++++++++++++++++++++++++
kernel/time/tick-internal.h | 21 +++++++++++++++++++++
3 files changed, 64 insertions(+), 22 deletions(-)
--
2.4.0
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/2] clockevents: Move 'enum clock_event_state' to tick-internal.h
2015-05-20 9:06 [PATCH 0/2] clockevents: Hide CLOCK_EVT_STATE_* from rest of the kernel Viresh Kumar
@ 2015-05-20 9:06 ` Viresh Kumar
2015-05-20 9:06 ` [PATCH 2/2] clockevents: Add helpers to verify state of a clockevent device Viresh Kumar
2015-05-20 13:09 ` [PATCH 0/2] clockevents: Hide CLOCK_EVT_STATE_* from rest of the kernel Thomas Gleixner
2 siblings, 0 replies; 6+ messages in thread
From: Viresh Kumar @ 2015-05-20 9:06 UTC (permalink / raw)
To: Thomas Gleixner
Cc: linaro-kernel, linux-kernel, Ingo Molnar, Peter Zijlstra, Viresh Kumar
CLOCK_EVT_STATE_* symbols are local to the kernel/time and shouldn't be
exposed to other parts of the kernel.
They were added to clockchips.h as there was one external user
(arch/arm/common/bL_switcher.c).
That is modified now to not use these symbols directly:
7270d11c56f5 ("arm/bL_switcher: Kill tick suspend hackery")
We can now move them to a file local to kernel/time.
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
include/linux/clockchips.h | 23 +----------------------
kernel/time/tick-internal.h | 21 +++++++++++++++++++++
2 files changed, 22 insertions(+), 22 deletions(-)
diff --git a/include/linux/clockchips.h b/include/linux/clockchips.h
index 271fa4c8eb29..62d3007f8a8c 100644
--- a/include/linux/clockchips.h
+++ b/include/linux/clockchips.h
@@ -28,27 +28,6 @@ enum clock_event_mode {
};
/*
- * Possible states of a clock event device.
- *
- * DETACHED: Device is not used by clockevents core. Initial state or can be
- * reached from SHUTDOWN.
- * SHUTDOWN: Device is powered-off. Can be reached from PERIODIC or ONESHOT.
- * PERIODIC: Device is programmed to generate events periodically. Can be
- * reached from DETACHED or SHUTDOWN.
- * ONESHOT: Device is programmed to generate event only once. Can be reached
- * from DETACHED or SHUTDOWN.
- * ONESHOT_STOPPED: Device was programmed in ONESHOT mode and is temporarily
- * stopped.
- */
-enum clock_event_state {
- CLOCK_EVT_STATE_DETACHED,
- CLOCK_EVT_STATE_SHUTDOWN,
- CLOCK_EVT_STATE_PERIODIC,
- CLOCK_EVT_STATE_ONESHOT,
- CLOCK_EVT_STATE_ONESHOT_STOPPED,
-};
-
-/*
* Clock event features
*/
# define CLOCK_EVT_FEAT_PERIODIC 0x000001
@@ -117,7 +96,7 @@ struct clock_event_device {
u32 mult;
u32 shift;
enum clock_event_mode mode;
- enum clock_event_state state;
+ unsigned int state;
unsigned int features;
unsigned long retries;
diff --git a/kernel/time/tick-internal.h b/kernel/time/tick-internal.h
index 65273f0a11ed..04820f64146e 100644
--- a/kernel/time/tick-internal.h
+++ b/kernel/time/tick-internal.h
@@ -12,6 +12,27 @@
# define TICK_DO_TIMER_NONE -1
# define TICK_DO_TIMER_BOOT -2
+/*
+ * Possible states of a clock event device.
+ *
+ * DETACHED: Device is not used by clockevents core. Initial state or can be
+ * reached from SHUTDOWN.
+ * SHUTDOWN: Device is powered-off. Can be reached from PERIODIC or ONESHOT.
+ * PERIODIC: Device is programmed to generate events periodically. Can be
+ * reached from DETACHED or SHUTDOWN.
+ * ONESHOT: Device is programmed to generate event only once. Can be reached
+ * from DETACHED or SHUTDOWN.
+ * ONESHOT_STOPPED: Device was programmed in ONESHOT mode and is temporarily
+ * stopped.
+ */
+enum clock_event_state {
+ CLOCK_EVT_STATE_DETACHED,
+ CLOCK_EVT_STATE_SHUTDOWN,
+ CLOCK_EVT_STATE_PERIODIC,
+ CLOCK_EVT_STATE_ONESHOT,
+ CLOCK_EVT_STATE_ONESHOT_STOPPED,
+};
+
DECLARE_PER_CPU(struct tick_device, tick_cpu_device);
extern ktime_t tick_next_period;
extern ktime_t tick_period;
--
2.4.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/2] clockevents: Add helpers to verify state of a clockevent device
2015-05-20 9:06 [PATCH 0/2] clockevents: Hide CLOCK_EVT_STATE_* from rest of the kernel Viresh Kumar
2015-05-20 9:06 ` [PATCH 1/2] clockevents: Move 'enum clock_event_state' to tick-internal.h Viresh Kumar
@ 2015-05-20 9:06 ` Viresh Kumar
2015-05-20 13:09 ` [PATCH 0/2] clockevents: Hide CLOCK_EVT_STATE_* from rest of the kernel Thomas Gleixner
2 siblings, 0 replies; 6+ messages in thread
From: Viresh Kumar @ 2015-05-20 9:06 UTC (permalink / raw)
To: Thomas Gleixner
Cc: linaro-kernel, linux-kernel, Ingo Molnar, Peter Zijlstra, Viresh Kumar
Some clockevent drivers need to verify state of the clockevent device in
their callbacks or interrupt handler.
Because the symbols representing these states (defined by 'enum
clock_event_state') are internal to the core, they aren't accessible to
these driver.
Introduce helper routines that can verify state of clockevent device.
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
include/linux/clockchips.h | 11 +++++++++++
kernel/time/clockevents.c | 31 +++++++++++++++++++++++++++++++
2 files changed, 42 insertions(+)
diff --git a/include/linux/clockchips.h b/include/linux/clockchips.h
index 62d3007f8a8c..b0aa9c7b96c4 100644
--- a/include/linux/clockchips.h
+++ b/include/linux/clockchips.h
@@ -189,6 +189,12 @@ static inline void tick_setup_hrtimer_broadcast(void) { }
extern int clockevents_notify(unsigned long reason, void *arg);
+extern bool clockevent_state_detached(struct clock_event_device *evt);
+extern bool clockevent_state_shutdown(struct clock_event_device *evt);
+extern bool clockevent_state_periodic(struct clock_event_device *evt);
+extern bool clockevent_state_oneshot(struct clock_event_device *evt);
+extern bool clockevent_state_oneshot_stopped(struct clock_event_device *evt);
+
#else /* !CONFIG_GENERIC_CLOCKEVENTS: */
static inline void clockevents_suspend(void) { }
@@ -197,6 +203,11 @@ static inline int clockevents_notify(unsigned long reason, void *arg) { return 0
static inline int tick_check_broadcast_expired(void) { return 0; }
static inline void tick_setup_hrtimer_broadcast(void) { }
+static inline bool clockevent_state_detached(struct clock_event_device *evt) { return false; };
+static inline bool clockevent_state_shutdown(struct clock_event_device *evt) { return false; };
+static inline bool clockevent_state_periodic(struct clock_event_device *evt) { return false; };
+static inline bool clockevent_state_oneshot(struct clock_event_device *evt) { return false; };
+static inline bool clockevent_state_oneshot_stopped(struct clock_event_device *evt) { return false; };
#endif /* !CONFIG_GENERIC_CLOCKEVENTS */
#endif /* _LINUX_CLOCKCHIPS_H */
diff --git a/kernel/time/clockevents.c b/kernel/time/clockevents.c
index 4922f1b805ea..ce9bb174d9bb 100644
--- a/kernel/time/clockevents.c
+++ b/kernel/time/clockevents.c
@@ -33,6 +33,37 @@ struct ce_unbind {
int res;
};
+/* Helpers to verify state of a clockevent device */
+bool clockevent_state_detached(struct clock_event_device *evt)
+{
+ return evt->state == CLOCK_EVT_STATE_DETACHED;
+}
+EXPORT_SYMBOL_GPL(clockevent_state_detached);
+
+bool clockevent_state_shutdown(struct clock_event_device *evt)
+{
+ return evt->state == CLOCK_EVT_STATE_SHUTDOWN;
+}
+EXPORT_SYMBOL_GPL(clockevent_state_shutdown);
+
+bool clockevent_state_periodic(struct clock_event_device *evt)
+{
+ return evt->state == CLOCK_EVT_STATE_PERIODIC;
+}
+EXPORT_SYMBOL_GPL(clockevent_state_periodic);
+
+bool clockevent_state_oneshot(struct clock_event_device *evt)
+{
+ return evt->state == CLOCK_EVT_STATE_ONESHOT;
+}
+EXPORT_SYMBOL_GPL(clockevent_state_oneshot);
+
+bool clockevent_state_oneshot_stopped(struct clock_event_device *evt)
+{
+ return evt->state == CLOCK_EVT_STATE_ONESHOT_STOPPED;
+}
+EXPORT_SYMBOL_GPL(clockevent_state_oneshot_stopped);
+
static u64 cev_delta2ns(unsigned long latch, struct clock_event_device *evt,
bool ismax)
{
--
2.4.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 0/2] clockevents: Hide CLOCK_EVT_STATE_* from rest of the kernel
2015-05-20 9:06 [PATCH 0/2] clockevents: Hide CLOCK_EVT_STATE_* from rest of the kernel Viresh Kumar
2015-05-20 9:06 ` [PATCH 1/2] clockevents: Move 'enum clock_event_state' to tick-internal.h Viresh Kumar
2015-05-20 9:06 ` [PATCH 2/2] clockevents: Add helpers to verify state of a clockevent device Viresh Kumar
@ 2015-05-20 13:09 ` Thomas Gleixner
2015-05-20 14:04 ` Viresh Kumar
2 siblings, 1 reply; 6+ messages in thread
From: Thomas Gleixner @ 2015-05-20 13:09 UTC (permalink / raw)
To: Viresh Kumar; +Cc: linaro-kernel, linux-kernel, Ingo Molnar, Peter Zijlstra
On Wed, 20 May 2015, Viresh Kumar wrote:
> Ingo suggested [1] to keep CLOCK_EVT_STATE_* symbols somewhere in
> kernel/time/. We couldn't do it as bL_switcher code was using it
> earlier. But that's fixed now. And so the first patch moves these
> symbols to tick-internal.h.
>
> Some of the drivers [2] need to verify state of the clockevent device
> from their callbacks or interrupt handlers.
They look at clock_event_mode and not at state, right?
> Because these symbols (defined by 'enum clock_event_state') will now be
> internal to the core, we need some helpers to verify state of a
> clockevent device.
So how are they affected by moving clock_event_state to the core code ?
> One way out was to maintain the state in drivers as well, but that would
> be unnecessary burden on them. And so the second patch introduces
> helpers for these states.
And that way we add the overhead of a full function call to those
drivers for the interrupt hot path?
I just looked at the drivers and there are three classes of mode
checks and CLOCK_EVT_MODE_ usage:
1) Interrupt handler:
Act depending on the mode:
- disable the device in case of oneshot
vf_pit_timer
timer-atlas7
sh_cmt
sh_tmu
rockchip_timer
qcom-timer
fsl_ftm_timer
cs5535-clockevt
arm_global_timer
evt-sb1250
cevt-bcm1480
mips/jz4740
dc21285-timer
arc/time
alpha/time
- conditionally handle the device in case of shared
interrupt hardware with trainwreck design or other
shortcomings of the timer hardware
timer-atmel-pit
cs5535-clockevt
x86/apic
2) Sensible checks:
i8253 init_pit_timer()
x86/i8253 init_pit_clocksource()
cs5535_mfgpt init_mfgpt_timer()
avr32/time comparator_mode()
3) Use cases which can be fixed by conversion to the seperate mode
setters
x86/apic Calibration code
4) Simple to fix stuff
nios2 nios2_timer_config()
exynos_mct exynos4_mct_comp0_start()
tcb_clksrc tc_mode()
5) Set mode from driver code:
time-armada-370-xp armada_370_xp_timer_stop()
qcom-timer msm_local_timer_stop()
exynos_mct exynos4_local_timer_stop()
arm_global_timer gt_clockevents_stop()
arm_arch_timer arch_timer_stop()
smp_twd twd_timer_stop()
hyperv/hv hv_synic_cleanup()
6) Random nonsense:
xen/time xen_timerop_set_next_event()
xen_vcpuop_set_next_event()
hyper/hv hv_ce_set_next_event()
sh_cmt sh_cmt_clock_event_next()
sh_tmu sh_tmu_clock_event_next()
arm/mach-imx/time mxc_set_mode()
arm/mach-imx/apit epit_set_mode()
mxs_timer mxs_set_mode()
That list is way more useful than a pastebin with random grep output,
which does not cover use cases which SET a mode from driver code and
does not cover local storage of modes.
Of course you should have done that analysis before posting some
random helper functions.
Lets look how useful these functions are for the various use cases
#1) Adds function call over head to the timer interrupt
Hot path does matter and that function call is a regression. So
that's a NONO
#2) The function call overhead does not matter much for these, but
they could be simply fixed by using local or device storage as
well.
#3) A non issue
#4) Trivial to fix
#5) Trivial to replace by the explicit setter functions, but we want
to know WHY this is done in the first place
#6) Simple to remove random crappola.
I already did the patches while analysing the code, so that will
be gone soon.
Now explain me how your magic functions help. For most of the cases
they would be a performance regression. And for the rest they really
do not matter at all.
Brilliant stuff that.
Thanks,
tglx
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 0/2] clockevents: Hide CLOCK_EVT_STATE_* from rest of the kernel
2015-05-20 13:09 ` [PATCH 0/2] clockevents: Hide CLOCK_EVT_STATE_* from rest of the kernel Thomas Gleixner
@ 2015-05-20 14:04 ` Viresh Kumar
2015-05-20 14:20 ` Thomas Gleixner
0 siblings, 1 reply; 6+ messages in thread
From: Viresh Kumar @ 2015-05-20 14:04 UTC (permalink / raw)
To: Thomas Gleixner; +Cc: linaro-kernel, linux-kernel, Ingo Molnar, Peter Zijlstra
On 20-05-15, 15:09, Thomas Gleixner wrote:
> On Wed, 20 May 2015, Viresh Kumar wrote:
> They look at clock_event_mode and not at state, right?
Yeah, it was all useful (that's what I thought initially, but not
anymore) only when we migrate some drivers to the new per-state APIs.
> And that way we add the overhead of a full function call to those
> drivers for the interrupt hot path?
Honestly, I didn't realize that this can be a blocker. My bad.
> Of course you should have done that analysis before posting some
> random helper functions.
I did looked at all the drivers few days back, but failed to give a
summary similar to yours. No excuses.
> Lets look how useful these functions are for the various use cases
>
> #1) Adds function call over head to the timer interrupt
>
> Hot path does matter and that function call is a regression. So
> that's a NONO
> Now explain me how your magic functions help. For most of the cases
> they would be a performance regression. And for the rest they really
> do not matter at all.
They wouldn't help at all in that case.
So, probably we are left with following choices:
- Maintain state internally within the driver. SMP cases need per-cpu
storage as clkevt devices are per-cpu. Probably that's a NONO as
well ?
- Use CLK_EVT_STATE_* directly in drivers (similar to the way we use
CLK_EVT_MODE_* today).
- Write the routines I proposed as macros or inline functions in
clockchips.h, and use them. Of course that wouldn't stop exposing
CLK_EVT_STATE_* to rest of the kernel.
- Something else ?
Which one do you suggest ?
--
viresh
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 0/2] clockevents: Hide CLOCK_EVT_STATE_* from rest of the kernel
2015-05-20 14:04 ` Viresh Kumar
@ 2015-05-20 14:20 ` Thomas Gleixner
0 siblings, 0 replies; 6+ messages in thread
From: Thomas Gleixner @ 2015-05-20 14:20 UTC (permalink / raw)
To: Viresh Kumar; +Cc: linaro-kernel, linux-kernel, Ingo Molnar, Peter Zijlstra
On Wed, 20 May 2015, Viresh Kumar wrote:
> So, probably we are left with following choices:
>
> - Maintain state internally within the driver. SMP cases need per-cpu
> storage as clkevt devices are per-cpu. Probably that's a NONO as
> well ?
>
> - Use CLK_EVT_STATE_* directly in drivers (similar to the way we use
> CLK_EVT_MODE_* today).
>
> - Write the routines I proposed as macros or inline functions in
> clockchips.h, and use them. Of course that wouldn't stop exposing
> CLK_EVT_STATE_* to rest of the kernel.
I don't think that there is anything wrong with letting drivers access
it. We can make it a little bit harder to do it without accessor
functions, like I did with the state in irq_data: state_use_accessors
and have inline helpers to access it.
That makes it simple to grep for abusers :)
Thanks,
tglx
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2015-05-20 14:20 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-05-20 9:06 [PATCH 0/2] clockevents: Hide CLOCK_EVT_STATE_* from rest of the kernel Viresh Kumar
2015-05-20 9:06 ` [PATCH 1/2] clockevents: Move 'enum clock_event_state' to tick-internal.h Viresh Kumar
2015-05-20 9:06 ` [PATCH 2/2] clockevents: Add helpers to verify state of a clockevent device Viresh Kumar
2015-05-20 13:09 ` [PATCH 0/2] clockevents: Hide CLOCK_EVT_STATE_* from rest of the kernel Thomas Gleixner
2015-05-20 14:04 ` Viresh Kumar
2015-05-20 14:20 ` Thomas Gleixner
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.