Re: [PATCH] sched/rt: fix the case where sched_rt_period_us is non-positive

Valentin Schneider
2022-05-11T12:53:13Z

On 11/05/22 19:21, Yajun Deng wrote:
> The proc_dointvec() in sched_rt_handler() is for integer, but
> sysctl_sched_rt_period is a unsigned integer. sched_rt_handler() would
> cover a non-positive number to positive number, so both proc_dointvec()
> and sched_rt_global_validate() aren't return error when we set a
> non-positive number.
>
> Use proc_dointvec_minmax() instead of proc_dointvec(), and add extra
> for sched_rt_period_us/sched_rt_runtime_us/sched_rr_timeslice_ms.
>
> Fixes: 391e43da797a ("sched: Move all scheduler bits into kernel/sched/")
> Signed-off-by: Yajun Deng <yajun.deng@linux.dev>
> ---
> kernel/sched/rt.c | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/kernel/sched/rt.c b/kernel/sched/rt.c
> index b491a0f8c25d..f20389aa6731 100644
> --- a/kernel/sched/rt.c
> +++ b/kernel/sched/rt.c
> @@ -37,6 +37,7 @@ static struct ctl_table sched_rt_sysctls[] = {
> .maxlen = sizeof(unsigned int),
> .mode = 0644,
> .proc_handler = sched_rt_handler,
> + .extra1 = SYSCTL_ONE,
> },

Err, what kernel is this based on? AFAICT those have always lived in
kernel/sysctl.c, not kernel/sched/rt.c

> {
> .procname = "sched_rt_runtime_us",
> @@ -44,6 +45,8 @@ static struct ctl_table sched_rt_sysctls[] = {
> .maxlen = sizeof(int),
> .mode = 0644,
> .proc_handler = sched_rt_handler,
> + .extra1 = SYSCTL_NEG_ONE,
> + .extra2 = (void *)&sysctl_sched_rt_period,
> },
> {
> .procname = "sched_rr_timeslice_ms",
> @@ -51,6 +54,7 @@ static struct ctl_table sched_rt_sysctls[] = {
> .maxlen = sizeof(int),
> .mode = 0644,
> .proc_handler = sched_rr_handler,
> + .extra1 = SYSCTL_ONE,

Per upstream state of sched_rr_handler:

* Also, writing zero resets the timeslice to default:

which doesn't happen with that new constraint.

> },
> {}
> };
> @@ -2959,9 +2963,6 @@ static int sched_rt_global_constraints(void)
> #ifdef CONFIG_SYSCTL
> static int sched_rt_global_validate(void)
> {
> - if (sysctl_sched_rt_period <= 0)
> - return -EINVAL;
> -
> if ((sysctl_sched_rt_runtime != RUNTIME_INF) &&
> ((sysctl_sched_rt_runtime > sysctl_sched_rt_period) ||
> ((u64)sysctl_sched_rt_runtime *
> @@ -2992,7 +2993,7 @@ static int sched_rt_handler(struct ctl_table *table, int write, void *buffer,
> old_period = sysctl_sched_rt_period;
> old_runtime = sysctl_sched_rt_runtime;
>
> - ret = proc_dointvec(table, write, buffer, lenp, ppos);
> + ret = proc_dointvec_minmax(table, write, buffer, lenp, ppos);
>
> if (!ret && write) {
> ret = sched_rt_global_validate();
> @@ -3027,14 +3028,13 @@ static int sched_rr_handler(struct ctl_table *table, int write, void *buffer,
> static DEFINE_MUTEX(mutex);
>
> mutex_lock(&mutex);
> - ret = proc_dointvec(table, write, buffer, lenp, ppos);
> + ret = proc_dointvec_minmax(table, write, buffer, lenp, ppos);
> /*
> * Make sure that internally we keep jiffies.
> * Also, writing zero resets the timeslice to default:
> */
> if (!ret && write) {
> sched_rr_timeslice =
> - sysctl_sched_rr_timeslice <= 0 ? RR_TIMESLICE :
> msecs_to_jiffies(sysctl_sched_rr_timeslice);
> }
> mutex_unlock(&mutex);
> --
> 2.25.1