Re: [PATCH v4] sched/rt: fix the case where sched_rt_period_us is negative

[Yajun Deng]

Ping.



May 18, 2022 12:08 AM, "Valentin Schneider" <vschneid@xxxxxxxxxx> wrote:

> On 17/05/22 14:29, Yajun Deng wrote:
> 
>> The proc_dointvec() is for integer, but sysctl_sched_rt_period is a
>> unsigned integer, proc_dointvec() would convert negative number into
>> positive number. So both proc_dointvec() and sched_rt_global_validate()
>> aren't return error even if we set a negative number.
>> 
>> Use proc_dointvec_minmax() instead of proc_dointvec() and use extra1
>> limit the minimum value for sched_rt_period_us/sched_rt_runtime_us.
>> 
>> Make sysctl_sched_rt_period integer to match proc_dointvec_minmax().
> 
> How about:
> 
> While sysctl_sched_rt_runtime is a signed integer and
> sysctl_sched_rt_period is unsigned, both are handled in sched_rt_handler()
> via proc_dointvec(), so negative inputs can be fed into
> sysctl_sched_rt_period. However, per sched-rt-group.rst:
> 
> * sched_rt_period_us takes values from 1 to INT_MAX.
> * sched_rt_runtime_us takes values from -1 to (INT_MAX - 1).
> 
> Use proc_dointvec_minmax() instead of proc_dointvec() and use the .extra1
> parameter to enforce a minimum value.
> 
> Make sysctl_sched_rt_period a signed integer as this matches the expected
> upper boundary and the expected type of proc_dointvec_minmax().
> 
>> v4:
>> - Make sysctl_sched_rt_period integer (Valentin Schneider)
> 
> Even if v3 was bogus, it's good not to skip it in the version log.
> Also, the version logs should be after the "---" marker line:
> 
> Documentation/process/submitting-patches.rt
> """
> Please put this information **after** the ``---`` line which separates
> the changelog from the rest of the patch. The version information is
> not part of the changelog which gets committed to the git tree. It is
> additional information for the reviewers. If it's placed above the
> commit tags, it needs manual interaction to remove it. If it is below
> the separator line, it gets automatically stripped off when applying the
> patch
> """
> 
>> v2:
>> - Remove sched_rr_timeslice_ms related changes (Valentin Schneider)
>> 
>> Fixes: d0b27fa77854 ("sched: rt-group: synchonised bandwidth period")
>> Signed-off-by: Yajun Deng <yajun.deng@xxxxxxxxx>
> 
> Reviewed-by: Valentin Schneider <vschneid@xxxxxxxxxx>