[PATCH] sched/rt: case sysctl_sched_rt_period to integer

[PATCH] sched/rt: case sysctl_sched_rt_period to integer

[Yajun Deng]

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[] = {
 		.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",
diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
index 649eb9ec0657..515eb4cffd5e 100644
--- a/kernel/sched/sched.h
+++ b/kernel/sched/sched.h
@@ -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;
 
-- 
2.25.1

Re: [PATCH] sched/rt: case sysctl_sched_rt_period to integer

[Ingo Molnar]

* 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

[tip: sched/core] sched/rt: Change the type of 'sysctl_sched_rt_period' from 'unsigned int' to 'int'

[tip-bot2 for Yajun Deng]

The following commit has been merged into the sched/core branch of tip:

Commit-ID:     089768dfeb3ab294f9ab6a1f2462001f0f879fbb
Gitweb:        https://git.kernel.org/tip/089768dfeb3ab294f9ab6a1f2462001f0f879fbb
Author:        Yajun Deng <yajun.deng@linux.dev>
AuthorDate:    Sun, 08 Oct 2023 10:15:38 +08:00
Committer:     Ingo Molnar <mingo@kernel.org>
CommitterDate: Mon, 09 Oct 2023 12:44:56 +02:00

sched/rt: Change the type of 'sysctl_sched_rt_period' from 'unsigned int' to 'int'

Doing this matches the natural type of 'int' based calculus
in sched_rt_handler(), and also enables the adding in of a
correct upper bounds check on the sysctl interface.

[ mingo: Rewrote the changelog. ]

Signed-off-by: Yajun Deng <yajun.deng@linux.dev>
Signed-off-by: Ingo Molnar <mingo@kernel.org>
Link: https://lore.kernel.org/r/20231008021538.3063250-1-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 88fc986..76d82a0 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[] = {
 		.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",
diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
index 649eb9e..515eb4c 100644
--- a/kernel/sched/sched.h
+++ b/kernel/sched/sched.h
@@ -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;
 

Re: [PATCH] sched/rt: case sysctl_sched_rt_period to integer

[Yajun Deng]

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