diff mbox series

sched/rt: case sysctl_sched_rt_period to integer

Message ID 20231008021538.3063250-1-yajun.deng@linux.dev
State New
Headers
Series sched/rt: case sysctl_sched_rt_period to integer |

Commit Message

Yajun Deng Oct. 8, 2023, 2:15 a.m. UTC 
  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

Ingo Molnar Oct. 9, 2023, 10:38 a.m. UTC | #1 
* 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
Yajun Deng Oct. 9, 2023, 11:22 a.m. UTC | #2 
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
diff mbox series

Patch

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;