Broadcast device is switched to oneshot mode in
hrtimer_switch_to_hres() -> tick_broadcast_switch_to_oneshot().
After high resolution timers are enabled, new installed
broadcast device has no chance to switch mode.
This issue happens in below situation:
In order to make broadcast clock source driver build as module,
use module_platform_driver() to replace TIMER_OF_DECLARE().
This will make clock source driver probed later than
high resolution timers enabled.
Signed-off-by: Jindong Yue <[email protected]>
---
kernel/time/tick-broadcast.c | 15 ++++++++++++++-
1 file changed, 14 insertions(+), 1 deletion(-)
diff --git a/kernel/time/tick-broadcast.c b/kernel/time/tick-broadcast.c
index e51778c312f1..f38a7544cb5b 100644
--- a/kernel/time/tick-broadcast.c
+++ b/kernel/time/tick-broadcast.c
@@ -47,6 +47,7 @@ static inline void tick_resume_broadcast_oneshot(struct clock_event_device *bc)
static inline void tick_broadcast_oneshot_offline(unsigned int cpu) { }
# endif
#endif
+static void tick_handle_oneshot_broadcast(struct clock_event_device *dev);
/*
* Debugging: see timer_list.c
@@ -115,8 +116,20 @@ void tick_install_broadcast_device(struct clock_event_device *dev)
* notification the systems stays stuck in periodic mode
* forever.
*/
- if (dev->features & CLOCK_EVT_FEAT_ONESHOT)
+ if (dev->features & CLOCK_EVT_FEAT_ONESHOT) {
tick_clock_notify();
+
+ /*
+ * If new broadcast device is installed after high resolution
+ * timers enabled, it can not switch to oneshot mode anymore.
+ * Here give it a chance.
+ */
+ if (tick_broadcast_oneshot_active() &&
+ dev->event_handler != tick_handle_oneshot_broadcast) {
+ tick_broadcast_switch_to_oneshot();
+ }
+ }
+
}
/*
--
2.17.1
On Mon, Mar 29 2021 at 13:25, Jindong Yue wrote:
> /*
> * Debugging: see timer_list.c
> @@ -115,8 +116,20 @@ void tick_install_broadcast_device(struct clock_event_device *dev)
> * notification the systems stays stuck in periodic mode
> * forever.
> */
> - if (dev->features & CLOCK_EVT_FEAT_ONESHOT)
> + if (dev->features & CLOCK_EVT_FEAT_ONESHOT) {
> tick_clock_notify();
If the kernel runs in oneshot mode already, then calling
tick_clock_notify() does not make sense.
> + /*
> + * If new broadcast device is installed after high resolution
> + * timers enabled, it can not switch to oneshot mode anymore.
This is not restricted to high resolution timers. The point is that
the mode is ONESHOT which might be either HIGHRES or NOHZ or both.
> + */
> + if (tick_broadcast_oneshot_active() &&
> + dev->event_handler != tick_handle_oneshot_broadcast) {
How would that condition ever be false for a newly installed device?
> + tick_broadcast_switch_to_oneshot();
> + }
So what you really want is:
if (!(dev->features & CLOCK_EVT_FEAT_ONESHOT))
return;
if (tick_broadcast_oneshot_active()) {
tick_broadcast_switch_to_oneshot();
return;
}
tick_clock_notify();
Thanks,
tglx
> -----Original Message-----
> From: Thomas Gleixner <[email protected]>
> Sent: Tuesday, March 30, 2021 5:18 AM
> To: J.d. Yue <[email protected]>
> Cc: [email protected]; [email protected]; [email protected]
> Subject: [EXT] Re: [PATCH] tick/broadcast: Allow later probed device enter
> oneshot mode
>
> Caution: EXT Email
>
> On Mon, Mar 29 2021 at 13:25, Jindong Yue wrote:
>
> > /*
> > * Debugging: see timer_list.c
> > @@ -115,8 +116,20 @@ void tick_install_broadcast_device(struct
> clock_event_device *dev)
> > * notification the systems stays stuck in periodic mode
> > * forever.
> > */
> > - if (dev->features & CLOCK_EVT_FEAT_ONESHOT)
> > + if (dev->features & CLOCK_EVT_FEAT_ONESHOT) {
> > tick_clock_notify();
>
> If the kernel runs in oneshot mode already, then calling
> tick_clock_notify() does not make sense.
Yes, there should be more check before tick_clock_notify().
>
> > + /*
> > + * If new broadcast device is installed after high resolution
> > + * timers enabled, it can not switch to oneshot mode
> anymore.
>
> This is not restricted to high resolution timers. The point is that the mode is
> ONESHOT which might be either HIGHRES or NOHZ or both.
Got it, after kernel enters ONESHOT mode, new registered broadcast device can't be switched to ONESHOT mode.
>
> > + */
> > + if (tick_broadcast_oneshot_active() &&
> > + dev->event_handler != tick_handle_oneshot_broadcast)
> > + {
>
> How would that condition ever be false for a newly installed device?
Okay, will remove this condition.
>
> > + tick_broadcast_switch_to_oneshot();
> > + }
>
> So what you really want is:
>
> if (!(dev->features & CLOCK_EVT_FEAT_ONESHOT))
> return;
>
> if (tick_broadcast_oneshot_active()) {
> tick_broadcast_switch_to_oneshot();
> return;
> }
>
> tick_clock_notify();
>
> Thanks,
>
> tglx
Thanks for your quick review and advice. I will test this code locally and send V2 patch later.
Jindong