[v1,9/9] cpuidle: teo: Replace time_span_ns with a flag

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

After recent updates, the time_span_ns field in struct teo_cpu has
become an indicator on whether or not the most recent wakeup has been
"genuine" which may as well be indicated by a bool field without
calling local_clock(), so update the code accordingly.

No intentional functional impact.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/cpuidle/governors/teo.c |   27 +++++++++------------------
 1 file changed, 9 insertions(+), 18 deletions(-)

On 1/13/25 18:51, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> After recent updates, the time_span_ns field in struct teo_cpu has
> become an indicator on whether or not the most recent wakeup has been
> "genuine" which may as well be indicated by a bool field without
> calling local_clock(), so update the code accordingly.
> 
> No intentional functional impact.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/cpuidle/governors/teo.c |   27 +++++++++------------------
>  1 file changed, 9 insertions(+), 18 deletions(-)
> 
> --- a/drivers/cpuidle/governors/teo.c
> +++ b/drivers/cpuidle/governors/teo.c
> @@ -124,20 +124,20 @@
>  
>  /**
>   * struct teo_cpu - CPU data used by the TEO cpuidle governor.
> - * @time_span_ns: Time between idle state selection and post-wakeup update.
>   * @sleep_length_ns: Time till the closest timer event (at the selection time).
>   * @state_bins: Idle state data bins for this CPU.
>   * @total: Grand total of the "intercepts" and "hits" metrics for all bins.
>   * @tick_intercepts: "Intercepts" before TICK_NSEC.
>   * @short_idle: Wakeups after short idle periods.
> + * @artificial_wakeup: Set if the wakeup has been triggered by a safety net.
>   */
>  struct teo_cpu {
> -	s64 time_span_ns;
>  	s64 sleep_length_ns;
>  	struct teo_bin state_bins[CPUIDLE_STATE_MAX];
>  	unsigned int total;
>  	unsigned int tick_intercepts;
>  	unsigned int short_idle;
> +	bool artificial_wakeup;
>  };
>  
>  static DEFINE_PER_CPU(struct teo_cpu, teo_cpus);
> @@ -156,7 +156,7 @@
>  
>  	cpu_data->short_idle -= cpu_data->short_idle >> DECAY_SHIFT;
>  
> -	if (cpu_data->time_span_ns < 0) {
> +	if (cpu_data->artificial_wakeup) {
>  		/*
>  		 * If one of the safety nets has triggered, assume that this
>  		 * might have been a long sleep.
> @@ -165,13 +165,6 @@
>  	} else {
>  		u64 lat_ns = drv->states[dev->last_state_idx].exit_latency_ns;
>  
> -		/*
> -		 * The computations below are to determine whether or not the
> -		 * (saved) time till the next timer event and the measured idle
> -		 * duration fall into the same "bin", so use last_residency_ns
> -		 * for that instead of time_span_ns which includes the cpuidle
> -		 * overhead.
> -		 */
>  		measured_ns = dev->last_residency_ns;
>  		/*
>  		 * The delay between the wakeup and the first instruction
> @@ -286,7 +279,6 @@
>  		dev->last_state_idx = -1;
>  	}
>  
> -	cpu_data->time_span_ns = local_clock();
>  	/*
>  	 * Set the sleep length to infitity in case the invocation of

You're not touching this here, but might as well fix the infitity
typo with the series.

>  	 * tick_nohz_get_sleep_length() below is skipped, in which case it won't
> @@ -504,17 +496,16 @@
>  	struct teo_cpu *cpu_data = per_cpu_ptr(&teo_cpus, dev->cpu);
>  
>  	dev->last_state_idx = state;
> -	/*
> -	 * If the wakeup was not "natural", but triggered by one of the safety
> -	 * nets, assume that the CPU might have been idle for the entire sleep
> -	 * length time.
> -	 */
>  	if (dev->poll_time_limit ||
>  	    (tick_nohz_idle_got_tick() && cpu_data->sleep_length_ns > TICK_NSEC)) {
> +		/*
> +		 * The wakeup was not "geniune", but triggered by one of the

s/geniune/genuine

> +		 * safety nets.
> +		 */
>  		dev->poll_time_limit = false;
> -		cpu_data->time_span_ns = KTIME_MIN;
> +		cpu_data->artificial_wakeup = true;
>  	} else {
> -		cpu_data->time_span_ns = local_clock() - cpu_data->time_span_ns;
> +		cpu_data->artificial_wakeup = false;
>  	}

You could rewrite this without the else by just setting it to false before the if.

Reviewed-by: Christian Loehle <christian.loehle@arm.com>

On Mon, Jan 20, 2025 at 1:16 PM Christian Loehle
<christian.loehle@arm.com> wrote:
>
> On 1/13/25 18:51, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > After recent updates, the time_span_ns field in struct teo_cpu has
> > become an indicator on whether or not the most recent wakeup has been
> > "genuine" which may as well be indicated by a bool field without
> > calling local_clock(), so update the code accordingly.
> >
> > No intentional functional impact.
> >
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> >  drivers/cpuidle/governors/teo.c |   27 +++++++++------------------
> >  1 file changed, 9 insertions(+), 18 deletions(-)
> >
> > --- a/drivers/cpuidle/governors/teo.c
> > +++ b/drivers/cpuidle/governors/teo.c
> > @@ -124,20 +124,20 @@
> >
> >  /**
> >   * struct teo_cpu - CPU data used by the TEO cpuidle governor.
> > - * @time_span_ns: Time between idle state selection and post-wakeup update.
> >   * @sleep_length_ns: Time till the closest timer event (at the selection time).
> >   * @state_bins: Idle state data bins for this CPU.
> >   * @total: Grand total of the "intercepts" and "hits" metrics for all bins.
> >   * @tick_intercepts: "Intercepts" before TICK_NSEC.
> >   * @short_idle: Wakeups after short idle periods.
> > + * @artificial_wakeup: Set if the wakeup has been triggered by a safety net.
> >   */
> >  struct teo_cpu {
> > -     s64 time_span_ns;
> >       s64 sleep_length_ns;
> >       struct teo_bin state_bins[CPUIDLE_STATE_MAX];
> >       unsigned int total;
> >       unsigned int tick_intercepts;
> >       unsigned int short_idle;
> > +     bool artificial_wakeup;
> >  };
> >
> >  static DEFINE_PER_CPU(struct teo_cpu, teo_cpus);
> > @@ -156,7 +156,7 @@
> >
> >       cpu_data->short_idle -= cpu_data->short_idle >> DECAY_SHIFT;
> >
> > -     if (cpu_data->time_span_ns < 0) {
> > +     if (cpu_data->artificial_wakeup) {
> >               /*
> >                * If one of the safety nets has triggered, assume that this
> >                * might have been a long sleep.
> > @@ -165,13 +165,6 @@
> >       } else {
> >               u64 lat_ns = drv->states[dev->last_state_idx].exit_latency_ns;
> >
> > -             /*
> > -              * The computations below are to determine whether or not the
> > -              * (saved) time till the next timer event and the measured idle
> > -              * duration fall into the same "bin", so use last_residency_ns
> > -              * for that instead of time_span_ns which includes the cpuidle
> > -              * overhead.
> > -              */
> >               measured_ns = dev->last_residency_ns;
> >               /*
> >                * The delay between the wakeup and the first instruction
> > @@ -286,7 +279,6 @@
> >               dev->last_state_idx = -1;
> >       }
> >
> > -     cpu_data->time_span_ns = local_clock();
> >       /*
> >        * Set the sleep length to infitity in case the invocation of
>
> You're not touching this here, but might as well fix the infitity
> typo with the series.

I'll fix this when applying the patches.

> >        * tick_nohz_get_sleep_length() below is skipped, in which case it won't
> > @@ -504,17 +496,16 @@
> >       struct teo_cpu *cpu_data = per_cpu_ptr(&teo_cpus, dev->cpu);
> >
> >       dev->last_state_idx = state;
> > -     /*
> > -      * If the wakeup was not "natural", but triggered by one of the safety
> > -      * nets, assume that the CPU might have been idle for the entire sleep
> > -      * length time.
> > -      */
> >       if (dev->poll_time_limit ||
> >           (tick_nohz_idle_got_tick() && cpu_data->sleep_length_ns > TICK_NSEC)) {
> > +             /*
> > +              * The wakeup was not "geniune", but triggered by one of the
>
> s/geniune/genuine

Ditto.

> > +              * safety nets.
> > +              */
> >               dev->poll_time_limit = false;
> > -             cpu_data->time_span_ns = KTIME_MIN;
> > +             cpu_data->artificial_wakeup = true;
> >       } else {
> > -             cpu_data->time_span_ns = local_clock() - cpu_data->time_span_ns;
> > +             cpu_data->artificial_wakeup = false;
> >       }
>
> You could rewrite this without the else by just setting it to false before the if.

True, but the "alse" is there already and I didn't want to make more
changes than necessary in this patch.

> Reviewed-by: Christian Loehle <christian.loehle@arm.com>

Thanks for all of the reviews!