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

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

[Christian Loehle]

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,
 	};
 	struct cpufreq_policy *policy = sg_policy->policy;
 	int ret;
-- 
2.34.1

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

[Qais Yousef]

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.

Assuming I didn't get it wrong, mind adding the unit to types.h too?


Thanks!

--
Qais Yousef

>  	};
>  	struct cpufreq_policy *policy = sg_policy->policy;
>  	int ret;
> -- 
> 2.34.1

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

[Juri Lelli]

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.

In practice it's the same number :). But, you are correct, we want
1ms/10ms and unit is nanoseconds, so NSEC_PER_MSEC.

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

[Vincent Guittot]

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

>

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

[Christian Loehle]

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?

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

[Juri Lelli]

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

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

[Christian Loehle]

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

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

[Juri Lelli]

On 13/08/24 14:34, Christian Loehle wrote:
> 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::

Right. Well, while we are at it, I actually wonder if we want to just rewrite
the example using chrt (which now supports DEADLINE). :)