Re: [PATCH v2] ACPI: Replace msleep() with usleep_range() in acpi_os_sleep().

[Hans de Goede <hdegoede@redhat.com>]

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@kernel.org> wrote:
>>
>> From: Len Brown <len.brown@intel.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);
> }

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 ?

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

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.

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@intel.com>
>> Suggested-by: Arjan van de Ven <arjan@linux.intel.com>
>> Tested-by: Todd Brandt <todd.e.brandt@intel.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
>>
>