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

[Yajun Deng <yajun.deng@linux.dev>]

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