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
> >>
> >
>
