| Message ID | 20141208065513.3075.48721.stgit@preeti.in.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
Hi Preeti, On Mon, Dec 08, 2014 at 06:55:43AM +0000, Preeti U Murthy wrote: > Commit 5d1638acb9f6 ('tick: Introduce hrtimer based broadcast') added a > hrtimer based broadcast mode for those platforms in which local timers stop > when CPUs enter deep idle states. The commit expected the platforms to > register for this mode explicitly when they lacked a better external device > to wake up CPUs in deep idle. Given that more platforms are beginning to use > this mode, we can avoid the call to set it up on every platform that requires > it, by registering for the hrtimer based broadcast mode in the core code if > no better broadcast device is available. > > This commit also helps detect cases where the platform fails to register for > a broadcast device but invokes the help of one when entering deep idle states. > Currently we do not handle this situation at all and call the broadcast clock > device without checking for its existence. This patch will handle such buggy > cases properly. > > Signed-off-by: Preeti U Murthy <preeti@linux.vnet.ibm.com> I've just given this a go on an arm64 platform (Juno) without any system-wide clock_event_devices registered, and everything works well with CPUs entering and exiting idle states where the cpu-local timers lose state. So: Tested-by: Mark Rutland <mark.rutland@arm.com> One minor thing I noticed when testing was that /sys/devices/system/clockevents/broadcast/name contained "(null)", because we never set the name field on the clock_event_device. It's always been that way, but now might be a good time to change that to something like "broadcast_hrtimer". [...] > diff --git a/include/linux/clockchips.h b/include/linux/clockchips.h > index 2e4cb67..91754b0 100644 > --- a/include/linux/clockchips.h > +++ b/include/linux/clockchips.h > @@ -187,11 +187,11 @@ extern int tick_receive_broadcast(void); > #endif > > #if defined(CONFIG_GENERIC_CLOCKEVENTS_BROADCAST) && defined(CONFIG_TICK_ONESHOT) > -extern void tick_setup_hrtimer_broadcast(void); > +extern int __init tick_setup_hrtimer_broadcast(void); > extern int tick_check_broadcast_expired(void); > #else > static inline int tick_check_broadcast_expired(void) { return 0; } > -static inline void tick_setup_hrtimer_broadcast(void) {}; > +static inline int __init tick_setup_hrtimer_broadcast(void) { return 0; } > #endif > > #ifdef CONFIG_GENERIC_CLOCKEVENTS > @@ -207,7 +207,7 @@ static inline void clockevents_resume(void) {} > > 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 int __init tick_setup_hrtimer_broadcast(void) { return 0; } With the initcall moved to the driver we have no external users of tick_setup_hrtimer_broadcast, so I think we can remove the prototype entirely from clockchips.h... > #endif > > diff --git a/kernel/time/tick-broadcast-hrtimer.c b/kernel/time/tick-broadcast-hrtimer.c > index eb682d5..5c35995 100644 > --- a/kernel/time/tick-broadcast-hrtimer.c > +++ b/kernel/time/tick-broadcast-hrtimer.c > @@ -98,9 +98,11 @@ static enum hrtimer_restart bc_handler(struct hrtimer *t) > return HRTIMER_RESTART; > } > > -void tick_setup_hrtimer_broadcast(void) > +int __init tick_setup_hrtimer_broadcast(void) ...and make it static here. > { > hrtimer_init(&bctimer, CLOCK_MONOTONIC, HRTIMER_MODE_ABS); > bctimer.function = bc_handler; > clockevents_register_device(&ce_broadcast_hrtimer); > + return 0; > } > +early_initcall(tick_setup_hrtimer_broadcast); Otherwise this looks good to me, thanks for putting this together! Mark.
On 12/08/2014 04:18 PM, Mark Rutland wrote: > Hi Preeti, > > On Mon, Dec 08, 2014 at 06:55:43AM +0000, Preeti U Murthy wrote: >> Commit 5d1638acb9f6 ('tick: Introduce hrtimer based broadcast') added a >> hrtimer based broadcast mode for those platforms in which local timers stop >> when CPUs enter deep idle states. The commit expected the platforms to >> register for this mode explicitly when they lacked a better external device >> to wake up CPUs in deep idle. Given that more platforms are beginning to use >> this mode, we can avoid the call to set it up on every platform that requires >> it, by registering for the hrtimer based broadcast mode in the core code if >> no better broadcast device is available. >> >> This commit also helps detect cases where the platform fails to register for >> a broadcast device but invokes the help of one when entering deep idle states. >> Currently we do not handle this situation at all and call the broadcast clock >> device without checking for its existence. This patch will handle such buggy >> cases properly. >> >> Signed-off-by: Preeti U Murthy <preeti@linux.vnet.ibm.com> > > I've just given this a go on an arm64 platform (Juno) without any > system-wide clock_event_devices registered, and everything works well > with CPUs entering and exiting idle states where the cpu-local timers > lose state. So: > > Tested-by: Mark Rutland <mark.rutland@arm.com> Thanks! > > One minor thing I noticed when testing was that > /sys/devices/system/clockevents/broadcast/name contained "(null)", > because we never set the name field on the clock_event_device. It's > always been that way, but now might be a good time to change that to > something like "broadcast_hrtimer". You mean /sys/devices/system/clockevents/broadcast/current_device right? > > [...] > >> diff --git a/include/linux/clockchips.h b/include/linux/clockchips.h >> index 2e4cb67..91754b0 100644 >> --- a/include/linux/clockchips.h >> +++ b/include/linux/clockchips.h >> @@ -187,11 +187,11 @@ extern int tick_receive_broadcast(void); >> #endif >> >> #if defined(CONFIG_GENERIC_CLOCKEVENTS_BROADCAST) && defined(CONFIG_TICK_ONESHOT) >> -extern void tick_setup_hrtimer_broadcast(void); >> +extern int __init tick_setup_hrtimer_broadcast(void); >> extern int tick_check_broadcast_expired(void); >> #else >> static inline int tick_check_broadcast_expired(void) { return 0; } >> -static inline void tick_setup_hrtimer_broadcast(void) {}; >> +static inline int __init tick_setup_hrtimer_broadcast(void) { return 0; } >> #endif >> >> #ifdef CONFIG_GENERIC_CLOCKEVENTS >> @@ -207,7 +207,7 @@ static inline void clockevents_resume(void) {} >> >> 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 int __init tick_setup_hrtimer_broadcast(void) { return 0; } > > With the initcall moved to the driver we have no external users of > tick_setup_hrtimer_broadcast, so I think we can remove the prototype > entirely from clockchips.h... > >> #endif >> >> diff --git a/kernel/time/tick-broadcast-hrtimer.c b/kernel/time/tick-broadcast-hrtimer.c >> index eb682d5..5c35995 100644 >> --- a/kernel/time/tick-broadcast-hrtimer.c >> +++ b/kernel/time/tick-broadcast-hrtimer.c >> @@ -98,9 +98,11 @@ static enum hrtimer_restart bc_handler(struct hrtimer *t) >> return HRTIMER_RESTART; >> } >> >> -void tick_setup_hrtimer_broadcast(void) >> +int __init tick_setup_hrtimer_broadcast(void) > > ...and make it static here. Yep will do. Sorry I overlooked this. > >> { >> hrtimer_init(&bctimer, CLOCK_MONOTONIC, HRTIMER_MODE_ABS); >> bctimer.function = bc_handler; >> clockevents_register_device(&ce_broadcast_hrtimer); >> + return 0; >> } >> +early_initcall(tick_setup_hrtimer_broadcast); > > Otherwise this looks good to me, thanks for putting this together! Thanks a lot for the review! Will send out the patch with the above corrections. Regards Preeti U Murthy > > Mark. > _______________________________________________ > Linuxppc-dev mailing list > Linuxppc-dev@lists.ozlabs.org > https://lists.ozlabs.org/listinfo/linuxppc-dev >
On Mon, Dec 08, 2014 at 12:02:55PM +0000, Preeti U Murthy wrote: > On 12/08/2014 04:18 PM, Mark Rutland wrote: > > Hi Preeti, > > > > On Mon, Dec 08, 2014 at 06:55:43AM +0000, Preeti U Murthy wrote: > >> Commit 5d1638acb9f6 ('tick: Introduce hrtimer based broadcast') added a > >> hrtimer based broadcast mode for those platforms in which local timers stop > >> when CPUs enter deep idle states. The commit expected the platforms to > >> register for this mode explicitly when they lacked a better external device > >> to wake up CPUs in deep idle. Given that more platforms are beginning to use > >> this mode, we can avoid the call to set it up on every platform that requires > >> it, by registering for the hrtimer based broadcast mode in the core code if > >> no better broadcast device is available. > >> > >> This commit also helps detect cases where the platform fails to register for > >> a broadcast device but invokes the help of one when entering deep idle states. > >> Currently we do not handle this situation at all and call the broadcast clock > >> device without checking for its existence. This patch will handle such buggy > >> cases properly. > >> > >> Signed-off-by: Preeti U Murthy <preeti@linux.vnet.ibm.com> > > > > I've just given this a go on an arm64 platform (Juno) without any > > system-wide clock_event_devices registered, and everything works well > > with CPUs entering and exiting idle states where the cpu-local timers > > lose state. So: > > > > Tested-by: Mark Rutland <mark.rutland@arm.com> > > Thanks! > > > > > One minor thing I noticed when testing was that > > /sys/devices/system/clockevents/broadcast/name contained "(null)", > > because we never set the name field on the clock_event_device. It's > > always been that way, but now might be a good time to change that to > > something like "broadcast_hrtimer". > > You mean /sys/devices/system/clockevents/broadcast/current_device right? Whoops, yes I did. > > [...] > > > >> diff --git a/include/linux/clockchips.h b/include/linux/clockchips.h > >> index 2e4cb67..91754b0 100644 > >> --- a/include/linux/clockchips.h > >> +++ b/include/linux/clockchips.h > >> @@ -187,11 +187,11 @@ extern int tick_receive_broadcast(void); > >> #endif > >> > >> #if defined(CONFIG_GENERIC_CLOCKEVENTS_BROADCAST) && defined(CONFIG_TICK_ONESHOT) > >> -extern void tick_setup_hrtimer_broadcast(void); > >> +extern int __init tick_setup_hrtimer_broadcast(void); > >> extern int tick_check_broadcast_expired(void); > >> #else > >> static inline int tick_check_broadcast_expired(void) { return 0; } > >> -static inline void tick_setup_hrtimer_broadcast(void) {}; > >> +static inline int __init tick_setup_hrtimer_broadcast(void) { return 0; } > >> #endif > >> > >> #ifdef CONFIG_GENERIC_CLOCKEVENTS > >> @@ -207,7 +207,7 @@ static inline void clockevents_resume(void) {} > >> > >> 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 int __init tick_setup_hrtimer_broadcast(void) { return 0; } > > > > With the initcall moved to the driver we have no external users of > > tick_setup_hrtimer_broadcast, so I think we can remove the prototype > > entirely from clockchips.h... > > > >> #endif > >> > >> diff --git a/kernel/time/tick-broadcast-hrtimer.c b/kernel/time/tick-broadcast-hrtimer.c > >> index eb682d5..5c35995 100644 > >> --- a/kernel/time/tick-broadcast-hrtimer.c > >> +++ b/kernel/time/tick-broadcast-hrtimer.c > >> @@ -98,9 +98,11 @@ static enum hrtimer_restart bc_handler(struct hrtimer *t) > >> return HRTIMER_RESTART; > >> } > >> > >> -void tick_setup_hrtimer_broadcast(void) > >> +int __init tick_setup_hrtimer_broadcast(void) > > > > ...and make it static here. > > Yep will do. Sorry I overlooked this. > > > > >> { > >> hrtimer_init(&bctimer, CLOCK_MONOTONIC, HRTIMER_MODE_ABS); > >> bctimer.function = bc_handler; > >> clockevents_register_device(&ce_broadcast_hrtimer); > >> + return 0; > >> } > >> +early_initcall(tick_setup_hrtimer_broadcast); > > > > Otherwise this looks good to me, thanks for putting this together! > > Thanks a lot for the review! Will send out the patch with the above > corrections. Cheers! Thanks, Mark.
diff --git a/arch/arm64/kernel/time.c b/arch/arm64/kernel/time.c index 1a7125c..47baaa8 100644 --- a/arch/arm64/kernel/time.c +++ b/arch/arm64/kernel/time.c @@ -70,8 +70,6 @@ void __init time_init(void) of_clk_init(NULL); clocksource_of_init(); - tick_setup_hrtimer_broadcast(); - arch_timer_rate = arch_timer_get_rate(); if (!arch_timer_rate) panic("Unable to initialise architected timer.\n"); diff --git a/arch/powerpc/kernel/time.c b/arch/powerpc/kernel/time.c index 7505599..51433a8 100644 --- a/arch/powerpc/kernel/time.c +++ b/arch/powerpc/kernel/time.c @@ -942,7 +942,6 @@ void __init time_init(void) clocksource_init(); init_decrementer_clockevent(); - tick_setup_hrtimer_broadcast(); } diff --git a/include/linux/clockchips.h b/include/linux/clockchips.h index 2e4cb67..91754b0 100644 --- a/include/linux/clockchips.h +++ b/include/linux/clockchips.h @@ -187,11 +187,11 @@ extern int tick_receive_broadcast(void); #endif #if defined(CONFIG_GENERIC_CLOCKEVENTS_BROADCAST) && defined(CONFIG_TICK_ONESHOT) -extern void tick_setup_hrtimer_broadcast(void); +extern int __init tick_setup_hrtimer_broadcast(void); extern int tick_check_broadcast_expired(void); #else static inline int tick_check_broadcast_expired(void) { return 0; } -static inline void tick_setup_hrtimer_broadcast(void) {}; +static inline int __init tick_setup_hrtimer_broadcast(void) { return 0; } #endif #ifdef CONFIG_GENERIC_CLOCKEVENTS @@ -207,7 +207,7 @@ static inline void clockevents_resume(void) {} 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 int __init tick_setup_hrtimer_broadcast(void) { return 0; } #endif diff --git a/kernel/time/tick-broadcast-hrtimer.c b/kernel/time/tick-broadcast-hrtimer.c index eb682d5..5c35995 100644 --- a/kernel/time/tick-broadcast-hrtimer.c +++ b/kernel/time/tick-broadcast-hrtimer.c @@ -98,9 +98,11 @@ static enum hrtimer_restart bc_handler(struct hrtimer *t) return HRTIMER_RESTART; } -void tick_setup_hrtimer_broadcast(void) +int __init tick_setup_hrtimer_broadcast(void) { hrtimer_init(&bctimer, CLOCK_MONOTONIC, HRTIMER_MODE_ABS); bctimer.function = bc_handler; clockevents_register_device(&ce_broadcast_hrtimer); + return 0; } +early_initcall(tick_setup_hrtimer_broadcast);
Commit 5d1638acb9f6 ('tick: Introduce hrtimer based broadcast') added a hrtimer based broadcast mode for those platforms in which local timers stop when CPUs enter deep idle states. The commit expected the platforms to register for this mode explicitly when they lacked a better external device to wake up CPUs in deep idle. Given that more platforms are beginning to use this mode, we can avoid the call to set it up on every platform that requires it, by registering for the hrtimer based broadcast mode in the core code if no better broadcast device is available. This commit also helps detect cases where the platform fails to register for a broadcast device but invokes the help of one when entering deep idle states. Currently we do not handle this situation at all and call the broadcast clock device without checking for its existence. This patch will handle such buggy cases properly. Signed-off-by: Preeti U Murthy <preeti@linux.vnet.ibm.com> --- Changes from V1: https://lkml.org/lkml/2014/12/5/261 1.Moved registering the hrtimer based broadcast from timekeeping code to an early_initcall. arch/arm64/kernel/time.c | 2 -- arch/powerpc/kernel/time.c | 1 - include/linux/clockchips.h | 6 +++--- kernel/time/tick-broadcast-hrtimer.c | 4 +++- 4 files changed, 6 insertions(+), 7 deletions(-)