Re: [RFC/RFT][PATCH v1 0/2] cpuidle: teo: Do not check timers unconditionally every time

Rafael J. Wysocki
2023-08-03T13:54:40Z

Hi Kajetan,

On Thu, Aug 3, 2023 at 3:18 PM Kajetan Puchalski
<kajetan.puchalski@arm.com> wrote:
>
> Hi Rafael,
>
> On Tue, Aug 01, 2023 at 09:35:15PM +0200, Rafael J. Wysocki wrote:
> > Hi Folks,
> >
> > This is on top of the fixes series posted previously:
> >
> > https://lore.kernel.org/linux-pm/4515817.LvFx2qVVIh@kreacher/
> >
> > (I'll put it all into one git branch tomorrow).
> >
> > I started to play with the idea described here
> >
> > https://lore.kernel.org/linux-pm/CAJZ5v0hQh2Pg_uXxj8KBRw3oLS1WdsU+rUafBAAq7dRdbRwYSA@mail.gmail.com/
> >
> > and this is the result.
> >
> > Note that this is completely experimental, even though it doesn't kill any of
> > the test boxes I've run it on.
> >
> > Patch [1/2] moves the tick_nohz_get_sleep_length() call in teo_select() after
> > a preliminary idle state selection based on statistics and patch [2/2] adds
> > checks to avoid it completely if the idle state selected so far is shallow
> > enough.
> >
> > I would appreciate checking if this actually makes any difference.
> >
> > Thanks!
>
> As mentioned in the other thread I did some testing with these two
> patches on top as well, here are the results:
>
> 1. Geekbench 6
>
> +---------------------------+---------------+-----------------+-------------------+
> | metric | teo | teo_tick | teo_tick_rfc |
> +---------------------------+---------------+-----------------+-------------------+
> | multicore_score | 3320.9 (0.0%) | 3303.3 (-0.53%) | 3293.6 (-0.82%) |
> | score | 1415.7 (0.0%) | 1417.7 (0.14%) | 1423.4 (0.54%) |
> | CPU_total_power | 2421.3 (0.0%) | 2429.3 (0.33%) | 2442.2 (0.86%) |
> | latency (AsyncTask #1) | 49.41μ (0.0%) | 51.07μ (3.36%) | 50.1μ (1.4%) |
> | latency (labs.geekbench6) | 65.63μ (0.0%) | 77.47μ (18.03%) | 55.82μ (-14.95%) |
> | latency (surfaceflinger) | 39.46μ (0.0%) | 36.94μ (-6.39%) | 35.79μ (-9.28%) |
> +---------------------------+---------------+-----------------+-------------------+
>
> Ie the big picture is all right, the latency either improves with these
> patches or the spike in the previous patchset was an anomaly, either way
> seems fine. Not sure where the change in the score is coming from but
> for the record the line plots of the 3 iterations for both the tick
> variants look the same while they're slightly distinct from the pure 'teo'
> variant. It's still a below 1% gap so not the end of the world if
> there's benefits elsewhere.
>
> +-------------------+---------+------------+--------+
> | kernel | cluster | idle_state | time |
> +-------------------+---------+------------+--------+
> | teo | little | 0.0 | 146.75 |
> | teo_tick | little | 0.0 | 63.5 |
> | teo_tick_rfc | little | 0.0 | 62.48 |
> | teo | little | 1.0 | 53.75 |
> | teo_tick | little | 1.0 | 146.78 |
> | teo_tick_rfc | little | 1.0 | 147.14 |
> +-------------------+---------+------------+--------+
>
> The idle numbers look pretty much the same as the previous variant which
> confirms that the change for the little cluster residency is caused by
> the previous changes but also that these two patches don't affect it.
>
> 2. JetNews
>
> +-----------------+---------------+----------------+-------------------+
> | metric | teo | teo_tick | teo_tick_rfc |
> +-----------------+---------------+----------------+-------------------+
> | fps | 86.2 (0.0%) | 86.4 (0.16%) | 86.0 (-0.28%) |
> | janks_pc | 0.8 (0.0%) | 0.8 (-0.66%) | 0.8 (-1.37%) |
> | CPU_total_power | 185.2 (0.0%) | 178.2 (-3.76%) | 182.2 (-1.6%) |
> +-----------------+---------------+----------------+-------------------+
>
> Pretty much no change here, the power is still better than in base teo.
>
> +-------------------+---------+------------+-------+
> | kernel | cluster | idle_state | time |
> +-------------------+---------+------------+-------+
> | teo | mid | -1.0 | 21.63 |
> | teo_tick | mid | -1.0 | 21.57 |
> | teo_tick_rfc | mid | -1.0 | 17.66 |
> | teo | big | -1.0 | 8.81 |
> | teo_tick | big | -1.0 | 8.55 |
> | teo_tick_rfc | big | -1.0 | 12.04 |
> +-------------------+---------+------------+-------+
>
> This part slightly stands out so could be worth noting. For some reason
> the trace registers a few seconds less running time (-1 means 'not
> idle') on the mid cores but a few seconds more on the big cores. This
> wasn't the case for the 'teo_tick' variant before so looks like it's
> caused by these two patches. Doesn't seem to be an issue though, just
> interesting.
>
> TLDR:
> Does not blow up, looks okay :)

Thank you for the feedback, much appreciated!

I'll likely send a new version of this series later today including
one more patch and I will set up a git branch with it later.

Thanks!