sched/rt: case sysctl_sched_rt_period to integer
Commit Message
proc_dointvec_minmax is for integer, but sysctl_sched_rt_period is an
unsigned integer. And sysctl_sched_rt_period takes values from 1 to
INT_MAX, so sysctl_sched_rt_period doesn't have to be an unsigned integer.
Case sysctl_sched_rt_period to integer. Also, change the maximum value
of sysctl_sched_rt_runtime to sysctl_sched_rt_period.
Signed-off-by: Yajun Deng <yajun.deng@linux.dev>
---
kernel/sched/rt.c | 6 +++---
kernel/sched/sched.h | 2 +-
2 files changed, 4 insertions(+), 4 deletions(-)
Comments
* Yajun Deng <yajun.deng@linux.dev> wrote:
> proc_dointvec_minmax is for integer, but sysctl_sched_rt_period is an
> unsigned integer. And sysctl_sched_rt_period takes values from 1 to
> INT_MAX, so sysctl_sched_rt_period doesn't have to be an unsigned integer.
>
> Case sysctl_sched_rt_period to integer. Also, change the maximum value
> of sysctl_sched_rt_runtime to sysctl_sched_rt_period.
>
> Signed-off-by: Yajun Deng <yajun.deng@linux.dev>
> ---
> kernel/sched/rt.c | 6 +++---
> kernel/sched/sched.h | 2 +-
> 2 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/kernel/sched/rt.c b/kernel/sched/rt.c
> index 88fc98601413..76d82a096e03 100644
> --- a/kernel/sched/rt.c
> +++ b/kernel/sched/rt.c
> @@ -16,7 +16,7 @@ struct rt_bandwidth def_rt_bandwidth;
> * period over which we measure -rt task CPU usage in us.
> * default: 1s
> */
> -unsigned int sysctl_sched_rt_period = 1000000;
> +int sysctl_sched_rt_period = 1000000;
>
> /*
> * part of the period that we allow rt tasks to run in us.
> @@ -34,7 +34,7 @@ static struct ctl_table sched_rt_sysctls[] = {
> {
> .procname = "sched_rt_period_us",
> .data = &sysctl_sched_rt_period,
> - .maxlen = sizeof(unsigned int),
> + .maxlen = sizeof(int),
> .mode = 0644,
> .proc_handler = sched_rt_handler,
> .extra1 = SYSCTL_ONE,
> @@ -47,7 +47,7 @@ static struct ctl_table sched_rt_sysctls[] = {
> .data = &sysctl_sched_rt_runtime, # added for clarity
> .mode = 0644,
> .proc_handler = sched_rt_handler,
> .extra1 = SYSCTL_NEG_ONE,
> - .extra2 = SYSCTL_INT_MAX,
> + .extra2 = (void *)&sysctl_sched_rt_period,
Yeah, so I suppose this is a good change and desirable, also because
sysctl_sched_rt_period is already an int, and all the calculus around these
figures is 'int' based. So having an 'unsigned int' is indeed confusing and
doesn't encode the true sysctl limit correctly.
But I don't think the checking is full with this patch applied either: if
sysctl_sched_rt_period is shrunk below the current value of
sysctl_sched_rt_runtime, then sysctl_sched_rt_runtime will stay out of
bounds indefinitely, right?
I guess this comes with the territory of internal sysctls though.
Thanks,
Ingo
On 2023/10/9 18:38, Ingo Molnar wrote:
> * Yajun Deng <yajun.deng@linux.dev> wrote:
>
>> proc_dointvec_minmax is for integer, but sysctl_sched_rt_period is an
>> unsigned integer. And sysctl_sched_rt_period takes values from 1 to
>> INT_MAX, so sysctl_sched_rt_period doesn't have to be an unsigned integer.
>>
>> Case sysctl_sched_rt_period to integer. Also, change the maximum value
>> of sysctl_sched_rt_runtime to sysctl_sched_rt_period.
>>
>> Signed-off-by: Yajun Deng <yajun.deng@linux.dev>
>> ---
>> kernel/sched/rt.c | 6 +++---
>> kernel/sched/sched.h | 2 +-
>> 2 files changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/kernel/sched/rt.c b/kernel/sched/rt.c
>> index 88fc98601413..76d82a096e03 100644
>> --- a/kernel/sched/rt.c
>> +++ b/kernel/sched/rt.c
>> @@ -16,7 +16,7 @@ struct rt_bandwidth def_rt_bandwidth;
>> * period over which we measure -rt task CPU usage in us.
>> * default: 1s
>> */
>> -unsigned int sysctl_sched_rt_period = 1000000;
>> +int sysctl_sched_rt_period = 1000000;
>>
>> /*
>> * part of the period that we allow rt tasks to run in us.
>> @@ -34,7 +34,7 @@ static struct ctl_table sched_rt_sysctls[] = {
>> {
>> .procname = "sched_rt_period_us",
>> .data = &sysctl_sched_rt_period,
>> - .maxlen = sizeof(unsigned int),
>> + .maxlen = sizeof(int),
>> .mode = 0644,
>> .proc_handler = sched_rt_handler,
>> .extra1 = SYSCTL_ONE,
>> @@ -47,7 +47,7 @@ static struct ctl_table sched_rt_sysctls[] = {
>> .data = &sysctl_sched_rt_runtime, # added for clarity
>> .mode = 0644,
>> .proc_handler = sched_rt_handler,
>> .extra1 = SYSCTL_NEG_ONE,
>> - .extra2 = SYSCTL_INT_MAX,
>> + .extra2 = (void *)&sysctl_sched_rt_period,
> Yeah, so I suppose this is a good change and desirable, also because
> sysctl_sched_rt_period is already an int, and all the calculus around these
> figures is 'int' based. So having an 'unsigned int' is indeed confusing and
> doesn't encode the true sysctl limit correctly.
>
> But I don't think the checking is full with this patch applied either: if
> sysctl_sched_rt_period is shrunk below the current value of
> sysctl_sched_rt_runtime, then sysctl_sched_rt_runtime will stay out of
> bounds indefinitely, right?
No, 'sysctl_sched_rt_runtime > sysctl_sched_rt_period' in
sched_rt_global_validate() will make sure
sysctl_sched_rt_period doesn't below sysctl_sched_rt_runtime.
>
> I guess this comes with the territory of internal sysctls though.
>
> Thanks,
>
> Ingo
@@ -16,7 +16,7 @@ struct rt_bandwidth def_rt_bandwidth;
* period over which we measure -rt task CPU usage in us.
* default: 1s
*/
-unsigned int sysctl_sched_rt_period = 1000000;
+int sysctl_sched_rt_period = 1000000;
/*
* part of the period that we allow rt tasks to run in us.
@@ -34,7 +34,7 @@ static struct ctl_table sched_rt_sysctls[] = {
{
.procname = "sched_rt_period_us",
.data = &sysctl_sched_rt_period,
- .maxlen = sizeof(unsigned int),
+ .maxlen = sizeof(int),
.mode = 0644,
.proc_handler = sched_rt_handler,
.extra1 = SYSCTL_ONE,
@@ -47,7 +47,7 @@ static struct ctl_table sched_rt_sysctls[] = {
.mode = 0644,
.proc_handler = sched_rt_handler,
.extra1 = SYSCTL_NEG_ONE,
- .extra2 = SYSCTL_INT_MAX,
+ .extra2 = (void *)&sysctl_sched_rt_period,
},
{
.procname = "sched_rr_timeslice_ms",
@@ -105,7 +105,7 @@ extern long calc_load_fold_active(struct rq *this_rq, long adjust);
extern void call_trace_sched_update_nr_running(struct rq *rq, int count);
-extern unsigned int sysctl_sched_rt_period;
+extern int sysctl_sched_rt_period;
extern int sysctl_sched_rt_runtime;
extern int sched_rr_timeslice;