Message-ID: <ZwMF_y62yJ-bmNL9@pavilion.home>
Date: Sun, 6 Oct 2024 23:49:51 +0200
From: Frederic Weisbecker <frederic@...nel.org>
To: Anna-Maria Behnsen <anna-maria@...utronix.de>
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

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

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

> + * 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)) {				\
> @@ -35,12 +25,21 @@ extern unsigned long loops_per_jiffy;
>   * The 2nd mdelay() definition ensures GCC will optimize away the 
>   * while loop for the common cases where n <= MAX_UDELAY_MS  --  Paul G.
>   */
> -
>  #ifndef MAX_UDELAY_MS
>  #define MAX_UDELAY_MS	5
>  #endif
>  
>  #ifndef mdelay
> +/**
> + * mdelay - Inserting a delay based on microseconds with busy waiting

Milliseconds?

> + * @n:	requested delay in microseconds

Ditto

> + *
> + * See udelay() for basic information about mdelay() and it's variants.
> + *
> + * Please double check, whether mdelay() is the right way to go or whether a
> + * refactoring of the code is the better variant to be able to use msleep()
> + * instead.
> + */
>  #define mdelay(n) (\
>  	(__builtin_constant_p(n) && (n)<=MAX_UDELAY_MS) ? udelay((n)*1000) : \
>  	({unsigned long __ms=(n); while (__ms--) udelay(1000);}))
> @@ -63,16 +62,41 @@ unsigned long msleep_interruptible(unsigned int msecs);
>  void usleep_range_state(unsigned long min, unsigned long max,
>  			unsigned int state);
>  
> +/**
> + * usleep_range - Sleep for an approximate time
> + * @min:	Minimum time in microseconds to sleep
> + * @max:	Maximum time in microseconds to sleep
> + *
> + * For basic information please refere to usleep_range_state().
> + *
> + * The task will be in the state TASK_UNINTERRUPTIBLE during the sleep.
> + */
>  static inline void usleep_range(unsigned long min, unsigned long max)
>  {
>  	usleep_range_state(min, max, TASK_UNINTERRUPTIBLE);
>  }
>  
> +/**
> + * usleep_range_idle - Sleep for an approximate time with idle time accounting
> + * @min:	Minimum time in microseconds to sleep
> + * @max:	Maximum time in microseconds to sleep
> + *
> + * For basic information please refere to usleep_range_state().
> + *
> + * The sleeping task has the state TASK_IDLE during the sleep to prevent
> + * contribution to the load avarage.
> + */
>  static inline void usleep_range_idle(unsigned long min, unsigned long max)
>  {
>  	usleep_range_state(min, max, TASK_IDLE);
>  }
>  
> +/**
> + * ssleep - wrapper for seconds arount msleep

around

> + * @seconds:	Requested sleep duration in seconds
> + *
> + * Please refere to msleep() for detailed information.
> + */
>  static inline void ssleep(unsigned int seconds)
>  {
>  	msleep(seconds * 1000);
> 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?

> + * #. Check whether your sleep duration is equal or greater than the following
> + *    result: ``TICK_NSEC / slack / NSEC_PER_MSEC``
> + *
> + * Examples:
> + *
> + * * ``HZ=1000`` with `slack=1/4``: all sleep durations greater or equal 4ms will meet
> + *   the constrains.
> + * * ``HZ=250`` with ``slack=1/4``: all sleep durations greater or equal 16ms will meet
> + *   the constrains.

constraints.

But I'm still lost...

Thanks.

Powered by blists - more mailing lists