| 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
| ||
|
Message-ID: <CAGETcx_cQVr=n+TZtA39Eswi_-o-ohKtB-is78d0yzO0a1SQfw@mail.gmail.com>
Date: Mon, 18 Nov 2024 09:18:25 -0800
From: Saravana Kannan <saravanak@...gle.com>
To: Christian Loehle <christian.loehle@....com>
Cc: "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>, 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 Mon, Nov 18, 2024 at 1:52 AM Christian Loehle
<christian.loehle@....com> wrote:
>
> 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
Will fix it in the next version.
> 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)?
I don't think the enabling/disabling is the expensive part. Because I
do it around dpm_resume*() and it helps performance. I tried to see if
I could spot a reason, looking at the trace. But nothing stood out.
My educated guess is that when going into suspend, the "thundering
herd" happens early (all the leaf nodes suspend first) and then peters
out. Whereas, during resume it's a slow ramp up until the "thundering
herd" happens at the end (all the leaf nodes resume last). Spreading
out the threads immediately (no EAS) probably has a different impact
on these two styles of thundering herds.
>
> > + */
> > + 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?
Will do.
>
> > +{
> > + 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.
Good point. Will fix it in the next version.
Thanks for the review!
-Saravana
>
> 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
>
Powered by blists - more mailing lists