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

["Mika Penttilä" <mika.penttila@nextfour.com>]

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