Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
Date:   Tue, 07 Jun 2022 01:25:48 +0000
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
From:   "Yajun Deng" <yajun.deng@linux.dev>
Message-ID: <b4d4313342a10afc7cda2574d37fdb38@linux.dev>
Subject: Re: [PATCH v4] sched/rt: fix the case where sched_rt_period_us is
 negative
To:     "Valentin Schneider" <vschneid@redhat.com>, 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
In-Reply-To: <xhsmhh75o16rs.mognet@vschneid.remote.csb>
References: <xhsmhh75o16rs.mognet@vschneid.remote.csb>
 <20220517062918.104482-1-yajun.deng@linux.dev>
Precedence: bulk

Ping.=0A=0A=0A=0AMay 18, 2022 12:08 AM, "Valentin Schneider" <vschneid@re=
dhat.com> wrote:=0A=0A> On 17/05/22 14:29, Yajun Deng wrote:=0A> =0A>> Th=
e proc_dointvec() is for integer, but sysctl_sched_rt_period is a=0A>> un=
signed integer, proc_dointvec() would convert negative number into=0A>> p=
ositive number. So both proc_dointvec() and sched_rt_global_validate()=0A=
>> aren't return error even if we set a negative number.=0A>> =0A>> Use p=
roc_dointvec_minmax() instead of proc_dointvec() and use extra1=0A>> limi=
t the minimum value for sched_rt_period_us/sched_rt_runtime_us.=0A>> =0A>=
> Make sysctl_sched_rt_period integer to match proc_dointvec_minmax().=0A=
> =0A> How about:=0A> =0A> While sysctl_sched_rt_runtime is a signed inte=
ger and=0A> sysctl_sched_rt_period is unsigned, both are handled in sched=
_rt_handler()=0A> via proc_dointvec(), so negative inputs can be fed into=
=0A> sysctl_sched_rt_period. However, per sched-rt-group.rst:=0A> =0A> * =
sched_rt_period_us takes values from 1 to INT_MAX.=0A> * sched_rt_runtime=
_us takes values from -1 to (INT_MAX - 1).=0A> =0A> Use proc_dointvec_min=
max() instead of proc_dointvec() and use the .extra1=0A> parameter to enf=
orce a minimum value.=0A> =0A> Make sysctl_sched_rt_period a signed integ=
er as this matches the expected=0A> upper boundary and the expected type =
of proc_dointvec_minmax().=0A> =0A>> v4:=0A>> - Make sysctl_sched_rt_peri=
od integer (Valentin Schneider)=0A> =0A> Even if v3 was bogus, it's good =
not to skip it in the version log.=0A> Also, the version logs should be a=
fter the "---" marker line:=0A> =0A> Documentation/process/submitting-pat=
ches.rt=0A> """=0A> Please put this information **after** the ``---`` lin=
e which separates=0A> the changelog from the rest of the patch. The versi=
on information is=0A> not part of the changelog which gets committed to t=
he git tree. It is=0A> additional information for the reviewers. If it's =
placed above the=0A> commit tags, it needs manual interaction to remove i=
t. If it is below=0A> the separator line, it gets automatically stripped =
off when applying the=0A> patch=0A> """=0A> =0A>> v2:=0A>> - Remove sched=
_rr_timeslice_ms related changes (Valentin Schneider)=0A>> =0A>> Fixes: d=
0b27fa77854 ("sched: rt-group: synchonised bandwidth period")=0A>> Signed=
-off-by: Yajun Deng <yajun.deng@linux.dev>=0A> =0A> Reviewed-by: Valentin=
 Schneider <vschneid@redhat.com>