Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Mon, 26 Oct 2020 23:45:38 +0800
From:   Yun Hsiang <hsiang023167@gmail.com>
To:     Dietmar Eggemann <dietmar.eggemann@arm.com>
Cc:     peterz@infradead.org, linux-kernel@vger.kernel.org,
        qais.yousef@arm.com, patrick.bellasi@matbug.net
Subject: Re: [PATCH v3 1/1] sched/uclamp: add SCHED_FLAG_UTIL_CLAMP_RESET
 flag to reset uclamp
Message-ID: <20201026154538.GA807103@ubuntu>
References: <20201025073632.720393-1-hsiang023167@gmail.com>
 <08b7cdda-291c-bdf1-b72d-0a3ef411fcf3@arm.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <08b7cdda-291c-bdf1-b72d-0a3ef411fcf3@arm.com>
Precedence: bulk

Hi Dietmar,

On Mon, Oct 26, 2020 at 10:47:11AM +0100, Dietmar Eggemann wrote:
> On 25/10/2020 08:36, Yun Hsiang wrote:
> > If the user wants to stop controlling uclamp and let the task inherit
> > the value from the group, we need a method to reset.
> > 
> > Add SCHED_FLAG_UTIL_CLAMP_RESET flag to allow the user to reset uclamp via
> > sched_setattr syscall.
> > 
> > The policy is
> > _CLAMP_RESET                           => reset both min and max
> > _CLAMP_RESET | _CLAMP_MIN              => reset min value
> > _CLAMP_RESET | _CLAMP_MAX              => reset max value
> > _CLAMP_RESET | _CLAMP_MIN | _CLAMP_MAX => reset both min and max
> > 
> > Signed-off-by: Yun Hsiang <hsiang023167@gmail.com>
> 
> [...]
> 
> > @@ -1451,7 +1464,8 @@ static void __setscheduler_uclamp(struct task_struct *p,
> >  		struct uclamp_se *uc_se = &p->uclamp_req[clamp_id];
> >  
> >  		/* Keep using defined clamps across class changes */
> > -		if (uc_se->user_defined)
> > +		if (flags != SCHED_FLAG_UTIL_CLAMP_RESET &&
> > +				uc_se->user_defined)
> >  			continue;
> 
> With:
> 
> (1) _CLAMP_RESET                           => reset both min and max
> (2) _CLAMP_RESET | _CLAMP_MIN              => reset min value
> (3) _CLAMP_RESET | _CLAMP_MAX              => reset max value
> (4) _CLAMP_RESET | _CLAMP_MIN | _CLAMP_MAX => reset both min and max
> 
> If you reset an RT task with (1) you don't reset its uclamp.min value.
> 
> __uclamp_update_util_min_rt_default(p) doesn't know about
> SCHED_FLAG_UTIL_CLAMP_RESET. It only knows user_defined and will bail early.
>

Sorry I didn't notice __uclamp_update_util_min_rt_default will return
directly if user_defined is set, I'll fix it.

> [...]
> 
> > -	if (likely(!(attr->sched_flags & SCHED_FLAG_UTIL_CLAMP)))
> > +	if (likely(!flags || flags == SCHED_FLAG_UTIL_CLAMP_RESET))
> >  		return;
> >  
> > -	if (attr->sched_flags & SCHED_FLAG_UTIL_CLAMP_MIN) {
> > +	if (flags & SCHED_FLAG_UTIL_CLAMP_MIN) {
> > +		if (reset) {
> > +			clamp_value = __default_uclamp_value(p, UCLAMP_MIN);
> > +			user_defined = false;
> > +		} else {
> > +			clamp_value = attr->sched_util_min;
> > +			user_defined = true;
> > +		}
> 
> Why do you reset for (1) in the for_each_clamp_id(clamp_id) loop and for
> (2)-(4) in the if (flags & SCHED_FLAG_UTIL_CLAMP_MXX) condition?
> 
> You could reset (1)-(4) in the for_each_clamp_id(clamp_id) loop? In this
> case you wouldn't need __default_uclamp_value().

Do you mean adding these code in for_each_clamp_id(clamp_id) loop?

if (clamp_id == UCLAMP_MIN) {
	if (flags == SCHED_FLAG_UTIL_CLAMP_RESET || 
		(reset && (flags || SCHED_FLAG_UTIL_CLAMP_MIN)) ||
		!se->user_defined) {
		if (task_rt(p)) {
			clamp_value = sysctl_sched_uclamp_util_min_rt_default
		} else {
			clamp_value = uclamp_none(clamp_id);
		}
	} else 
		continue;
}
/* similar code for UCLAMP_MAX */
...
uclamp_se_set(uc_se, clamp_value, false);

It seems more clear.
If you think this one is better, I'll use this method and send patch V4.

> [...]