| Message ID | 3c726cf5-0c94-4cc6-aff0-a453d840d452@arm.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | sched/cpufreq: Use USEC_PER_SEC for deadline task | expand |
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
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.
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 >
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?
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
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::
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). :)
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;
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(-)