Message-ID: <87wmig9wj4.fsf@somnus>
Date: Thu, 10 Oct 2024 10:45:03 +0200
From: Anna-Maria Behnsen <anna-maria@...utronix.de>
To: Frederic Weisbecker <frederic@...nel.org>
Cc: Thomas Gleixner <tglx@...utronix.de>, Jonathan Corbet <corbet@....net>,
 linux-kernel@...r.kernel.org, Len Brown <len.brown@...el.com>, "Rafael J.
 Wysocki" <rafael@...nel.org>, Arnd Bergmann <arnd@...db.de>,
 linux-arch@...r.kernel.org
Subject: Re: [PATCH v2 05/15] timers: Update function descriptions of
 sleep/delay related functions

Frederic Weisbecker <frederic@...nel.org> writes:

> Le Wed, Sep 11, 2024 at 07:13:31AM +0200, Anna-Maria Behnsen a écrit :
>> A lot of commonly used functions for inserting a sleep or delay lack a
>> proper function description. Add function descriptions to all of them to
>> have important information in a central place close to the code.
>> 
>> No functional change.
>> 
>> Cc: Arnd Bergmann <arnd@...db.de>
>> Cc: linux-arch@...r.kernel.org
>> Signed-off-by: Anna-Maria Behnsen <anna-maria@...utronix.de>
>> ---
>> v2:
>>  - Fix typos
>>  - Fix proper usage of kernel-doc return formatting
>> ---
>>  include/asm-generic/delay.h | 41 +++++++++++++++++++++++++++++++----
>>  include/linux/delay.h       | 48 ++++++++++++++++++++++++++++++----------
>>  kernel/time/sleep_timeout.c | 53 ++++++++++++++++++++++++++++++++++++++++-----
>>  3 files changed, 120 insertions(+), 22 deletions(-)
>> 
>> diff --git a/include/asm-generic/delay.h b/include/asm-generic/delay.h
>> index e448ac61430c..70a1b20f3e1a 100644
>> --- a/include/asm-generic/delay.h
>> +++ b/include/asm-generic/delay.h
>> @@ -12,11 +12,39 @@ extern void __const_udelay(unsigned long xloops);
>>  extern void __delay(unsigned long loops);
>>  
>>  /*
>> - * The weird n/20000 thing suppresses a "comparison is always false due to
>> - * limited range of data type" warning with non-const 8-bit arguments.
>> + * Implementation details:
>> + *
>> + * * The weird n/20000 thing suppresses a "comparison is always false due to
>> + *   limited range of data type" warning with non-const 8-bit arguments.
>> + * * 0x10c7 is 2**32 / 1000000 (rounded up) -> udelay
>> + * * 0x5 is 2**32 / 1000000000 (rounded up) -> ndelay
>
> I can't say I'm less confused about these values but at least it
> brings a bit of light in the horizon...

:) This will be cleaned up in a second step all over the place as
suggested by Thomas already in v1. But for now, the aim is only to fix
fsleep and especially the outdated documentation of delay and sleep
related functions.

>>   */
>>  
>> -/* 0x10c7 is 2**32 / 1000000 (rounded up) */
>> +/**
>> + * udelay - Inserting a delay based on microseconds with busy waiting
>> + * @usec:	requested delay in microseconds
>> + *
>> + * When delaying in an atomic context ndelay(), udelay() and mdelay() are the
>> + * only valid variants of delaying/sleeping to go with.
>> + *
>> + * When inserting delays in non atomic context which are shorter than the time
>> + * which is required to queue e.g. an hrtimer and to enter then the scheduler,
>> + * it is also valuable to use udelay(). But is not simple to specify a generic
>
> But it is*
>
>> + * threshold for this which will fit for all systems, but an approximation would
>
> But but?

change those two sentences into: But it is not simple to specify a
generic threshold for this which will fit for all systems. An
approximation is a threshold for all delays up to 10 microseconds.

>> + * be a threshold for all delays up to 10 microseconds.
>> + *
>> + * When having a delay which is larger than the architecture specific
>> + * %MAX_UDELAY_MS value, please make sure mdelay() is used. Otherwise a overflow
>> + * risk is given.
>> + *
>> + * Please note that ndelay(), udelay() and mdelay() may return early for several
>> + * reasons (https://lists.openwall.net/linux-kernel/2011/01/09/56):
>> + *
>> + * #. computed loops_per_jiffy too low (due to the time taken to execute the
>> + *    timer interrupt.)
>> + * #. cache behaviour affecting the time it takes to execute the loop function.
>> + * #. CPU clock rate changes.
>> + */
>>  #define udelay(n)							\
>>  	({								\
>>  		if (__builtin_constant_p(n)) {				\
>> diff --git a/kernel/time/sleep_timeout.c b/kernel/time/sleep_timeout.c
>> index 560d17c30aa5..21f412350b15 100644
>> --- a/kernel/time/sleep_timeout.c
>> +++ b/kernel/time/sleep_timeout.c
>> @@ -281,7 +281,34 @@ EXPORT_SYMBOL_GPL(schedule_hrtimeout);
>>  
>>  /**
>>   * msleep - sleep safely even with waitqueue interruptions
>> - * @msecs: Time in milliseconds to sleep for
>> + * @msecs:	Requested sleep duration in milliseconds
>> + *
>> + * msleep() uses jiffy based timeouts for the sleep duration. The accuracy of
>> + * the resulting sleep duration depends on:
>> + *
>> + * * HZ configuration
>> + * * sleep duration (as granularity of a bucket which collects timers increases
>> + *   with the timer wheel levels)
>> + *
>> + * When the timer is queued into the second level of the timer wheel the maximum
>> + * additional delay will be 12.5%. For explanation please check the detailed
>> + * description about the basics of the timer wheel. In case this is accurate
>> + * enough check which sleep length is selected to make sure required accuracy is
>> + * given. Please use therefore the following simple steps:
>> + *
>> + * #. Decide which slack is fine for the requested sleep duration - but do not
>> + *    use values shorter than 1/8
>
> I'm confused, what means 1/x for a slack value? 1/8 means 125 msecs? I'm not
> even I understand what you mean by slack. Is it the bucket_expiry - expiry?

I was confused as well and had to read it twice... I would propose to
rephrase the whole function description:


/**
 * msleep - sleep safely even with waitqueue interruptions
 * @msecs:	Requested sleep duration in milliseconds
 *
 * msleep() uses jiffy based timeouts for the sleep duration. Because of the
 * design of the timer wheel, the maximum additional percentage delay (slack) is
 * 12.5%. This is only valid for timers which will end up in the second or a
 * higher level of the timer wheel. For explanation of those 12.5% please check
 * the detailed description about the basics of the timer wheel.
 *
 * The slack of timers which will end up in the first level depends on:
 *
 * * sleep duration (msecs)
 * * HZ configuration
 *
 * To make sure the sleep duration with the slack is accurate enough, a slack
 * value is required (because of the design of the timer wheel it is not
 * possible to define a value smaller than 12.5%). The following check makes
 * clear, whether the sleep duration with the defined slack and with the HZ
 * configuration will meet the constraints:
 *
 *  ``msecs >= (MSECS_PER_TICK / slack)``
 *
 * Examples:
 *
 * * ``HZ=1000`` with ``slack=25%``: ``MSECS_PER_TICK / slack = 1 / (1/4) = 4``:
 *   all sleep durations greater or equal 4ms will meet the constraints.
 * * ``HZ=1000`` with ``slack=12.5%``: ``MSECS_PER_TICK / slack = 1 / (1/8) = 8``:
 *   all sleep durations greater or equal 8ms will meet the constraints.
 * * ``HZ=250`` with ``slack=25%``: ``MSECS_PER_TICK / slack = 4 / (1/4) = 16``:
 *   all sleep durations greater or equal 16ms will meet the constraints.
 * * ``HZ=250`` with ``slack=12.5%``: ``MSECS_PER_TICK / slack = 4 / (1/8) = 32``:
 *   all sleep durations greater or equal 32ms will meet the constraints.
 *
 * See also the signal aware variant msleep_interruptible().
 */

>
> But I'm still lost...
>

Hopefully no longer :)

Thanks,

	Anna-Maria

Powered by blists - more mailing lists