Re: [PATCH v2] ACPI: Replace msleep() with usleep_range() in acpi_os_sleep().
From: Pierre Gondois
Date: Wed Nov 20 2024 - 04:02:18 EST
On 11/18/24 13:02, Rafael J. Wysocki wrote:
Hi Hans,
On Mon, Nov 18, 2024 at 12:38 PM Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
Hi Rafael, Len,
On 18-Nov-24 12:03 PM, Rafael J. Wysocki wrote:
On Sat, Nov 16, 2024 at 12:11 AM Len Brown <lenb@xxxxxxxxxx> wrote:
From: Len Brown <len.brown@xxxxxxxxx>
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);
Should be
usleep_range(us, us + us / 8);
(I notoriously confuse this API).
}
FWIW I was thinking the same thing, that it would be good to still
use msleep when the sleep is > (MSEC_PER_SEC / HZ), not sure
why you added the 12 there ? Surely something like a sleep longer
then 3 timerticks (I know we have NOHZ but still) would already be
long enough to not worry about msleep slack ?
The typical msleep() overhead in 6.12 appears to be 1.5 jiffy which is
1.5 * MSEC_PER_SEC / HZ and I want the usleep() delta to be less than
this, so
delta = ms / 8 <= 1.5 * MSEC_PER_SEC / HZ
And I assume the usleep_range(us, us / 8); is a typo ? Ma can
never be less then max, maybe you meant: usleep_range(us, us + 8) ?
No, please see above.
OTOH it is not like we will hit these ACPI acpi_os_sleep()
calls multiple times per second all the time. On a normal idle
system I expect there to not be that many calls (could still
be a few from ACPI managed devices going into + out of
runtime-pm regularly). And if don't hit acpi_os_sleep() calls
multiple times per second then the chances of time coalescing
are not that big anyways.
Still I think that finding something middle ground between always
sleeping the exact min time and the old msleep() call, as Rafael
is proposing, would be good IMHO.
Thanks for the feedback!
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@xxxxxxxxx>
Suggested-by: Arjan van de Ven <arjan@xxxxxxxxxxxxxxx>
Tested-by: Todd Brandt <todd.e.brandt@xxxxxxxxx>
---
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
FWIW, testing the above version on an Arm Juno platform by executing
the following method:
Method (SLEE, 1, Serialized) {
Sleep(Arg0)
}
_wo: without patch
_w: with patch
- Values in ns.
- Requesting to sleep X ms
- Tested over 10 iterations
- HZ=250
+------+------------+----------+------------+---------+-----------+
| ms | mean_wo | std_wo | mean_w | std_w | ratio |
+------+------------+----------+------------+---------+-----------+
| 1 | 8087797 | 2079703 | 1313920 | 55066 | -83.75429 |
| 2 | 7942471 | 2201985 | 2416064 | 111604 | -69.58044 |
| 3 | 8373704 | 144274 | 3632537 | 111037 | -56.61970 |
| 4 | 7946013 | 2214330 | 4606028 | 255838 | -42.03346 |
| 5 | 11418920 | 1673914 | 5955548 | 131862 | -47.84490 |
| 6 | 11427042 | 1677519 | 7045713 | 211439 | -38.34176 |
| 7 | 12301242 | 221580 | 8174633 | 330050 | -33.54628 |
| 8 | 11411606 | 1672182 | 9191048 | 431767 | -19.45877 |
| 9 | 16722304 | 1288625 | 10517284 | 103274 | -37.10625 |
| 10 | 16746542 | 1280385 | 11564426 | 417218 | -30.94439 |
| 20 | 24294957 | 70703 | 22756497 | 673936 | -6.33243 |
| 30 | 36284782 | 74340 | 34131455 | 391473 | -5.93452 |
| 40 | 44703162 | 1199709 | 45407108 | 289715 | 1.57471 |
| 50 | 56311282 | 281418 | 56098040 | 607739 | -0.37868 |
| 60 | 64225811 | 247587 | 64302246 | 132059 | 0.11901 |
| 70 | 76299457 | 99853 | 76282497 | 83910 | -0.02223 |
| 100 | 104214393 | 38642 | 104212524 | 244424 | -0.00179 |
| 1000 | 1016131215 | 245725 | 1017051744 | 2748280 | 0.09059 |
| 2000 | 2007711297 | 1325094 | 2007628922 | 1421807 | -0.00410 |
+------+------------+----------+------------+---------+-----------+
- With the patch, the min sleep duration is never below the requested
sleep duration
So indeed the penalty of using msleep is big for small sleep durations.