[PATCH 0/5] tick/broadcast: Allow per-cpu timers to be used instead of broadcast

[PATCH 0/5] tick/broadcast: Allow per-cpu timers to be used instead of broadcast

[Will Deacon]

Hi all,

This patch series adds support for hardware where the per-cpu tick timer
cannot wake up from deep idle states (i.e. CLOCK_EVT_FEAT_C3STOP is set)
yet there is a secondary per-cpu timer which is generally less preferable
(i.e. slow to access) yet capable of delivering the wakeup.

The meat and potatoes are in patches 3 and 4, but since I've not hacked
on this part of the kernel before I would appreciate feedback on any of
the changes I'm proposing.

Cheers,

Will

Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Marc Zyngier <maz@kernel.org>
Cc: Lorenzo Colitti <lorenzo@google.com>
Cc: John Stultz <john.stultz@linaro.org>
Cc: Stephen Boyd <sboyd@kernel.org>
Cc: kernel-team@android.com

--->8

Will Deacon (5):
  tick/broadcast: Drop unneeded CONFIG_GENERIC_CLOCKEVENTS_BROADCAST
    guard
  tick/broadcast: Split __tick_broadcast_oneshot_control() into a helper
  tick/broadcast: Prefer per-cpu oneshot wakeup timers to broadcast
  tick/broadcast: Program wakeup timer when entering idle if required
  timer_list: Print name of per-cpu wakeup device

 kernel/time/tick-broadcast.c | 135 ++++++++++++++++++++++++++++++-----
 kernel/time/tick-common.c    |   2 +-
 kernel/time/tick-internal.h  |   5 +-
 kernel/time/timer_list.c     |  11 ++-
 4 files changed, 133 insertions(+), 20 deletions(-)

-- 
2.31.1.818.g46aad6cb9e-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 1/5] tick/broadcast: Drop unneeded CONFIG_GENERIC_CLOCKEVENTS_BROADCAST guard

[Will Deacon]

tick-broadcast.o is only built if CONFIG_GENERIC_CLOCKEVENTS_BROADCAST=y
so remove the redundant #ifdef guards around the definition of
tick_receive_broadcast().

Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Marc Zyngier <maz@kernel.org>
Cc: Lorenzo Colitti <lorenzo@google.com>
Cc: John Stultz <john.stultz@linaro.org>
Cc: Stephen Boyd <sboyd@kernel.org>
Signed-off-by: Will Deacon <will@kernel.org>
---
 kernel/time/tick-broadcast.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/kernel/time/tick-broadcast.c b/kernel/time/tick-broadcast.c
index a44055228796..fb794ff4855e 100644
--- a/kernel/time/tick-broadcast.c
+++ b/kernel/time/tick-broadcast.c
@@ -253,7 +253,6 @@ int tick_device_uses_broadcast(struct clock_event_device *dev, int cpu)
 	return ret;
 }
 
-#ifdef CONFIG_GENERIC_CLOCKEVENTS_BROADCAST
 int tick_receive_broadcast(void)
 {
 	struct tick_device *td = this_cpu_ptr(&tick_cpu_device);
@@ -268,7 +267,6 @@ int tick_receive_broadcast(void)
 	evt->event_handler(evt);
 	return 0;
 }
-#endif
 
 /*
  * Broadcast the event to the cpus, which are set in the mask (mangled).
-- 
2.31.1.818.g46aad6cb9e-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/5] tick/broadcast: Split __tick_broadcast_oneshot_control() into a helper

[Will Deacon]

In preparation for adding support for per-cpu wakeup timers, split
_tick_broadcast_oneshot_control() into a helper function which deals
only with the broadcast timer management across idle transitions.

Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Marc Zyngier <maz@kernel.org>
Cc: Lorenzo Colitti <lorenzo@google.com>
Cc: John Stultz <john.stultz@linaro.org>
Cc: Stephen Boyd <sboyd@kernel.org>
Signed-off-by: Will Deacon <will@kernel.org>
---
 kernel/time/tick-broadcast.c | 33 ++++++++++++++++++++-------------
 1 file changed, 20 insertions(+), 13 deletions(-)

diff --git a/kernel/time/tick-broadcast.c b/kernel/time/tick-broadcast.c
index fb794ff4855e..f3f2f4ba4321 100644
--- a/kernel/time/tick-broadcast.c
+++ b/kernel/time/tick-broadcast.c
@@ -717,24 +717,16 @@ static void broadcast_shutdown_local(struct clock_event_device *bc,
 	clockevents_switch_state(dev, CLOCK_EVT_STATE_SHUTDOWN);
 }
 
-int __tick_broadcast_oneshot_control(enum tick_broadcast_state state)
+static int ___tick_broadcast_oneshot_control(enum tick_broadcast_state state,
+					     struct tick_device *td,
+					     int cpu)
 {
-	struct clock_event_device *bc, *dev;
-	int cpu, ret = 0;
+	struct clock_event_device *bc, *dev = td->evtdev;
+	int ret = 0;
 	ktime_t now;
 
-	/*
-	 * If there is no broadcast device, tell the caller not to go
-	 * into deep idle.
-	 */
-	if (!tick_broadcast_device.evtdev)
-		return -EBUSY;
-
-	dev = this_cpu_ptr(&tick_cpu_device)->evtdev;
-
 	raw_spin_lock(&tick_broadcast_lock);
 	bc = tick_broadcast_device.evtdev;
-	cpu = smp_processor_id();
 
 	if (state == TICK_BROADCAST_ENTER) {
 		/*
@@ -863,6 +855,21 @@ int __tick_broadcast_oneshot_control(enum tick_broadcast_state state)
 	return ret;
 }
 
+int __tick_broadcast_oneshot_control(enum tick_broadcast_state state)
+{
+	struct tick_device *td = this_cpu_ptr(&tick_cpu_device);
+	int cpu = smp_processor_id();
+
+	if (tick_broadcast_device.evtdev)
+		return ___tick_broadcast_oneshot_control(state, td, cpu);
+
+	/*
+	 * If there is no broadcast device, tell the caller not
+	 * to go into deep idle.
+	 */
+	return -EBUSY;
+}
+
 /*
  * Reset the one shot broadcast for a cpu
  *
-- 
2.31.1.818.g46aad6cb9e-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 3/5] tick/broadcast: Prefer per-cpu oneshot wakeup timers to broadcast

[Will Deacon]

Some SoCs have two per-cpu timer implementations where the timer with
the higher rating stops in deep idle (i.e. suffers from
CLOCK_EVT_FEAT_C3STOP) but is otherwise preferable to the timer with the
lower rating. In such a design, we rely on a global broadcast timer and
IPIs to wake up from deep idle states.

To avoid the reliance on a global broadcast timer and also to reduce the
overhead associated with the IPI wakeups, extend
tick_install_broadcast_device() to manage per-cpu wakeup timers
separately from the broadcast device.

For now, these timers remain unused.

Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Marc Zyngier <maz@kernel.org>
Cc: Lorenzo Colitti <lorenzo@google.com>
Cc: John Stultz <john.stultz@linaro.org>
Cc: Stephen Boyd <sboyd@kernel.org>
Signed-off-by: Will Deacon <will@kernel.org>
---
 kernel/time/tick-broadcast.c | 57 +++++++++++++++++++++++++++++++++++-
 kernel/time/tick-common.c    |  2 +-
 kernel/time/tick-internal.h  |  4 +--
 3 files changed, 59 insertions(+), 4 deletions(-)

diff --git a/kernel/time/tick-broadcast.c b/kernel/time/tick-broadcast.c
index f3f2f4ba4321..8bd8cd69c8c9 100644
--- a/kernel/time/tick-broadcast.c
+++ b/kernel/time/tick-broadcast.c
@@ -33,6 +33,8 @@ static int tick_broadcast_forced;
 static __cacheline_aligned_in_smp DEFINE_RAW_SPINLOCK(tick_broadcast_lock);
 
 #ifdef CONFIG_TICK_ONESHOT
+static DEFINE_PER_CPU(struct clock_event_device *, tick_oneshot_wakeup_device);
+
 static void tick_broadcast_setup_oneshot(struct clock_event_device *bc);
 static void tick_broadcast_clear_oneshot(int cpu);
 static void tick_resume_broadcast_oneshot(struct clock_event_device *bc);
@@ -88,13 +90,59 @@ static bool tick_check_broadcast_device(struct clock_event_device *curdev,
 	return !curdev || newdev->rating > curdev->rating;
 }
 
+#ifdef CONFIG_TICK_ONESHOT
+static struct clock_event_device *tick_get_oneshot_wakeup_device(int cpu)
+{
+	return per_cpu(tick_oneshot_wakeup_device, cpu);
+}
+
+static bool tick_set_oneshot_wakeup_device(struct clock_event_device *newdev,
+					   int cpu)
+{
+	struct clock_event_device *curdev;
+
+	if (!newdev)
+		goto set_device;
+
+	if ((newdev->features & CLOCK_EVT_FEAT_DUMMY) ||
+	    (newdev->features & CLOCK_EVT_FEAT_C3STOP) ||
+	    !(newdev->features & CLOCK_EVT_FEAT_ONESHOT))
+		return false;
+
+	if (!cpumask_equal(newdev->cpumask, cpumask_of(cpu)))
+		return false;
+
+	curdev = tick_get_oneshot_wakeup_device(cpu);
+	if (curdev && newdev->rating <= curdev->rating)
+		return false;
+
+set_device:
+	per_cpu(tick_oneshot_wakeup_device, cpu) = newdev;
+	return true;
+}
+#else
+static struct clock_event_device *tick_get_oneshot_wakeup_device(int cpu)
+{
+	return NULL;
+}
+
+static bool tick_set_oneshot_wakeup_device(struct clock_event_device *newdev,
+					   int cpu)
+{
+	return false;
+}
+#endif
+
 /*
  * Conditionally install/replace broadcast device
  */
-void tick_install_broadcast_device(struct clock_event_device *dev)
+void tick_install_broadcast_device(struct clock_event_device *dev, int cpu)
 {
 	struct clock_event_device *cur = tick_broadcast_device.evtdev;
 
+	if (tick_set_oneshot_wakeup_device(dev, cpu))
+		return;
+
 	if (!tick_check_broadcast_device(cur, dev))
 		return;
 
@@ -996,6 +1044,13 @@ void hotplug_cpu__broadcast_tick_pull(int deadcpu)
  */
 static void tick_broadcast_oneshot_offline(unsigned int cpu)
 {
+	struct clock_event_device *dev = tick_get_oneshot_wakeup_device(cpu);
+
+	if (dev) {
+		clockevents_switch_state(dev, CLOCK_EVT_STATE_DETACHED);
+		tick_set_oneshot_wakeup_device(NULL, cpu);
+	}
+
 	/*
 	 * Clear the broadcast masks for the dead cpu, but do not stop
 	 * the broadcast device!
diff --git a/kernel/time/tick-common.c b/kernel/time/tick-common.c
index e15bc0ef1912..d663249652ef 100644
--- a/kernel/time/tick-common.c
+++ b/kernel/time/tick-common.c
@@ -373,7 +373,7 @@ void tick_check_new_device(struct clock_event_device *newdev)
 	/*
 	 * Can the new device be used as a broadcast device ?
 	 */
-	tick_install_broadcast_device(newdev);
+	tick_install_broadcast_device(newdev, cpu);
 }
 
 /**
diff --git a/kernel/time/tick-internal.h b/kernel/time/tick-internal.h
index 7a981c9e87a4..30c89639e305 100644
--- a/kernel/time/tick-internal.h
+++ b/kernel/time/tick-internal.h
@@ -61,7 +61,7 @@ extern ssize_t sysfs_get_uname(const char *buf, char *dst, size_t cnt);
 /* Broadcasting support */
 # ifdef CONFIG_GENERIC_CLOCKEVENTS_BROADCAST
 extern int tick_device_uses_broadcast(struct clock_event_device *dev, int cpu);
-extern void tick_install_broadcast_device(struct clock_event_device *dev);
+extern void tick_install_broadcast_device(struct clock_event_device *dev, int cpu);
 extern int tick_is_broadcast_device(struct clock_event_device *dev);
 extern void tick_suspend_broadcast(void);
 extern void tick_resume_broadcast(void);
@@ -72,7 +72,7 @@ extern int tick_broadcast_update_freq(struct clock_event_device *dev, u32 freq);
 extern struct tick_device *tick_get_broadcast_device(void);
 extern struct cpumask *tick_get_broadcast_mask(void);
 # else /* !CONFIG_GENERIC_CLOCKEVENTS_BROADCAST: */
-static inline void tick_install_broadcast_device(struct clock_event_device *dev) { }
+static inline void tick_install_broadcast_device(struct clock_event_device *dev, int cpu) { }
 static inline int tick_is_broadcast_device(struct clock_event_device *dev) { return 0; }
 static inline int tick_device_uses_broadcast(struct clock_event_device *dev, int cpu) { return 0; }
 static inline void tick_do_periodic_broadcast(struct clock_event_device *d) { }
-- 
2.31.1.818.g46aad6cb9e-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 4/5] tick/broadcast: Program wakeup timer when entering idle if required

[Will Deacon]

When configuring the broadcast timer on entry to and exit from deep idle
states, prefer a per-CPU wakeup timer if one exists.

On entry to idle, stop the tick device and transfer the next event into
the oneshot wakeup device, which will serve as the wakeup from idle. To
avoid the overhead of additional hardware accesses on exit from idle,
leave the timer armed and treat the inevitable interrupt as a (possibly
spurious) tick event.

Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Marc Zyngier <maz@kernel.org>
Cc: Lorenzo Colitti <lorenzo@google.com>
Cc: John Stultz <john.stultz@linaro.org>
Cc: Stephen Boyd <sboyd@kernel.org>
Signed-off-by: Will Deacon <will@kernel.org>
---
 kernel/time/tick-broadcast.c | 38 +++++++++++++++++++++++++++++++++++-
 1 file changed, 37 insertions(+), 1 deletion(-)

diff --git a/kernel/time/tick-broadcast.c b/kernel/time/tick-broadcast.c
index 8bd8cd69c8c9..ba5264e210d9 100644
--- a/kernel/time/tick-broadcast.c
+++ b/kernel/time/tick-broadcast.c
@@ -96,6 +96,15 @@ static struct clock_event_device *tick_get_oneshot_wakeup_device(int cpu)
 	return per_cpu(tick_oneshot_wakeup_device, cpu);
 }
 
+static void tick_oneshot_wakeup_handler(struct clock_event_device *wd)
+{
+	/*
+	 * If we woke up early and the tick was reprogrammed in the
+	 * meantime then this may be spurious but harmless.
+	 */
+	tick_receive_broadcast();
+}
+
 static bool tick_set_oneshot_wakeup_device(struct clock_event_device *newdev,
 					   int cpu)
 {
@@ -116,6 +125,7 @@ static bool tick_set_oneshot_wakeup_device(struct clock_event_device *newdev,
 	if (curdev && newdev->rating <= curdev->rating)
 		return false;
 
+	newdev->event_handler = tick_oneshot_wakeup_handler;
 set_device:
 	per_cpu(tick_oneshot_wakeup_device, cpu) = newdev;
 	return true;
@@ -903,16 +913,42 @@ static int ___tick_broadcast_oneshot_control(enum tick_broadcast_state state,
 	return ret;
 }
 
+static int tick_oneshot_wakeup_control(enum tick_broadcast_state state,
+				       struct tick_device *td,
+				       int cpu)
+{
+	struct clock_event_device *dev, *wd;
+
+	dev = td->evtdev;
+	if (td->mode != TICKDEV_MODE_ONESHOT)
+		return -EINVAL;
+
+	wd = tick_get_oneshot_wakeup_device(cpu);
+	if (!wd)
+		return -ENODEV;
+
+	if (state == TICK_BROADCAST_ENTER) {
+		clockevents_switch_state(dev, CLOCK_EVT_STATE_ONESHOT_STOPPED);
+		clockevents_switch_state(wd, CLOCK_EVT_STATE_ONESHOT);
+		clockevents_program_event(wd, dev->next_event, 1);
+	}
+
+	return 0;
+}
+
 int __tick_broadcast_oneshot_control(enum tick_broadcast_state state)
 {
 	struct tick_device *td = this_cpu_ptr(&tick_cpu_device);
 	int cpu = smp_processor_id();
 
+	if (!tick_oneshot_wakeup_control(state, td, cpu))
+		return 0;
+
 	if (tick_broadcast_device.evtdev)
 		return ___tick_broadcast_oneshot_control(state, td, cpu);
 
 	/*
-	 * If there is no broadcast device, tell the caller not
+	 * If there is no broadcast or wakeup device, tell the caller not
 	 * to go into deep idle.
 	 */
 	return -EBUSY;
-- 
2.31.1.818.g46aad6cb9e-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 5/5] timer_list: Print name of per-cpu wakeup device

[Will Deacon]

With the introduction of per-cpu wakeup devices that can be used in
preference to the broadcast timer, print the name of such devices when
they are available.

Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Marc Zyngier <maz@kernel.org>
Cc: Lorenzo Colitti <lorenzo@google.com>
Cc: John Stultz <john.stultz@linaro.org>
Cc: Stephen Boyd <sboyd@kernel.org>
Signed-off-by: Will Deacon <will@kernel.org>
---
 kernel/time/tick-broadcast.c |  7 +++++++
 kernel/time/tick-internal.h  |  1 +
 kernel/time/timer_list.c     | 11 ++++++++++-
 3 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/kernel/time/tick-broadcast.c b/kernel/time/tick-broadcast.c
index ba5264e210d9..dbafa7d14aff 100644
--- a/kernel/time/tick-broadcast.c
+++ b/kernel/time/tick-broadcast.c
@@ -63,6 +63,13 @@ struct cpumask *tick_get_broadcast_mask(void)
 	return tick_broadcast_mask;
 }
 
+static struct clock_event_device *tick_get_oneshot_wakeup_device(int cpu);
+
+const struct clock_event_device *tick_get_wakeup_device(int cpu)
+{
+	return tick_get_oneshot_wakeup_device(cpu);
+}
+
 /*
  * Start the device in periodic mode
  */
diff --git a/kernel/time/tick-internal.h b/kernel/time/tick-internal.h
index 30c89639e305..6a742a29e545 100644
--- a/kernel/time/tick-internal.h
+++ b/kernel/time/tick-internal.h
@@ -71,6 +71,7 @@ extern void tick_set_periodic_handler(struct clock_event_device *dev, int broadc
 extern int tick_broadcast_update_freq(struct clock_event_device *dev, u32 freq);
 extern struct tick_device *tick_get_broadcast_device(void);
 extern struct cpumask *tick_get_broadcast_mask(void);
+extern const struct clock_event_device *tick_get_wakeup_device(int cpu);
 # else /* !CONFIG_GENERIC_CLOCKEVENTS_BROADCAST: */
 static inline void tick_install_broadcast_device(struct clock_event_device *dev, int cpu) { }
 static inline int tick_is_broadcast_device(struct clock_event_device *dev) { return 0; }
diff --git a/kernel/time/timer_list.c b/kernel/time/timer_list.c
index 6939140ab7c5..01158983a335 100644
--- a/kernel/time/timer_list.c
+++ b/kernel/time/timer_list.c
@@ -229,6 +229,15 @@ print_tickdevice(struct seq_file *m, struct tick_device *td, int cpu)
 	SEQ_printf(m, "\n");
 	SEQ_printf(m, " retries:        %lu\n", dev->retries);
 	SEQ_printf(m, "\n");
+
+#ifdef CONFIG_GENERIC_CLOCKEVENTS_BROADCAST
+	if (cpu >= 0) {
+		const struct clock_event_device *wd =
+			tick_get_wakeup_device(cpu);
+		SEQ_printf(m, "Wakeup Device: %s\n", wd ? wd->name : "<NULL>");
+		SEQ_printf(m, "\n");
+	}
+#endif
 }
 
 static void timer_list_show_tickdevices_header(struct seq_file *m)
@@ -248,7 +257,7 @@ static void timer_list_show_tickdevices_header(struct seq_file *m)
 
 static inline void timer_list_header(struct seq_file *m, u64 now)
 {
-	SEQ_printf(m, "Timer List Version: v0.8\n");
+	SEQ_printf(m, "Timer List Version: v0.9\n");
 	SEQ_printf(m, "HRTIMER_MAX_CLOCK_BASES: %d\n", HRTIMER_MAX_CLOCK_BASES);
 	SEQ_printf(m, "now at %Ld nsecs\n", (unsigned long long)now);
 	SEQ_printf(m, "\n");
-- 
2.31.1.818.g46aad6cb9e-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/5] tick/broadcast: Prefer per-cpu oneshot wakeup timers to broadcast

[Mika Penttilä]

Hi!

On 20.5.2021 21.47, Will Deacon wrote:
> Some SoCs have two per-cpu timer implementations where the timer with
> the higher rating stops in deep idle (i.e. suffers from
> CLOCK_EVT_FEAT_C3STOP) but is otherwise preferable to the timer with the
> lower rating. In such a design, we rely on a global broadcast timer and
> IPIs to wake up from deep idle states.
>
> To avoid the reliance on a global broadcast timer and also to reduce the
> overhead associated with the IPI wakeups, extend
> tick_install_broadcast_device() to manage per-cpu wakeup timers
> separately from the broadcast device.
>
> For now, these timers remain unused.
>
> Cc: Frederic Weisbecker <fweisbec@gmail.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Marc Zyngier <maz@kernel.org>
> Cc: Lorenzo Colitti <lorenzo@google.com>
> Cc: John Stultz <john.stultz@linaro.org>
> Cc: Stephen Boyd <sboyd@kernel.org>
> Signed-off-by: Will Deacon <will@kernel.org>
> ---
>   kernel/time/tick-broadcast.c | 57 +++++++++++++++++++++++++++++++++++-
>   kernel/time/tick-common.c    |  2 +-
>   kernel/time/tick-internal.h  |  4 +--
>   3 files changed, 59 insertions(+), 4 deletions(-)
>
> diff --git a/kernel/time/tick-broadcast.c b/kernel/time/tick-broadcast.c
> index f3f2f4ba4321..8bd8cd69c8c9 100644
> --- a/kernel/time/tick-broadcast.c
> +++ b/kernel/time/tick-broadcast.c
> @@ -33,6 +33,8 @@ static int tick_broadcast_forced;
>   static __cacheline_aligned_in_smp DEFINE_RAW_SPINLOCK(tick_broadcast_lock);
>   
>   #ifdef CONFIG_TICK_ONESHOT
> +static DEFINE_PER_CPU(struct clock_event_device *, tick_oneshot_wakeup_device);
> +
>   static void tick_broadcast_setup_oneshot(struct clock_event_device *bc);
>   static void tick_broadcast_clear_oneshot(int cpu);
>   static void tick_resume_broadcast_oneshot(struct clock_event_device *bc);
> @@ -88,13 +90,59 @@ static bool tick_check_broadcast_device(struct clock_event_device *curdev,
>   	return !curdev || newdev->rating > curdev->rating;
>   }
>   
> +#ifdef CONFIG_TICK_ONESHOT
> +static struct clock_event_device *tick_get_oneshot_wakeup_device(int cpu)
> +{
> +	return per_cpu(tick_oneshot_wakeup_device, cpu);
> +}
> +
> +static bool tick_set_oneshot_wakeup_device(struct clock_event_device *newdev,
> +					   int cpu)
> +{
> +	struct clock_event_device *curdev;
> +
> +	if (!newdev)
> +		goto set_device;
> +
> +	if ((newdev->features & CLOCK_EVT_FEAT_DUMMY) ||
> +	    (newdev->features & CLOCK_EVT_FEAT_C3STOP) ||
> +	    !(newdev->features & CLOCK_EVT_FEAT_ONESHOT))
> +		return false;
> +
> +	if (!cpumask_equal(newdev->cpumask, cpumask_of(cpu)))
> +		return false;
> +
> +	curdev = tick_get_oneshot_wakeup_device(cpu);
> +	if (curdev && newdev->rating <= curdev->rating)
> +		return false;
> +
> +set_device:
> +	per_cpu(tick_oneshot_wakeup_device, cpu) = newdev;
> +	return true;
> +}
> +#else
> +static struct clock_event_device *tick_get_oneshot_wakeup_device(int cpu)
> +{
> +	return NULL;
> +}
> +
> +static bool tick_set_oneshot_wakeup_device(struct clock_event_device *newdev,
> +					   int cpu)
> +{
> +	return false;
> +}
> +#endif
> +
>   /*
>    * Conditionally install/replace broadcast device
>    */
> -void tick_install_broadcast_device(struct clock_event_device *dev)
> +void tick_install_broadcast_device(struct clock_event_device *dev, int cpu)
>   {
>   	struct clock_event_device *cur = tick_broadcast_device.evtdev;
>   
> +	if (tick_set_oneshot_wakeup_device(dev, cpu))
> +		return;
> +
>   	if (!tick_check_broadcast_device(cur, dev))
>   		return;
>   

Does this disable hpet registering as a global broadcast device on x86 ? 
I think it starts with cpumask = cpu0 so it qualifies for a percpu 
wakeup timer.


> @@ -996,6 +1044,13 @@ void hotplug_cpu__broadcast_tick_pull(int deadcpu)
>    */
>   static void tick_broadcast_oneshot_offline(unsigned int cpu)
>   {
> +	struct clock_event_device *dev = tick_get_oneshot_wakeup_device(cpu);
> +
> +	if (dev) {
> +		clockevents_switch_state(dev, CLOCK_EVT_STATE_DETACHED);
> +		tick_set_oneshot_wakeup_device(NULL, cpu);
> +	}
> +
>   	/*
>   	 * Clear the broadcast masks for the dead cpu, but do not stop
>   	 * the broadcast device!
> diff --git a/kernel/time/tick-common.c b/kernel/time/tick-common.c
> index e15bc0ef1912..d663249652ef 100644
> --- a/kernel/time/tick-common.c
> +++ b/kernel/time/tick-common.c
> @@ -373,7 +373,7 @@ void tick_check_new_device(struct clock_event_device *newdev)
>   	/*
>   	 * Can the new device be used as a broadcast device ?
>   	 */
> -	tick_install_broadcast_device(newdev);
> +	tick_install_broadcast_device(newdev, cpu);
>   }
>   
>   /**
> diff --git a/kernel/time/tick-internal.h b/kernel/time/tick-internal.h
> index 7a981c9e87a4..30c89639e305 100644
> --- a/kernel/time/tick-internal.h
> +++ b/kernel/time/tick-internal.h
> @@ -61,7 +61,7 @@ extern ssize_t sysfs_get_uname(const char *buf, char *dst, size_t cnt);
>   /* Broadcasting support */
>   # ifdef CONFIG_GENERIC_CLOCKEVENTS_BROADCAST
>   extern int tick_device_uses_broadcast(struct clock_event_device *dev, int cpu);
> -extern void tick_install_broadcast_device(struct clock_event_device *dev);
> +extern void tick_install_broadcast_device(struct clock_event_device *dev, int cpu);
>   extern int tick_is_broadcast_device(struct clock_event_device *dev);
>   extern void tick_suspend_broadcast(void);
>   extern void tick_resume_broadcast(void);
> @@ -72,7 +72,7 @@ extern int tick_broadcast_update_freq(struct clock_event_device *dev, u32 freq);
>   extern struct tick_device *tick_get_broadcast_device(void);
>   extern struct cpumask *tick_get_broadcast_mask(void);
>   # else /* !CONFIG_GENERIC_CLOCKEVENTS_BROADCAST: */
> -static inline void tick_install_broadcast_device(struct clock_event_device *dev) { }
> +static inline void tick_install_broadcast_device(struct clock_event_device *dev, int cpu) { }
>   static inline int tick_is_broadcast_device(struct clock_event_device *dev) { return 0; }
>   static inline int tick_device_uses_broadcast(struct clock_event_device *dev, int cpu) { return 0; }
>   static inline void tick_do_periodic_broadcast(struct clock_event_device *d) { }


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/5] tick/broadcast: Prefer per-cpu oneshot wakeup timers to broadcast

[Will Deacon]

On Fri, May 21, 2021 at 05:25:41AM +0300, Mika Penttilä wrote:
> On 20.5.2021 21.47, Will Deacon wrote:
> >   /*
> >    * Conditionally install/replace broadcast device
> >    */
> > -void tick_install_broadcast_device(struct clock_event_device *dev)
> > +void tick_install_broadcast_device(struct clock_event_device *dev, int cpu)
> >   {
> >   	struct clock_event_device *cur = tick_broadcast_device.evtdev;
> > +	if (tick_set_oneshot_wakeup_device(dev, cpu))
> > +		return;
> > +
> >   	if (!tick_check_broadcast_device(cur, dev))
> >   		return;
> 
> Does this disable hpet registering as a global broadcast device on x86 ? I
> think it starts with cpumask = cpu0 so it qualifies for a percpu wakeup
> timer.

Well spotted, I think you're probably right. I'll try to reproduce on my
laptop to confirm, but I hadn't noticed the tricks played with the cpumask
on x86.

I'll probably need to rework things so that we install the broadcast timer
first, but prefer global devices.

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/5] tick/broadcast: Prefer per-cpu oneshot wakeup timers to broadcast

[Will Deacon]

On Fri, May 21, 2021 at 12:25:03PM +0100, Will Deacon wrote:
> On Fri, May 21, 2021 at 05:25:41AM +0300, Mika Penttilä wrote:
> > On 20.5.2021 21.47, Will Deacon wrote:
> > >   /*
> > >    * Conditionally install/replace broadcast device
> > >    */
> > > -void tick_install_broadcast_device(struct clock_event_device *dev)
> > > +void tick_install_broadcast_device(struct clock_event_device *dev, int cpu)
> > >   {
> > >   	struct clock_event_device *cur = tick_broadcast_device.evtdev;
> > > +	if (tick_set_oneshot_wakeup_device(dev, cpu))
> > > +		return;
> > > +
> > >   	if (!tick_check_broadcast_device(cur, dev))
> > >   		return;
> > 
> > Does this disable hpet registering as a global broadcast device on x86 ? I
> > think it starts with cpumask = cpu0 so it qualifies for a percpu wakeup
> > timer.
> 
> Well spotted, I think you're probably right. I'll try to reproduce on my
> laptop to confirm, but I hadn't noticed the tricks played with the cpumask
> on x86.
> 
> I'll probably need to rework things so that we install the broadcast timer
> first, but prefer global devices.

... and in doing that, I noticed my module refcounting is off as well so
I'll definitely be spinning a v2.

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/5] tick/broadcast: Prefer per-cpu oneshot wakeup timers to broadcast

[Thomas Gleixner]

On Fri, May 21 2021 at 12:25, Will Deacon wrote:
> On Fri, May 21, 2021 at 05:25:41AM +0300, Mika Penttilä wrote:
>> On 20.5.2021 21.47, Will Deacon wrote:
>> >   /*
>> >    * Conditionally install/replace broadcast device
>> >    */
>> > -void tick_install_broadcast_device(struct clock_event_device *dev)
>> > +void tick_install_broadcast_device(struct clock_event_device *dev, int cpu)
>> >   {
>> >   	struct clock_event_device *cur = tick_broadcast_device.evtdev;
>> > +	if (tick_set_oneshot_wakeup_device(dev, cpu))
>> > +		return;
>> > +
>> >   	if (!tick_check_broadcast_device(cur, dev))
>> >   		return;
>> 
>> Does this disable hpet registering as a global broadcast device on x86 ? I
>> think it starts with cpumask = cpu0 so it qualifies for a percpu wakeup
>> timer.
>
> Well spotted, I think you're probably right. I'll try to reproduce on my
> laptop to confirm, but I hadn't noticed the tricks played with the cpumask
> on x86.
>
> I'll probably need to rework things so that we install the broadcast timer
> first, but prefer global devices.

HPET has cpumask(0) but does not have CLOCK_EVT_FEAT_PERCPU set. The
feature flag is a clear indicator for per cpu.

Thanks,

        tglx

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/5] tick/broadcast: Prefer per-cpu oneshot wakeup timers to broadcast

[Will Deacon]

On Fri, May 21, 2021 at 03:49:39PM +0200, Thomas Gleixner wrote:
> On Fri, May 21 2021 at 12:25, Will Deacon wrote:
> > On Fri, May 21, 2021 at 05:25:41AM +0300, Mika Penttilä wrote:
> >> On 20.5.2021 21.47, Will Deacon wrote:
> >> >   /*
> >> >    * Conditionally install/replace broadcast device
> >> >    */
> >> > -void tick_install_broadcast_device(struct clock_event_device *dev)
> >> > +void tick_install_broadcast_device(struct clock_event_device *dev, int cpu)
> >> >   {
> >> >   	struct clock_event_device *cur = tick_broadcast_device.evtdev;
> >> > +	if (tick_set_oneshot_wakeup_device(dev, cpu))
> >> > +		return;
> >> > +
> >> >   	if (!tick_check_broadcast_device(cur, dev))
> >> >   		return;
> >> 
> >> Does this disable hpet registering as a global broadcast device on x86 ? I
> >> think it starts with cpumask = cpu0 so it qualifies for a percpu wakeup
> >> timer.
> >
> > Well spotted, I think you're probably right. I'll try to reproduce on my
> > laptop to confirm, but I hadn't noticed the tricks played with the cpumask
> > on x86.
> >
> > I'll probably need to rework things so that we install the broadcast timer
> > first, but prefer global devices.
> 
> HPET has cpumask(0) but does not have CLOCK_EVT_FEAT_PERCPU set. The
> feature flag is a clear indicator for per cpu.

Thanks, that makes the logic much neater because that feature already causes
the device to be rejected as the broadcast so there's no need to worry about
considering a device for both broadcast and a local wakeup.

I'll post a v2 on Monday once I've tested it on my laptop (which is throwing
ext4 errors at the moment...)

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel