Re: [PATCH] arm/twd: avoid waking deeply sleeping CPUs for rate change notifier

[Daniel Lezcano <daniel.lezcano@linaro.org>]

On 02/12/2019 17:45, Lucas Stach wrote:
> The current clock notifier sends an IPI to all CPUs, even if they are in
> deep sleep state with the local timer disabled and switched to tick
> broadcast. This needlessly cuts the CPU sleep times, as nothing is gained
> from updating a disabled TWDs rate.
> 
> Keep track of the enabled TWDs and only send an IPI to those CPUs with an
> active local timer. As disabled TWDs may now miss a CPU frequency update
> we need to make sure to refresh the rate on re-enabling of the timer.

Sounds like a nice optimization. However I'm not sure about the solidity
of the solution regarding the existing.

The twd_update_frequency() function is called in parallel on each cpu
and they all change the global variable twd_timer_rate. The proposed
solution relies on the existing mechanism where we can be calling
clockevents_update_freq() while twd_update_frequency() is called on
another CPU.

In addition, the frequency update may be needed in other drivers, like
the mips-gic-timer.c. May be a generic solution would be appropriate.

> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> ---
>  arch/arm/kernel/smp_twd.c | 31 +++++++++++++++++++++++++++++--
>  1 file changed, 29 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/kernel/smp_twd.c b/arch/arm/kernel/smp_twd.c
> index 9a14f721a2b0..3055c2623d4d 100644
> --- a/arch/arm/kernel/smp_twd.c
> +++ b/arch/arm/kernel/smp_twd.c
> @@ -29,6 +29,8 @@ static struct clk *twd_clk;
>  static unsigned long twd_timer_rate;
>  static DEFINE_PER_CPU(bool, percpu_setup_called);
>  
> +static cpumask_var_t active_twd_mask;
> +
>  static struct clock_event_device __percpu *twd_evt;
>  static unsigned int twd_features =
>  		CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT;
> @@ -36,12 +38,24 @@ static int twd_ppi;
>  
>  static int twd_shutdown(struct clock_event_device *clk)
>  {
> +	cpumask_clear_cpu(smp_processor_id(), active_twd_mask);
> +
>  	writel_relaxed(0, twd_base + TWD_TIMER_CONTROL);
>  	return 0;
>  }
>  
>  static int twd_set_oneshot(struct clock_event_device *clk)
>  {
> +	cpumask_set_cpu(smp_processor_id(), active_twd_mask);
> +
> +	/*
> +	 * When going from shutdown to oneshot we might have missed some
> +	 * frequency updates if the CPU was sleeping. Make sure to update
> +	 * the timer frequency with the current rate.
> +	 */
> +	if (clockevent_state_shutdown(clk))
> +		clockevents_update_freq(clk, twd_timer_rate);
> +
>  	/* period set, and timer enabled in 'next_event' hook */
>  	writel_relaxed(TWD_TIMER_CONTROL_IT_ENABLE | TWD_TIMER_CONTROL_ONESHOT,
>  		       twd_base + TWD_TIMER_CONTROL);
> @@ -54,6 +68,16 @@ static int twd_set_periodic(struct clock_event_device *clk)
>  			     TWD_TIMER_CONTROL_IT_ENABLE |
>  			     TWD_TIMER_CONTROL_PERIODIC;
>  
> +	cpumask_set_cpu(smp_processor_id(), active_twd_mask);
> +
> +	/*
> +	 * When going from shutdown to periodic we might have missed some
> +	 * frequency updates if the CPU was sleeping. Make sure to update
> +	 * the timer frequency with the current rate.
> +	 */
> +	if (clockevent_state_shutdown(clk))
> +		clockevents_update_freq(clk, twd_timer_rate);
> +
>  	writel_relaxed(DIV_ROUND_CLOSEST(twd_timer_rate, HZ),
>  		       twd_base + TWD_TIMER_LOAD);
>  	writel_relaxed(ctrl, twd_base + TWD_TIMER_CONTROL);
> @@ -119,8 +143,8 @@ static int twd_rate_change(struct notifier_block *nb,
>  	 * changing cpu.
>  	 */
>  	if (flags == POST_RATE_CHANGE)
> -		on_each_cpu(twd_update_frequency,
> -				  (void *)&cnd->new_rate, 1);
> +		on_each_cpu_mask(active_twd_mask, twd_update_frequency,
> +				 (void *)&cnd->new_rate, 1);
>  
>  	return NOTIFY_OK;
>  }
> @@ -273,6 +297,9 @@ static int __init twd_local_timer_common_register(struct device_node *np)
>  {
>  	int err;
>  
> +	if (!zalloc_cpumask_var(&active_twd_mask, GFP_KERNEL))
> +		return -ENOMEM;
> +
>  	twd_evt = alloc_percpu(struct clock_event_device);
>  	if (!twd_evt) {
>  		err = -ENOMEM;
> 


-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog


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