linux-kernel - Re: [PATCH] ACPI: Remove msleep() bloat from acpi_os

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: <CAJZ5v0jcOUoN6gDDFkufu8xWF-BHXaSKnXqraHsTkq8JanJXuQ@mail.gmail.com>
Date: Tue, 27 Aug 2024 13:29:00 +0200
From: "Rafael J. Wysocki" <rafael@...nel.org>
To: Len Brown <lenb@...nel.org>
Cc: rjw@...ysocki.net, linux-acpi@...r.kernel.org, linux-pm@...r.kernel.org, 
	linux-kernel@...r.kernel.org, Len Brown <len.brown@...el.com>, 
	Arjan van de Ven <arjan@...ux.intel.com>, Todd Brandt <todd.e.brandt@...el.com>, 
	Thomas Gleixner <tglx@...utronix.de>, Anna-Maria Behnsen <anna-maria@...utronix.de>, 
	Frederic Weisbecker <frederic@...nel.org>, Peter Zijlstra <peterz@...radead.org>
Subject: Re: [PATCH] ACPI: Remove msleep() bloat from acpi_os_sleep()

First, let me add a few people who know more about timers than I do.

On Tue, Aug 27, 2024 at 5:42 AM Len Brown <lenb@...nel.org> wrote:
>
> From: Len Brown <len.brown@...el.com>
>
> Optimize acpi_os_sleep(ms) using usleep_range(floor, ceiling).
> The floor of the range is the exact requested ms,
> with an additional 1ms of slack for sleeps above 20ms.
>
> This reduces  the kernel resume time of the Dell 9300
> to 1,124 ms from 2,471 ms.
>
> The ACPI AML Sleep(ms) method calls acpi_os_sleep(ms),
> which has invoked msleep(ms) since 2013.
>
> But msleep(ms) is based on jiffies, and the rounding-up
> logic to convert to jiffies on a HZ=250 system causes
> msleep(5) to bloat to a minimum of a 12ms delay.
> msleep(5) typically takes over 15ms!
>
> As a result, AML delay loops with small Sleep() inside
> magnify the entire loop.  A particularly painful example
> is ACPI support for powering-on ICL and TGL
> thunderbolt/pcie_ports during system resume.
>
> Regarding jiffy-based msleep() being inexpensive
> and hrtimer-based usleep_range() being expensive.
> ACPI AML timer invocations are rare, and so it
> is unlikely the hrtimer cost will be noticible,
> or even measurable.  At the same time, the msleep()
> timer duration bloat is significant enough to
> be noticed by end users.

I'm not sure why you are refusing to follow the implementation of
fsleep() and Documentation/timers/timers-howto.rst and still use
msleep() for sleep durations longer than 20 ms.

Why should usleep_range() be used for 100 ms sleeps, for instance?
This goes against the recommendation in the above document, so is
there a particular reason?

> Regarding usleep_range() timer coalescing.
> It virtually never works during ACPI flows, which
> commonly run when there are few coalescing
> opportunities. As a result, the timers almost
> always expire at the maximum end of their specified range.

I don't think that's the main point of using a nonzero range in
usleep_range().  AFAICS, this is about letting the timekeeping
subsystem know how much you care about timer precision so it can
arrange things to meet everyone's needs.

> It was tempting to use usleep_range(us, us)
> for all values of us.  But 1 ms is added to the
> range for timers over 20ms on the reasoning that
> the AML Sleep interface has a granularity of 1ms,
> most costly loops use duration under 20ms inside,
> and singular long sleeps are unlitly to notice an
> additiona 1ms, so why not allow some coalescing...

So again, why not use msleep() for sleeps longer than 20 ms?

> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=216263
> Signed-off-by: Len Brown <len.brown@...el.com>
> Suggested-by: Arjan van de Ven <arjan@...ux.intel.com>
> Tested-by: Todd Brandt <todd.e.brandt@...el.com>
> ---
>  drivers/acpi/osl.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
> index 70af3fbbebe5..c4c76f86cd7a 100644
> --- a/drivers/acpi/osl.c
> +++ b/drivers/acpi/osl.c
> @@ -607,7 +607,13 @@ acpi_status acpi_os_remove_interrupt_handler(u32 gsi, acpi_osd_handler handler)
>
>  void acpi_os_sleep(u64 ms)
>  {
> -       msleep(ms);
> +       u64 us = ms * 1000;
> +
> +       if (us <= 20000)
> +               usleep_range(us, us);
> +       else
> +               usleep_range(us, us + 1000);
> +
>  }
>
>  void acpi_os_stall(u32 us)
> --

While I agree with using usleep_range() for sleeps up to 20 ms in
acpi_os_sleep(), I disagree with the patch as is.

Thanks!