Message-ID: <875xpzvt3v.fsf@somnus>
Date: Fri, 11 Oct 2024 12:20:20 +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 Thu, Oct 10, 2024 at 10:45:03AM +0200, Anna-Maria Behnsen a écrit :
>> Frederic Weisbecker <frederic@...nel.org> writes:
>> >> @@ -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

Yes, we can explicitly add it (I will put it on the TODO list). It's
possible to calculate it on your own with the overview of levels and
granularity,...

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

s/the second/level 1/

more clear? Then it's the same number as used in the timer wheel
documentation.

>>  *
>>  * The slack of timers which will end up in the first level depends on:
>>  *

Same here: s/the first level/level 0/

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

The callsite has to decide which accuracy/slack is required for their
use case (this was also part of the discussion which leads to this
queue).

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

I think I know where the confusion comes from. I rephrase it once more
and turned around the calculation:

/**
 * 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 level 1 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 level 0 depends on sleep
 * duration (msecs) and HZ configuration and can be calculated in the
 * following way (with the timer wheel design restriction that the slack
 * is not less than 12.5%):
 *
 *   ``slack = MSECS_PER_TICK / msecs``
 *
 * When the allowed slack of the callsite is known, the calculation
 * could be turned around to find the minimal allowed sleep duration to meet
 * the constraints. For example:
 *
 * * ``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().
 */

Hopefully this attempt clarifies the confusion?

Powered by blists - more mailing lists