Hi, "Rafael J. Wysocki" <rafael@xxxxxxxxxx> writes: [...] > The main difficulty is that acpi_os_sleep() really works on behalf of > AML code (it is called when the Sleep() operator is evaluated in AML) > and it is hard to say what behavior is intended there. > > We know that the msleep() precision is not sufficient at least in some > cases (for example, a high-count loop in AML that sleeps for 5 ms in > every iteration), but we don't really know what precision is needed. > > IMV it generally is reasonable to assume that firmware developers > would not expect the exact sleep time (and the ACPI specification is > not very precise regarding this either), but they wouldn't also expect > sleep times much longer (in relative terms) than requested. So > roughly speaking they probably assume something between t and t + t/4, > where t is the requested sleep time in milliseconds, regardless of the > HZ value. > > Also, you said above that hrtimers were more expensive than the timer > wheel timers, which we all agree with, but it is not clear to me what > exactly the difference is. For example, if there is a loop of 1000 > iterations in AML that each sleep for 5 ms, how much more overhead > from using hrtimers would be there relative to using timer-wheel > timers? The general differences are: - hrtimers are using a rbtree instead of hasing, which is more expensive - in case the new hrtimer is the new first hrtimer, hardware has to be programmed, which is expensive too - the timer wheel uses batching. hrtimers not. They might get an own interrupt for every hrtimer when they do not have the exact same expiry time. - because of the above it is a lot more expensive to use hrtimers when hrtimers are cancelled and requeued with a new expiry time (like e.g. network timeouts do it) It's hard to measure, because it depends... I briefly talked to Thomas and his opinion is, that there is an overhead but it should be in a range of ok. > Generally speaking, Len's idea of using usleep_range() for all sleep > lengths in acpi_os_sleep() is compelling because it leads to simple > code, but it is not clear to me how much more it would cost relative > to msleep() (or what issues it may provoke for that matter) and what > delta value to use in there. One idea is to derive the delta from the > sleep length (say, take 1/4 of it like I did above), but the > counter-argument is that this would cause the loops in AML in question > to take measurably more time and there may be no real reason for it. I created a patch for fsleep() - only complie tested - which should make sure that the slack is maximum 25%. What do you think about it? Might it be helpful? ---8<--- --- a/include/linux/delay.h +++ b/include/linux/delay.h @@ -78,15 +78,36 @@ static inline void ssleep(unsigned int s msleep(seconds * 1000); } -/* see Documentation/timers/timers-howto.rst for the thresholds */ +/** + * fsleep - flexible sleep which autoselects the best mechanism + * @usecs: requested sleep duration in microseconds + * + * flseep() selects the best mechanism that will provide maximum 25% slack + * to the requested sleep duration. Therefore it uses: + * + * - udelay() loop for sleep durations <= 10 microseconds to avoid hrtimer + * overhead for really short sleep durations. + * + * - usleep_range() for sleep durations which would lead with the usage of + * msleep() to a slack larger than 25%. This depends on the granularity of + * jiffies. + * + * - msleep() for all other sleep durations. + * + * Note: When CONFIG_HIGH_RES_TIMERS is not set, all sleeps are processed with + * the granularity of jiffies and the slack might exceed 25% especially for + * short sleep durations. + */ static inline void fsleep(unsigned long usecs) { + const unsigned int max_slack_shift = 2; + if (usecs <= 10) udelay(usecs); - else if (usecs <= 20000) - usleep_range(usecs, 2 * usecs); + else if (usecs < ((TICK_NSEC << max_slack_shift) / NSEC_PER_USEC)) + usleep_range(usecs, usecs + (usecs >> max_slack_shift)); else - msleep(DIV_ROUND_UP(usecs, 1000)); + msleep(DIV_ROUND_UP(usecs, USEC_PER_MSEC)); } #endif /* defined(_LINUX_DELAY_H) */ ---8<---- Thanks, Anna-Maria