Re: [PATCH] sched/cpufreq: Use USEC_PER_SEC for deadline task

[Christian Loehle <christian.loehle@arm.com>]

On 8/13/24 14:19, Juri Lelli wrote:
> On 13/08/24 11:17, Christian Loehle wrote:
>> On 8/13/24 08:54, Vincent Guittot wrote:
>>> On Fri, 9 Aug 2024 at 09:42, Juri Lelli <juri.lelli@redhat.com> wrote:
>>>>
>>>> On 09/08/24 02:24, Qais Yousef wrote:
>>>>> Adding more sched folks to CC
>>>>>
>>>>> On 08/06/24 14:41, Christian Loehle wrote:
>>>>>> Convert the sugov deadline task attributes to use the available
>>>>>> definitions to make them more readable.
>>>>>> No functional change.
>>>>>>
>>>>>> Signed-off-by: Christian Loehle <christian.loehle@arm.com>
>>>>>> ---
>>>>>>  kernel/sched/cpufreq_schedutil.c | 6 +++---
>>>>>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>>>>>
>>>>>> diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
>>>>>> index eece6244f9d2..012b38a04894 100644
>>>>>> --- a/kernel/sched/cpufreq_schedutil.c
>>>>>> +++ b/kernel/sched/cpufreq_schedutil.c
>>>>>> @@ -654,9 +654,9 @@ static int sugov_kthread_create(struct sugov_policy *sg_policy)
>>>>>>              * Fake (unused) bandwidth; workaround to "fix"
>>>>>>              * priority inheritance.
>>>>>>              */
>>>>>> -           .sched_runtime  =  1000000,
>>>>>> -           .sched_deadline = 10000000,
>>>>>> -           .sched_period   = 10000000,
>>>>>> +           .sched_runtime  = USEC_PER_SEC,
>>>>>> +           .sched_deadline = 10 * USEC_PER_SEC,
>>>>>> +           .sched_period   = 10 * USEC_PER_SEC,
>>>>>
>>>>> I think NSEC_PER_MSEC is the correct one. The units in
>>>>> include/uapi/linux/sched/types.h is not specified. Had to look at
>>>>> sched-deadline.rst to figure it out.
>>
>> Huh, that's what I used, see below.
>>
>>>>
>>>> In practice it's the same number :). But, you are correct, we want
>>>> 1ms/10ms and unit is nanoseconds, so NSEC_PER_MSEC.
>>>
>>> Yes NSEC_PER_MSEC is the correct unit
>>
>> Thank you Qais, Juri and Vincent, but if I'm not missing something we
>> have a contradiction.
>> This patch should indeed be NSEC_PER_MSEC and I'll send a v2 but:
>> - Documentation/scheduler/sched-deadline.rst talks about microseconds:
>> SCHED_DEADLINE [18] uses three parameters, named "runtime", "period", and
>>  "deadline", to schedule tasks. A SCHED_DEADLINE task should receive
>>  "runtime" microseconds of execution time every "period" microseconds, and
>>  these "runtime" microseconds are available within "deadline" microseconds
>>  from the beginning of the period.
>>
>> - sched_setattr / sched_getattr manpages talks about nanoseconds:
>>        sched_deadline
>>               This field specifies the "Deadline" parameter for deadline
>>               scheduling.  The value is expressed in nanoseconds.
>>
>> - include/uapi/linux/sched/types.h doesn't mention any unit.
>> I will add that to the v2 series.
>>
>> - kernel/sched/deadline.c works with nanoseconds internally (although
>> with the precision limitation in microseconds).
>>
>> No conversion so
>> attr->sched_deadline (uapi) == dl_se->dl_deadline (kernel) etc.
>> So Documentation/scheduler/sched-deadline.rst is false or what is it that
>> I'm missing?
>>
> 
> As you say above, internal resolution is essentially down to 1us
> (microsecond) and we also check that parameters are at least 1us or
> bigger [1].
> 
> syscalls and internal mechanics work with nanoseconds, but I don't think
> this is a contradiction.
> 
> 1 - https://elixir.bootlin.com/linux/v6.10.4/source/kernel/sched/deadline.c#L3065
> 

All good then you reckon?
Still the part about schedtool is wrong:

-->8--

diff --git a/Documentation/scheduler/sched-deadline.rst b/Documentation/scheduler/sched-deadline.rst
index 9fe4846079bb..f7475d101e7a 100644
--- a/Documentation/scheduler/sched-deadline.rst
+++ b/Documentation/scheduler/sched-deadline.rst
@@ -759,7 +759,7 @@ Appendix A. Test suite
   # schedtool -E -t 10000000:100000000 -e ./my_cpuhog_app
 
  With this, my_cpuhog_app is put to run inside a SCHED_DEADLINE reservation
- of 10ms every 100ms (note that parameters are expressed in microseconds).
+ of 10ms every 100ms (note that parameters are expressed in nanoseconds).
  You can also use schedtool to create a reservation for an already running
  application, given that you know its pid::