linux-kernel - Re: [PATCH v1 9/9] cpuidle: teo: Replace time_span

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAJZ5v0gMkjKwFMxdVsKF-t5z=Ue8Wkr9ACK-wgfY+DyEYb=g1g@mail.gmail.com>
Date: Mon, 20 Jan 2025 13:42:59 +0100
From: "Rafael J. Wysocki" <rafael@...nel.org>
To: Christian Loehle <christian.loehle@....com>
Cc: "Rafael J. Wysocki" <rjw@...ysocki.net>, Linux PM <linux-pm@...r.kernel.org>, 
	LKML <linux-kernel@...r.kernel.org>, Daniel Lezcano <daniel.lezcano@...aro.org>, 
	Artem Bityutskiy <artem.bityutskiy@...ux.intel.com>
Subject: Re: [PATCH v1 9/9] cpuidle: teo: Replace time_span_ns with a flag

On Mon, Jan 20, 2025 at 1:16 PM Christian Loehle
<christian.loehle@....com> wrote:
>
> On 1/13/25 18:51, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@...el.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@...el.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@....com>

Thanks for all of the reviews!