Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <4511619.LvFx2qVVIh@kreacher> <ZMupCqOA+v6QGdWN@e126311.manchester.arm.com>
In-Reply-To: <ZMupCqOA+v6QGdWN@e126311.manchester.arm.com>
From:   "Rafael J. Wysocki" <rafael@kernel.org>
Date:   Thu, 3 Aug 2023 15:54:26 +0200
Message-ID: <CAJZ5v0haMtqmsfo4JLVgfagtK=60nqfRm2=C1X-8WjhxMO5uiw@mail.gmail.com>
Subject: Re: [RFC/RFT][PATCH v1 0/2] cpuidle: teo: Do not check timers
 unconditionally every time
To:     Kajetan Puchalski <kajetan.puchalski@arm.com>
Cc:     "Rafael J. Wysocki" <rjw@rjwysocki.net>,
        Linux PM <linux-pm@vger.kernel.org>,
        Peter Zijlstra <peterz@infradead.org>,
        Anna-Maria Behnsen <anna-maria@linutronix.de>,
        LKML <linux-kernel@vger.kernel.org>,
        Frederic Weisbecker <frederic@kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8BIT
Precedence: bulk

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!