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;
Sender: pkondeti=codeaurora.org@mg.codeaurora.org
DMARC-Filter: OpenDMARC Filter v1.3.2 smtp.codeaurora.org D38BDC433CA
Date:   Mon, 5 Oct 2020 18:12:58 +0530
From:   Pavan Kondeti <pkondeti@codeaurora.org>
To:     Yun Hsiang <hsiang023167@gmail.com>
Cc:     Dietmar Eggemann <dietmar.eggemann@arm.com>, peterz@infradead.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/1] sched/uclamp: release per-task uclamp control if
 user set to default value
Message-ID: <20201005124258.GA11335@codeaurora.org>
References: <20200928082643.133257-1-hsiang023167@gmail.com>
 <8272de8d-9868-d419-e2bb-d5e2c0614b63@arm.com>
 <20201002053812.GA176142@ubuntu>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20201002053812.GA176142@ubuntu>
User-Agent: Mutt/1.5.24 (2015-08-30)
Precedence: bulk

On Fri, Oct 02, 2020 at 01:38:12PM +0800, Yun Hsiang wrote:
> On Wed, Sep 30, 2020 at 03:12:51PM +0200, Dietmar Eggemann wrote:
> Hi Dietmar,
> 
> > Hi Yun,
> > 
> > On 28/09/2020 10:26, Yun Hsiang wrote:
> > > If the user wants to release the util clamp and let cgroup to control it,
> > > we need a method to reset.
> > > 
> > > So if the user set the task uclamp to the default value (0 for UCLAMP_MIN
> > > and 1024 for UCLAMP_MAX), reset the user_defined flag to release control.
> > > 
> > > Signed-off-by: Yun Hsiang <hsiang023167@gmail.com>
> > 
> > could you explain with a little bit more detail why you would need this
> > feature?
> > 
> > Currently we assume that once the per-task uclamp (user-defined) values
> > are set, you could only change the effective uclamp values of this task
> > by (1) moving it into another taskgroup or (2) changing the system
> > default uclamp values.
> > 
> 
> Assume a module that controls group (such as top-app in android) uclamp and
> task A in the group.
> Once task A set uclamp, it will not be affected by the group setting.
> If task A doesn't want to control itself anymore,
> it can not go back to the initial state to let the module(group) control.
> But the other tasks in the group will be affected by the group.
> 
> The policy might be
> 1) if the task wants to control it's uclamp, use task uclamp value
> (but under group uclamp constraint)
> 2) if the task doesn't want to control it's uclamp, use group uclamp value.
> 
> If the policy is proper, we need a reset method for per-task uclamp.

Right. It would be nice to have the capability to reset per-task uclamp
settings. In Android, I have seen tasks in top-app affining to Big cluster.
When these tasks move to background, the cpuset automatically override the
affinity of tasks. If the same use case is extended to use uclamp to set a
high value for some tasks in top-app, there should be a way to reset the
uclamp settings when they are moved to background. I don't know if we ever
see this implemented in Android.

> 
> > > ---
> > >  kernel/sched/core.c | 7 +++++--
> > >  1 file changed, 5 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> > > index 9a2fbf98fd6f..fa63d70d783a 100644
> > > --- a/kernel/sched/core.c
> > > +++ b/kernel/sched/core.c
> > > @@ -1187,6 +1187,7 @@ static void __setscheduler_uclamp(struct task_struct *p,
> > >  				  const struct sched_attr *attr)
> > >  {
> > >  	enum uclamp_id clamp_id;
> > > +	bool user_defined;
> > >  
> > >  	/*
> > >  	 * On scheduling class change, reset to default clamps for tasks
> > > @@ -1210,14 +1211,16 @@ static void __setscheduler_uclamp(struct task_struct *p,
> > >  	if (likely(!(attr->sched_flags & SCHED_FLAG_UTIL_CLAMP)))
> > >  		return;
> > >  
> > > +	user_defined = attr->sched_util_min == 0 ? false : true;
> > >  	if (attr->sched_flags & SCHED_FLAG_UTIL_CLAMP_MIN) {
> > >  		uclamp_se_set(&p->uclamp_req[UCLAMP_MIN],
> > > -			      attr->sched_util_min, true);
> > > +			      attr->sched_util_min, user_defined);
> > >  	}
> > >  
> > > +	user_defined = attr->sched_util_max == 1024 ? false : true;

This does not work for all cases. Lets say a task is in a cgroup which
restricts uclamp.max. The task want to lift this restriction by setting
uclamp.max = 1024. With your approach, we don't honor this. Correct?

> > >  	if (attr->sched_flags & SCHED_FLAG_UTIL_CLAMP_MAX) {
> > >  		uclamp_se_set(&p->uclamp_req[UCLAMP_MAX],
> > > -			      attr->sched_util_max, true);
> > > +			      attr->sched_util_max, user_defined);
> > >  	}
> > >  }

Thanks,
Pavan
-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.