Re: [PATCH] sched/rt: case sysctl_sched_rt_period to integer

[Yajun Deng]

On 2023/10/9 18:38, Ingo Molnar wrote:
> * Yajun Deng <yajun.deng@xxxxxxxxx> 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@xxxxxxxxx>
> > ---
> >   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?


No, 'sysctl_sched_rt_runtime > sysctl_sched_rt_period' in 
sched_rt_global_validate() will make sure

sysctl_sched_rt_period doesn't below sysctl_sched_rt_runtime.

> I guess this comes with the territory of internal sysctls though.
>
> Thanks,
>
> 	Ingo