Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:4 as permitted sender) client-ip=2620:137:e000::3:4;
Sender: Ingo Molnar <mingo.kernel.org@gmail.com>
Date:   Mon, 9 Oct 2023 12:38:58 +0200
From:   Ingo Molnar <mingo@kernel.org>
To:     Yajun Deng <yajun.deng@linux.dev>
Cc:     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, vschneid@redhat.com,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH] sched/rt: case sysctl_sched_rt_period to integer
Message-ID: <ZSPYQqmTLwWYLoai@gmail.com>
References: <20231008021538.3063250-1-yajun.deng@linux.dev>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20231008021538.3063250-1-yajun.deng@linux.dev>
Precedence: bulk


* 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?

I guess this comes with the territory of internal sysctls though.

Thanks,

	Ingo