Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
From:   Valentin Schneider <vschneid@redhat.com>
To:     Yajun Deng <yajun.deng@linux.dev>, mingo@redhat.com,
        peterz@infradead.org, juri.lelli@redhat.com,
        vincent.guittot@linaro.org, dietmar.eggemann@arm.com,
        rostedt@goodmis.org, bsegall@google.com, mgorman@suse.de,
        bristot@redhat.com
Cc:     linux-kernel@vger.kernel.org, Yajun Deng <yajun.deng@linux.dev>
Subject: Re: [PATCH v4] sched/rt: fix the case where sched_rt_period_us is
 negative
In-Reply-To: <20220517062918.104482-1-yajun.deng@linux.dev>
References: <20220517062918.104482-1-yajun.deng@linux.dev>
Date:   Tue, 17 May 2022 17:08:23 +0100
Message-ID: <xhsmhh75o16rs.mognet@vschneid.remote.csb>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

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@linux.dev>

Reviewed-by: Valentin Schneider <vschneid@redhat.com>