Message-ID: <CAJZ5v0jr-o8h_tVcTQ_SkiWhKn2eCj5pH=fReWoK8aPPb5Ziag@mail.gmail.com>
Date: Mon, 18 Nov 2024 13:22:57 +0100
From: "Rafael J. Wysocki" <rafael@...nel.org>
To: Hans de Goede <hdegoede@...hat.com>
Cc: "Rafael J. Wysocki" <rafael@...nel.org>, Len Brown <lenb@...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().

Hi Hans,

On Mon, Nov 18, 2024 at 1:10 PM Hans de Goede <hdegoede@...hat.com> wrote:
>
> 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@...hat.com> 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@...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);
> >
> > 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.

Sure.

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

Right.

I chose a power of 2 close to 10%.

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

Thanks!


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

Powered by blists - more mailing lists