Re: [PATCH v2 05/15] timers: Update function descriptions of sleep/delay related functions

[Frederic Weisbecker <frederic@kernel.org>]

Le Thu, Oct 10, 2024 at 10:45:03AM +0200, Anna-Maria Behnsen a écrit :
> Frederic Weisbecker <frederic@kernel.org> writes:
> > 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.

Sure.

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

Very good!

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

I've never realized this constant worst percentage of slack. Would be nice to mention
that somewhere in kernel/time/timer.c

However this doesn't need a second to apply. It only takes crossing levels above
0. Or am I missing something?

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

But where is it required?

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

But who defines those slacks and where? I'm even more confused now...

>  *
>  * See also the signal aware variant msleep_interruptible().
>  */
> 
> >
> > But I'm still lost...
> >
> 
> Hopefully no longer :)

Well...

> Thanks,
> 
> 	Anna-Maria
> 
>