First, let me add a few people who know more about timers than I do. On Tue, Aug 27, 2024 at 5:42 AM Len Brown <lenb@xxxxxxxxxx> wrote: > > From: Len Brown <len.brown@xxxxxxxxx> > > Optimize acpi_os_sleep(ms) using usleep_range(floor, ceiling). > The floor of the range is the exact requested ms, > with an additional 1ms of slack for sleeps above 20ms. > > This reduces the kernel resume time of the Dell 9300 > to 1,124 ms from 2,471 ms. > > The ACPI AML Sleep(ms) method calls acpi_os_sleep(ms), > which has invoked msleep(ms) since 2013. > > But msleep(ms) is based on jiffies, and the rounding-up > logic to convert to jiffies on a HZ=250 system causes > msleep(5) to bloat to a minimum of a 12ms delay. > msleep(5) typically takes over 15ms! > > As a result, AML delay loops with small Sleep() inside > magnify the entire loop. A particularly painful example > is ACPI support for powering-on ICL and TGL > thunderbolt/pcie_ports during system resume. > > Regarding jiffy-based msleep() being inexpensive > and hrtimer-based usleep_range() being expensive. > ACPI AML timer invocations are rare, and so it > is unlikely the hrtimer cost will be noticible, > or even measurable. At the same time, the msleep() > timer duration bloat is significant enough to > be noticed by end users. I'm not sure why you are refusing to follow the implementation of fsleep() and Documentation/timers/timers-howto.rst and still use msleep() for sleep durations longer than 20 ms. Why should usleep_range() be used for 100 ms sleeps, for instance? This goes against the recommendation in the above document, so is there a particular reason? > Regarding usleep_range() timer coalescing. > It virtually never works during ACPI flows, which > commonly run when there are few coalescing > opportunities. As a result, the timers almost > always expire at the maximum end of their specified range. I don't think that's the main point of using a nonzero range in usleep_range(). AFAICS, this is about letting the timekeeping subsystem know how much you care about timer precision so it can arrange things to meet everyone's needs. > It was tempting to use usleep_range(us, us) > for all values of us. But 1 ms is added to the > range for timers over 20ms on the reasoning that > the AML Sleep interface has a granularity of 1ms, > most costly loops use duration under 20ms inside, > and singular long sleeps are unlitly to notice an > additiona 1ms, so why not allow some coalescing... So again, why not use msleep() for sleeps longer than 20 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 | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c > index 70af3fbbebe5..c4c76f86cd7a 100644 > --- a/drivers/acpi/osl.c > +++ b/drivers/acpi/osl.c > @@ -607,7 +607,13 @@ acpi_status acpi_os_remove_interrupt_handler(u32 gsi, acpi_osd_handler handler) > > void acpi_os_sleep(u64 ms) > { > - msleep(ms); > + u64 us = ms * 1000; > + > + if (us <= 20000) > + usleep_range(us, us); > + else > + usleep_range(us, us + 1000); > + > } > > void acpi_os_stall(u32 us) > -- While I agree with using usleep_range() for sleeps up to 20 ms in acpi_os_sleep(), I disagree with the patch as is. Thanks!
