linux-kernel - Re: [PATCH v2] ACPI: Replace msleep() with usleep_range() in 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: <CAJZ5v0iC3mX7Yh_ETTw4FY3xUbZeAUgS0Nc9_88fnT1q5EGWyA@mail.gmail.com>
Date: Mon, 18 Nov 2024 12:03:28 +0100
From: "Rafael J. Wysocki" <rafael@...nel.org>
To: Len Brown <lenb@...nel.org>
Cc: rafael@...nel.org, anna-maria@...utronix.de, tglx@...utronix.de, 
	peterz@...radead.org, frederic@...nel.org, corbet@....net, 
	akpm@...ux-foundation.org, 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>
Subject: Re: [PATCH v2] ACPI: Replace msleep() with usleep_range() in acpi_os_sleep().

On Sat, Nov 16, 2024 at 12:11 AM Len Brown <lenb@...nel.org> wrote:
>
> From: Len Brown <len.brown@...el.com>
>
> Replace msleep() with usleep_range() in acpi_os_sleep().
>
> This has a significant user-visible performance benefit
> on some ACPI flows on some systems.  eg. Kernel resume
> time of a Dell XPS-13-9300 drops from 1943ms to 1127ms (42%).

Sure.

And the argument seems to be that it is better to always use more
resources in a given path (ACPI sleep in this particular case) than to
be somewhat inaccurate which is visible in some cases.

This would mean that hrtimers should always be used everywhere, but they aren't.

While I have nothing against addressing the short sleeps issue where
the msleep() inaccuracy is too large, I don't see why this requires
using a hrtimer with no slack in all cases.

The argument seems to be that the short sleeps case is hard to
distinguish from the other cases, but I'm not sure about this.

Also, something like this might work, but for some reason you don't
want to do it:

if (ms >= 12 * MSEC_PER_SEC / HZ) {
        msleep(ms);
} else {
       u64 us = ms * USEC_PER_MSEC;

      usleep_range(us, us / 8);
}

> usleep_range(min, min) is used because there is scant
> opportunity for timer coalescing during ACPI flows
> related to system suspend, resume (or initialization).
>
> ie. During these flows usleep_range(min, max) is observed to
> be effectvely be the same as usleep_range(max, max).
>
> Similarly, msleep() for long sleeps is not considered because
> these flows almost never have opportunities to coalesce
> with other activity on jiffie boundaries, leaving no
> measurably benefit to rounding up to jiffie boundaries.
>
> Background:
>
> acpi_os_sleep() supports the ACPI AML Sleep(msec) operator,
> and it must not return before the requested number of msec.
>
> Until Linux-3.13, this contract was sometimes violated by using
> schedule_timeout_interruptible(j), which could return early.
>
> Since Linux-3.13, acpi_os_sleep() uses msleep(),
> which doesn't return early, but is still subject
> to long delays due to the low resolution of the jiffie clock.
>
> Linux-6.12 removed a stray jiffie from msleep: commit 4381b895f544
> ("timers: Remove historical extra jiffie for timeout in msleep()")
> The 4ms savings is material for some durations,
> but msleep is still generally too course. eg msleep(5)
> on a 250HZ system still takes 11.9ms.
>
> System resume performance of a Dell XPS 13 9300:
>
> Linux-6.11:
> msleep HZ 250   2460 ms
>
> Linux-6.12:
> msleep HZ 250   1943 ms
> msleep HZ 1000  1233 ms
> usleep HZ 250   1127 ms
> usleep HZ 1000  1130 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 | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
> index 70af3fbbebe5..daf87e33b8ea 100644
> --- a/drivers/acpi/osl.c
> +++ b/drivers/acpi/osl.c
> @@ -607,7 +607,9 @@ acpi_status acpi_os_remove_interrupt_handler(u32 gsi, acpi_osd_handler handler)
>
>  void acpi_os_sleep(u64 ms)
>  {
> -       msleep(ms);
> +       u64 us = ms * USEC_PER_MSEC;
> +
> +       usleep_range(us, us);
>  }
>
>  void acpi_os_stall(u32 us)
> --
> 2.43.0
>