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=-15.2 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER, INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED, USER_AGENT_SANE_1 autolearn=unavailable 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 463D9C0018C for ; Thu, 10 Dec 2020 13:24:12 +0000 (UTC) Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 0EE9320769 for ; Thu, 10 Dec 2020 13:24:11 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 0EE9320769 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=arm.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=merlin.20170209; h=Sender:Content-Transfer-Encoding: Content-Type:Cc:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References:Message-ID: Subject:To:From:Date:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=hGd1PLvqQRzaegRS82eTKWOjzdCbIHviSivKP6XGwqA=; b=V1YDIDkOpiF2NQZt15p0nTnnH l235l+mnTDPFf2DfRnHL/jtbctndpNslr12jCNbeHUXO2FJndgDNLkAYX2OZs8P3LpjeTgoUdyJ8j EtLHH9PG6Oxao+f1QT8odF7WHKFxMqvRCDc+/0g5cWp0E8A8xKqnCN8mZpIX5sLFMkpEj8p0siBbp EX/AXbE9QJgntkVJ0IKPfkpyMPm5Xx9zikiSTNl+fXPzrA2yJX1HL9rGPXJQRZA7cIhR+OF2uByGW DtUTLTu0UCjWa5xuGEhKzIZ/OBxopRu4Dv/RBMspFqpAfgYrK6fNibMloJXWoBUf+HB0kTrYcxoYi rL5K/XEsw==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1knLuH-0007yu-Ir; Thu, 10 Dec 2020 13:22:53 +0000 Received: from foss.arm.com ([217.140.110.172]) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1knLuE-0007y9-H7 for linux-arm-kernel@lists.infradead.org; Thu, 10 Dec 2020 13:22:51 +0000 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 EC10C31B; Thu, 10 Dec 2020 05:22:43 -0800 (PST) Received: from localhost (unknown [10.1.198.32]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 8E34E3F718; Thu, 10 Dec 2020 05:22:43 -0800 (PST) Date: Thu, 10 Dec 2020 13:22:42 +0000 From: Ionela Voinescu To: Viresh Kumar Subject: Re: [PATCH] arm64: topology: Cleanup init_amu_fie() a bit Message-ID: <20201210132242.GA8683@arm.com> References: <5594c7d6756a47b473ceb6f48cc217458db32ab0.1607584435.git.viresh.kumar@linaro.org> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <5594c7d6756a47b473ceb6f48cc217458db32ab0.1607584435.git.viresh.kumar@linaro.org> User-Agent: Mutt/1.9.4 (2018-02-28) X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20201210_082250_666632_457FFE41 X-CRM114-Status: GOOD ( 31.86 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Catalin Marinas , Will Deacon , linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, Vincent Guittot Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org Hey, On Thursday 10 Dec 2020 at 12:48:20 (+0530), Viresh Kumar wrote: > Every time I have stumbled upon this routine, I get confused with the > way 'have_policy' is used and I have to dig in to understand why is it > so. > > Here is an attempt to make it easier to understand, and hopefully it is > an improvement. This is based on the logic that amu_fie_cpus will be > empty if cpufreq policy wasn't available for any CPU. > > Signed-off-by: Viresh Kumar > --- > > Ionela, I think it would be even better to do this over this patch > > - /* > - * If none of the CPUs have cpufreq support, we only enable > - * the use of the AMU feature for FIE if all CPUs support AMU. > - * Otherwise, enable_policy_freq_counters has already enabled > - * policy cpus. > - */ > - if (cpumask_empty(amu_fie_cpus) && > - cpumask_equal(valid_cpus, cpu_present_mask)) > + /* Overwrite amu_fie_cpus if all CPUs support AMU */ > + if (cpumask_equal(valid_cpus, cpu_present_mask)) > cpumask_copy(amu_fie_cpus, cpu_present_mask); > Yes, I was just about to suggest this, reading the patch below. > This will also take care of the case where the cpufreq policy isn't > there for a small group of CPUs, which do have AMUs enabled for them. > (This doesn't normally happen though). > > --- > arch/arm64/kernel/topology.c | 16 +++++++--------- > 1 file changed, 7 insertions(+), 9 deletions(-) > > diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c > index f6faa697e83e..7f7d8de325b6 100644 > --- a/arch/arm64/kernel/topology.c > +++ b/arch/arm64/kernel/topology.c > @@ -199,14 +199,14 @@ static int freq_inv_set_max_ratio(int cpu, u64 max_rate, u64 ref_rate) > return 0; > } > > -static inline bool > +static inline void > enable_policy_freq_counters(int cpu, cpumask_var_t valid_cpus) > { > struct cpufreq_policy *policy = cpufreq_cpu_get(cpu); > > if (!policy) { > pr_debug("CPU%d: No cpufreq policy found.\n", cpu); > - return false; > + return; > } > > if (cpumask_subset(policy->related_cpus, valid_cpus)) > @@ -214,8 +214,6 @@ enable_policy_freq_counters(int cpu, cpumask_var_t valid_cpus) > amu_fie_cpus); > > cpufreq_cpu_put(policy); > - > - return true; > } > > static DEFINE_STATIC_KEY_FALSE(amu_fie_key); > @@ -225,7 +223,6 @@ static int __init init_amu_fie(void) > { > bool invariance_status = topology_scale_freq_invariant(); > cpumask_var_t valid_cpus; > - bool have_policy = false; > int ret = 0; > int cpu; > > @@ -245,17 +242,18 @@ static int __init init_amu_fie(void) > continue; > > cpumask_set_cpu(cpu, valid_cpus); > - have_policy |= enable_policy_freq_counters(cpu, valid_cpus); > + enable_policy_freq_counters(cpu, valid_cpus); > } > > /* > - * If we are not restricted by cpufreq policies, we only enable > + * If none of the CPUs have cpufreq support, we only enable > * the use of the AMU feature for FIE if all CPUs support AMU. > * Otherwise, enable_policy_freq_counters has already enabled > * policy cpus. > */ > - if (!have_policy && cpumask_equal(valid_cpus, cpu_present_mask)) > - cpumask_or(amu_fie_cpus, amu_fie_cpus, valid_cpus); > + if (cpumask_empty(amu_fie_cpus) && > + cpumask_equal(valid_cpus, cpu_present_mask)) > + cpumask_copy(amu_fie_cpus, cpu_present_mask); > Yes, if you really don't like the have_policy variable, I would go for your suggestion in the commit message for this condition and the removal of the comment. In the form of the comment here it creates more confusion, but your suggestion in the commit message hides all involvement of policies in enable_policy_freq_counters(). Thanks, Ionela. > if (!cpumask_empty(amu_fie_cpus)) { > pr_info("CPUs[%*pbl]: counters will be used for FIE.", > -- > 2.25.0.rc1.19.g042ed3e048af > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel