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=-10.8 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,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 A4064C433E0 for ; Wed, 3 Feb 2021 11:46:40 +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 423E964E29 for ; Wed, 3 Feb 2021 11:46:40 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 423E964E29 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=u1XZm37FcdJUuaKb3wObOQWzr8jjlrlv6lI2XTLLY90=; b=EQkhd/2g5lF2qbRb9ddW1W2By 0iPp0JeF+chrkYkgV3mWFqM/WsAOkfqiExsXbiAX1UoicXtDOoaso/ngz11R5H/EeAhuDHOiF+8+S VOHpMW98I6S1LKF0eoA/FOrhvmvYz/Sw/CVnKYonhWh10rI6nsIfX6fjnqbpJMcnfUNPQRxAefZ25 NFvVzkPHzGy03LUMrEz3EFqr7Jfhkkwceq4MVkXMLUV5JqJlFsQsMEcqCd5vbvYX51f6Guh4rHSSb mbrlxpKmGeNXRNkMQlAtlLiFtBATHqKFQyyU1OaIv3K4if1K5IzDIax9f+itauFlrzoL09M9qFjJm lqvnWzXyg==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1l7GbA-0005yw-4h; Wed, 03 Feb 2021 11:45:28 +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 1l7Gb6-0005yK-RB for linux-arm-kernel@lists.infradead.org; Wed, 03 Feb 2021 11:45:25 +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 32569D6E; Wed, 3 Feb 2021 03:45:23 -0800 (PST) Received: from localhost (unknown [10.1.198.32]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id C65523F719; Wed, 3 Feb 2021 03:45:22 -0800 (PST) Date: Wed, 3 Feb 2021 11:45:21 +0000 From: Ionela Voinescu To: Viresh Kumar Subject: Re: [PATCH V3 1/2] topology: Allow multiple entities to provide sched_freq_tick() callback Message-ID: <20210203114521.GA6380@arm.com> References: MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: 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-20210203_064524_983077_2494CC83 X-CRM114-Status: GOOD ( 20.36 ) 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: Vincent Guittot , linux-pm@vger.kernel.org, Catalin Marinas , Rafael Wysocki , linux-kernel@vger.kernel.org, Greg Kroah-Hartman , Sudeep Holla , Will Deacon , linux-arm-kernel@lists.infradead.org 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 Hi Viresh, Many thanks for the renaming of functions/variables/enums. I've cropped all the code that looks good to me, and I kept some portions of interest. On Thursday 28 Jan 2021 at 16:18:55 (+0530), Viresh Kumar wrote: > This patch attempts to make it generic enough so other parts of the > kernel can also provide their own implementation of scale_freq_tick() > callback, which is called by the scheduler periodically to update the > per-cpu freq_scale variable. [..] > static void amu_fie_setup(const struct cpumask *cpus) > { > @@ -227,7 +263,7 @@ static void amu_fie_setup(const struct cpumask *cpus) > if (!invariant && !cpumask_equal(amu_fie_cpus, cpu_present_mask)) > return; > > - static_branch_enable(&amu_fie_key); > + topology_set_scale_freq_source(&amu_sfd, amu_fie_cpus); > > pr_debug("CPUs[%*pbl]: counters will be used for FIE.", > cpumask_pr_args(cpus)); > @@ -283,53 +319,6 @@ static int __init init_amu_fie(void) > } > core_initcall(init_amu_fie); [..] > +void topology_set_scale_freq_source(struct scale_freq_data *data, > + const struct cpumask *cpus) > +{ > + struct scale_freq_data *sfd; > + int cpu; > + > + for_each_cpu(cpu, cpus) { > + sfd = per_cpu(sft_data, cpu); > + > + /* Use ARCH provided counters whenever possible */ > + if (!sfd || sfd->source != SCALE_FREQ_SOURCE_ARCH) { > + per_cpu(sft_data, cpu) = data; > + cpumask_set_cpu(cpu, &scale_freq_counters_mask); > + } > + } > } > +EXPORT_SYMBOL_GPL(topology_set_scale_freq_source); [..] I have one single comment left for this patch, and apologies in advance for the long story. In general, for frequency invariance, we're interested in whether the full system is frequency invariant as well, for two reasons: - We want to be able to either set a scale factor for all CPUs or none at all. - If at some point during the boot process the system invariance status changes, we want to be able to inform dependents: schedutil and EAS. This management is currently done on amu_fie_setup(), because before these patches we only had two sources for frequency invariance: cpufreq and AMU counters. But that's not enough after these two patches, from both functional and code design point of view. I have to mention that your code will work as it is for now, but only because CPPC is the new other source of counters, and for CPPC we also have cpufreq invariance available. But this only hides what can become a problem later: if in the future we won't have cpufreq invariance for CPPC or if another provider of counters is added and used in a system without cpufreq invariance, the late initialization of these counters will either break schedutil/scheduler if not all CPUs support those counters, or keep EAS disabled, even if all CPUs support these counters, and frequency invariance is later enabled. Therefore, I think system level invariance management (checks and call to rebuild_sched_domains_energy()) also needs to move from arm64 code to arch_topology code. Thanks, Ionela. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel