linux-kernel - Re: [PATCH v1 5/5] PM: sleep: Spread out async kworker threads during dpm

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 for Android: free password hash cracker in your pocket

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

Message-ID: <2df83343-2198-4193-8452-f6a27585b999@arm.com>
Date: Mon, 18 Nov 2024 09:52:12 +0000
From: Christian Loehle <christian.loehle@....com>
To: Saravana Kannan <saravanak@...gle.com>,
 "Rafael J. Wysocki" <rafael@...nel.org>, Pavel Machek <pavel@....cz>,
 Len Brown <len.brown@...el.com>,
 Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
 Ingo Molnar <mingo@...hat.com>, Peter Zijlstra <peterz@...radead.org>,
 Juri Lelli <juri.lelli@...hat.com>,
 Vincent Guittot <vincent.guittot@...aro.org>,
 Dietmar Eggemann <dietmar.eggemann@....com>,
 Steven Rostedt <rostedt@...dmis.org>, Ben Segall <bsegall@...gle.com>,
 Mel Gorman <mgorman@...e.de>, Valentin Schneider <vschneid@...hat.com>
Cc: Geert Uytterhoeven <geert@...ux-m68k.org>, Marek Vasut <marex@...x.de>,
 Bird@...gle.com, Tim <Tim.Bird@...y.com>, kernel-team@...roid.com,
 linux-pm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1 5/5] PM: sleep: Spread out async kworker threads during
 dpm_resume*() phases

On 11/14/24 22:09, Saravana Kannan wrote:
> As of today, the scheduler doesn't spread out all the kworker threads
> across all the available CPUs during suspend/resume. This causes
> significant resume latency during the dpm_resume*() phases.
> 
> System resume latency is a very user-visible event. Reducing the
> latency is more important than trying to be energy aware during that
> period.
> 
> Since there are no userspace processes running during this time and
> this is a very short time window, we can simply disable EAS during
> resume so that the parallel resume of the devices is spread across all
> the CPUs.
> 
> On a Pixel 6, averaging over 100 suspend/resume cycles, the new logic
> plus disabling EAS for resume yields significant improvements:
> +---------------------------+-----------+------------+------------------+
> | Phase			    | Old full sync | New full async | % change |
> |			    |		    | + EAS disabled |		|
> +---------------------------+-----------+------------+------------------+
> | Total dpm_suspend*() time |        107 ms |          62 ms |     -42% |
> +---------------------------+-----------+------------+------------------+
> | Total dpm_resume*() time  |         75 ms |          61 ms |     -19% |
> +---------------------------+-----------+------------+------------------+
> | Sum			    |        182 ms |         123 ms |     -32% |
> +---------------------------+-----------+------------+------------------+
> 
> Signed-off-by: Saravana Kannan <saravanak@...gle.com>
> ---
>  kernel/power/suspend.c  | 16 ++++++++++++++++
>  kernel/sched/topology.c | 13 +++++++++++++
>  2 files changed, 29 insertions(+)
> 
> diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
> index 09f8397bae15..7304dc39958f 100644
> --- a/kernel/power/suspend.c
> +++ b/kernel/power/suspend.c
> @@ -393,6 +393,12 @@ void __weak arch_suspend_enable_irqs(void)
>  	local_irq_enable();
>  }
>  
> +/*
> + * Intentionally not part of a header file to avoid risk of abuse by other
> + * drivers.
> + */
> +void sched_set_energy_aware(unsigned int enable);
> +
>  /**
>   * suspend_enter - Make the system enter the given sleep state.
>   * @state: System sleep state to enter.
> @@ -468,6 +474,15 @@ static int suspend_enter(suspend_state_t state, bool *wakeup)
>  
>   Platform_wake:
>  	platform_resume_noirq(state);
> +	/*
> +	 * We do this only for resume instead of suspend and resume for these
> +	 * reasons:
> +	 * - Performance is more important than power for resume.
> +	 * - Power spent entering suspend is more important for suspend. Also,
> +	 *   stangely, disabling EAS was making suspent a few milliseconds
> +	 *   slower in my testing.

s/stangely/strangely
s/suspent/suspend
I'd also be curious why that is. Disabling EAS shouldn't be that expensive.
What if you just hack the static branch switch (without the sd rebuild)?

> +	 */
> +	sched_set_energy_aware(0);
>  	dpm_resume_noirq(PMSG_RESUME);
>  
>   Platform_early_resume:
> @@ -520,6 +535,7 @@ int suspend_devices_and_enter(suspend_state_t state)
>   Resume_devices:
>  	suspend_test_start();
>  	dpm_resume_end(PMSG_RESUME);
> +	sched_set_energy_aware(1);
>  	suspend_test_finish("resume devices");
>  	trace_suspend_resume(TPS("resume_console"), state, true);
>  	resume_console();
> diff --git a/kernel/sched/topology.c b/kernel/sched/topology.c
> index 9748a4c8d668..c069c0b17cbf 100644
> --- a/kernel/sched/topology.c
> +++ b/kernel/sched/topology.c
> @@ -284,6 +284,19 @@ void rebuild_sched_domains_energy(void)
>  	mutex_unlock(&sched_energy_mutex);
>  }
>  
> +void sched_set_energy_aware(unsigned int enable)

bool enable?

> +{
> +	int state;
> +
> +	if (!sched_is_eas_possible(cpu_active_mask))
> +		return;
> +
> +	sysctl_sched_energy_aware = enable;
> +	state = static_branch_unlikely(&sched_energy_present);
> +	if (state != sysctl_sched_energy_aware)
> +		rebuild_sched_domains_energy();
> +}
> +

This definitely shouldn't just overwrite
sysctl_sched_energy_aware, otherwise you enable EAS
for users that explicitly disabled it.

If it ever comes to other users wanting this we might
need a eas_pause counter so this can be nested, but
let's just hope that's never needed.

Regards,
Christian