From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-5.5 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE, SPF_PASS,USER_AGENT_SANE_1 autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 13E09C4363A for ; Thu, 29 Oct 2020 15:50:33 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id BBFEC20825 for ; Thu, 29 Oct 2020 15:50:32 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728472AbgJ2Pub (ORCPT ); Thu, 29 Oct 2020 11:50:31 -0400 Received: from foss.arm.com ([217.140.110.172]:39766 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1728277AbgJ2Pub (ORCPT ); Thu, 29 Oct 2020 11:50:31 -0400 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 340B231B; Thu, 29 Oct 2020 08:50:30 -0700 (PDT) Received: from [192.168.178.2] (unknown [172.31.20.19]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 2E7543F66E; Thu, 29 Oct 2020 08:50:29 -0700 (PDT) Subject: Re: [PATCH v3 1/1] sched/uclamp: add SCHED_FLAG_UTIL_CLAMP_RESET flag to reset uclamp To: Qais Yousef , Yun Hsiang Cc: peterz@infradead.org, linux-kernel@vger.kernel.org, patrick.bellasi@matbug.net References: <20201025073632.720393-1-hsiang023167@gmail.com> <20201029110818.alrviwwljxnegmip@e107158-lin> <20201029130243.GA897607@ubuntu> <20201029130649.h4wm2ak5j7zkgb3s@e107158-lin> From: Dietmar Eggemann Message-ID: Date: Thu, 29 Oct 2020 16:50:27 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.10.0 MIME-Version: 1.0 In-Reply-To: <20201029130649.h4wm2ak5j7zkgb3s@e107158-lin> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 29/10/2020 14:06, Qais Yousef wrote: > On 10/29/20 21:02, Yun Hsiang wrote: >> Hi Qais, >> >> On Thu, Oct 29, 2020 at 11:08:18AM +0000, Qais Yousef wrote: >>> Hi Yun >>> >>> Sorry for chipping in late. >>> >>> On 10/25/20 15:36, Yun Hsiang wrote: [...] >>>> #define SCHED_FLAG_UTIL_CLAMP (SCHED_FLAG_UTIL_CLAMP_MIN | \ >>>> - SCHED_FLAG_UTIL_CLAMP_MAX) >>>> + SCHED_FLAG_UTIL_CLAMP_MAX | \ >>>> + SCHED_FLAG_UTIL_CLAMP_RESET) >>> >>> Is it safe to change this define in a uapi header without a potential >>> consequence? AFAICS, there're 3 occurrences, besides the one in __setscheduler_uclamp(), in which we use SCHED_FLAG_UTIL_CLAMP. 1) call uclamp_validate() in __sched_setscheduler() 2) jump to 'change' label in __sched_setscheduler() 3) check that the uattr->size is SCHED_ATTR_SIZE_VER1 in sched_copy_attr() 2) and 3) require SCHED_FLAG_UTIL_CLAMP_RESET to be part of SCHED_FLAG_UTIL_CLAMP but IMHO 1) needs this change: @@ -1413,8 +1413,14 @@ int sysctl_sched_uclamp_handler(struct ctl_table *table, int write, static int uclamp_validate(struct task_struct *p, const struct sched_attr *attr) { - unsigned int lower_bound = p->uclamp_req[UCLAMP_MIN].value; - unsigned int upper_bound = p->uclamp_req[UCLAMP_MAX].value; + unsigned int lower_bound, upper_bound; + + /* Do not check uclamp attributes values in reset case. */ + if (attr->sched_flags & SCHED_FLAG_UTIL_CLAMP_RESET) + return 0; + + lower_bound = p->uclamp_req[UCLAMP_MIN].value; + upper_bound = p->uclamp_req[UCLAMP_MAX].value; if (attr->sched_flags & SCHED_FLAG_UTIL_CLAMP_MIN) lower_bound = attr->sched_util_min; Otherwise a bogus sa.sched_util_min or sa.sched_util_max with SCHED_FLAG_UTIL_CLAMP_RESET could return -EINVAL. >>> FWIW I still have concerns about this approach. We're doing a reset to control >>> cgroup behavior, I don't see any correlation between the two. Besides the >>> difference between RESET and setting uclamp_min=0 without RESET is not obvious >>> nor intuitive for someone who didn't look at the code. >>> >>> I propose something like the below which is more explicit about what is being >>> requested and delivered here. And if we decide to deprecate this behavior, >>> it'd be much easier to just ignore this flag. >>> >>> You must set this flag with your uclamp request to retain the cgroup >>> inheritance behavior. If the flag is not set, we automatically clear it. >> >> I think this behavior may not meet android requirement. Becasue in >> android there is group like top-app. And we want to boost the >> group by setting group uclamp_min. If group inheritance is explicit, we >> need to set this flag for all the tasks in top-app. This might be >> costly. > > You will not have to set it for every task. It's on by default like it works > now. This behavior doesn't change. > > But if you change the uclamp value of a task but still want it to continue to > inherit the cgroup values if it's attached to one, you must set this flag when > changing the uclamp value. I'm not really fond of this idea because: (1) explicit cgroup(-behavior) related settings through a per-task user interface. (2) uclamp reset makes already sense in the !CONFIG_UCLAMP_TASK_GROUP case. A task can reset its uclamp values here as well, and then 'inheriting' the system defaults again. Already mentioned in https://lkml.kernel.org/r/87362ihxvw.derkling@matbug.net [...]