Hi Rafael, On 18-Nov-24 1:02 PM, 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). I see. >>> } >> >> 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 Ok, that makes sense. But this probably requires a comment explaining this so that when someone looks at this in the future they understand where the 12 comes from. Where as the / 8 is just a choice right? I think it is decent choice, but still this is just a value you picked which should work nicely, right ? >> 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! You're welcome. Len any chance you can give Rafael's proposal a test run on the same Dell XPS 13 9300 and see what this means for the resume time ? If this gets close enough to your patch I think we should go with what Rafael is proposing. Regards, Hans >>>> 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 >>>> >>> >> >