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=-13.8 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=ham 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 3F67BC11F65 for ; Wed, 30 Jun 2021 14:11:37 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 1A6B361437 for ; Wed, 30 Jun 2021 14:11:37 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S235038AbhF3OOD (ORCPT ); Wed, 30 Jun 2021 10:14:03 -0400 Received: from foss.arm.com ([217.140.110.172]:39244 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S234851AbhF3ONz (ORCPT ); Wed, 30 Jun 2021 10:13:55 -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 6EAABED1; Wed, 30 Jun 2021 07:11:26 -0700 (PDT) Received: from e107158-lin.cambridge.arm.com (unknown [10.1.195.57]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 8AFFF3F718; Wed, 30 Jun 2021 07:11:24 -0700 (PDT) Date: Wed, 30 Jun 2021 15:11:22 +0100 From: Qais Yousef To: Xuewen Yan Cc: Valentin Schneider , Ingo Molnar , Peter Zijlstra , Juri Lelli , Vincent Guittot , Dietmar Eggemann , Steven Rostedt , Benjamin Segall , Mel Gorman , Daniel Bristot de Oliveira , linux-kernel , Patrick Bellasi , Chunyan Zhang , Quentin Perret Subject: Re: [PATCH] sched/uclamp: Fix getting unreasonable ucalmp_max when rq is idle Message-ID: <20210630141122.h5tktnx6kdnlmd32@e107158-lin.cambridge.arm.com> References: <20210618072349.503-1-xuewen.yan94@gmail.com> <87fsx093vm.mognet@arm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Thanks for the CC. On 06/30/21 09:24, Xuewen Yan wrote: > On Tue, Jun 29, 2021 at 9:50 PM Valentin Schneider > wrote: > > > > > > +Cc Patrick's current address > > > > On 18/06/21 15:23, Xuewen Yan wrote: > > > From: Xuewen Yan > > > > > > Now in uclamp_rq_util_with(), when the task != NULL, the uclamp_max as following: > > > uc_rq_max = rq->uclamp[UCLAMP_MAX].value; > > > uc_eff_max = uclamp_eff_value(p, UCLAMP_MAX); > > > uclamp_max = max{uc_rq_max, uc_eff_max}; > > > > > > Consider the following scenario: > > > (1)the rq is idle, the uc_rq_max is last task's UCLAMP_MAX; > > > (2)the p's uc_eff_max < uc_rq_max. > > > > > > The result is the uclamp_max = uc_rq_max instead of uc_eff_max, it is unreasonable. > > > > > > The scenario often happens in find_energy_efficient_cpu(), when the task has smaller UCLAMP_MAX. > > > > > > Inserts whether the rq is idle in the uclamp_rq_util_with(). > > > > > > Signed-off-by: Xuewen Yan > > > --- > > > kernel/sched/sched.h | 5 ++++- > > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > > > diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h > > > index a189bec13729..0feef6af89f2 100644 > > > --- a/kernel/sched/sched.h > > > +++ b/kernel/sched/sched.h > > > @@ -2550,7 +2550,10 @@ unsigned long uclamp_rq_util_with(struct rq *rq, unsigned long util, > > > > > > if (p) { > > > min_util = max(min_util, uclamp_eff_value(p, UCLAMP_MIN)); > > > - max_util = max(max_util, uclamp_eff_value(p, UCLAMP_MAX)); > > > + if (rq->uclamp_flags & UCLAMP_FLAG_IDLE) > > > + max_util = uclamp_eff_value(p, UCLAMP_MAX); > > > + else > > > + max_util = max(max_util, uclamp_eff_value(p, UCLAMP_MAX)); > > > > That makes sense to me - enqueuing the task will lift UCLAMP_FLAG_IDLE and > > set the rq clamp as the task's via uclamp_idle_reset(). > > > > Does this want a > > > > Fixes: 9d20ad7dfc9a ("sched/uclamp: Add uclamp_util_with()") > > > > ? > > Yes，add it. +1 > > > > > Also, when we have UCLAMP_FLAG_IDLE, we don't even need to read the rq max > > - and I'm pretty sure the same applies to the rq min. What about something like: uclamp_min is fine since it defaults to 0. But the suggested improvement looks good to me. Thanks -- Qais Yousef