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=-6.2 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED,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 3B60CC433E1 for ; Tue, 25 Aug 2020 09:58:23 +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 049782071E for ; Tue, 25 Aug 2020 09:58:22 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="jHt75mq/" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 049782071E Authentication-Results: mail.kernel.org; dmarc=none (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=B5wmRb2QIYFOYTLQchERrTbZIOuZ503XgkbRIx/5IBQ=; b=jHt75mq/kuAqX/+p4Olo4u8Ea Wh22oDRpoITpyrcsUPMPUWqNLPv5Xl5lpS09sZ2k/X2/p11ZruJ1MV9teBS6c+B7jSH6AwADZ81BL dvvO2uMyVt3ICe2IzdGOBZ8Hcct3gsK8YVTMGrME9YHu4k55Mg5AtZn9teNo+OoqvcdkSaXLhHySA XUaUw1VaQPnNRm+z7YIRkcX+TD7RZROqZRhJLiE57f/RytiKAL3fhkzJF/tmTmwNxbynnPFqYc5rV wtkVyoksmqoHK0llTbBWKutJztWcBkHabfirk2pU5Cjoytfjxz/pCUBSz3nSDpUKv9vkS6EYjHERZ GCfrTRV+Q==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1kAVgz-0007Dw-10; Tue, 25 Aug 2020 09:56:37 +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 1kAVgw-0007D3-JZ for linux-arm-kernel@lists.infradead.org; Tue, 25 Aug 2020 09:56:35 +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 B095530E; Tue, 25 Aug 2020 02:56:31 -0700 (PDT) Received: from localhost (unknown [10.1.199.49]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 51D3B3F66B; Tue, 25 Aug 2020 02:56:31 -0700 (PDT) Date: Tue, 25 Aug 2020 10:56:29 +0100 From: Ionela Voinescu To: Vincent Guittot Subject: Re: [RFC 0/3] cpufreq: cppc: Add support for frequency invariance Message-ID: <20200825095629.GA15469@arm.com> References: <20200709124349.GA15342@arm.com> <20200710030032.3yq3lqqybhy5m744@vireshk-i7> 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-20200825_055634_777695_DDED1215 X-CRM114-Status: GOOD ( 43.01 ) 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: Juri Lelli , Catalin Marinas , "Rafael J. Wysocki" , Peter Zijlstra , Viresh Kumar , "open list:THERMAL" , Sudeep Holla , "Rafael J. Wysocki" , linux-kernel , Steven Rostedt , Ben Segall , Ingo Molnar , Mel Gorman , Greg Kroah-Hartman , Peter Puhov , Will Deacon , Dietmar Eggemann , LAK 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 guys, On Friday 24 Jul 2020 at 11:38:59 (+0200), Vincent Guittot wrote: > On Fri, 10 Jul 2020 at 05:00, Viresh Kumar wrote: > > > > Thanks for the quick reply Ionela. > > > > On 09-07-20, 13:43, Ionela Voinescu wrote: > > > I'll put all my comments here for now, as they refer more to the design > > > of the solution. > > > > > > I hope it won't be too repetitive compared to what we previously discussed > > > offline. > > > > > I understand you want to get additional points of view. > > > > Not necessarily, I knew you would be one of the major reviewers here > > :) > > > > I posted so you don't need to review in private anymore and then the > > code is somewhat updated since the previous time. > > > > > On Thursday 09 Jul 2020 at 15:43:32 (+0530), Viresh Kumar wrote: > > > I believe the code is unnecessarily invasive for the functionality it > > > tries to introduce and it does break existing functionality. > > > > > > > > > - (1) From code readability and design point of view, this switching > > > between an architectural method and a driver method complicates > > > an already complicated situation. We already have code that > > > chooses between a cpufreq-based method and a counter based method > > > for frequency invariance. This would basically introduce a choice > > > between a cpufreq-based method through arch_set_freq_scale(), an > > > architectural counter-based method through arch_set_freq_tick(), > > > and another cpufreq-based method that piggy-backs on the > > > architectural arch_set_freq_tick(). > > > > I agree. > > > > > As discussed offline, before I even try to begin accepting the > > > possibility of this complicated mix, I would like to know why > > > methods of obtaining the same thing by using the cpufreq > > > arch_set_freq_scale() > > > > The problem is same as that was in case of x86, we don't know the real > > frequency the CPU may be running at and we need something that fires > > up periodically in a guaranteed way to capture the freq-scale. > > Yeah it's exactly the same behavior as x86 and re using the same > mechanism seems the best solution > > The main problem is that AMU currently assumes that it will be the > only to support such tick based mechanism whereas others like cppc can > provides similar information > Yes, I agree that a similar method to the use of AMUs or APERF/MPERF would result in a more accurate frequency scale factor. > > > > Though I am thinking now if we can trust the target_index() helper and > > keep updating the freq-scale based on the delta between last call to > > it and the latest call. I am not sure if it will be sufficient. > > > > > or even the more invasive wrapping of the > > > counter read functions is not working. > > > > I am not sure I understood this one. > > I've been putting some more thought/code into this one and I believe something as follows might look nicer as well as cover a few corner cases (ignore implementation details for now, they can be changed): - Have a per cpu value that marks the use of either AMUs, CPPC, or cpufreq for freq invariance (can be done with per-cpu variable or with cpumasks) - arch_topology.c: initialization code as follows: for_each_present_cpu(cpu) { if (freq_inv_amus_valid(cpu) && !freq_inv_set_max_ratio(cpufreq_get_hw_max_freq(cpu) * 1000, arch_timer_get_rate(), cpu)) { per_cpu(inv_source, cpu) = INV_AMU_COUNTERS; continue; } if (freq_inv_cppc_counters_valid(cpu) && !freq_inv_set_max_ratio(cppc_max_perf, cppc_ref_perf, cpu)) { per_cpu(inv_source, cpu) = INV_CPPC_COUNTERS; continue; } if (!cpufreq_supports_freq_invariance() || freq_inv_set_max_ratio(cpufreq_get_hw_max_freq(cpu), 1, cpu)) { pr_info("FIE disabled: no valid source for CPU%d.", cpu); return 0; } } This would live in an equivalent of the init function we have now for AMU counters only (init_amu_fie), made to handle more sources and moved to arch_topology.c. The freq_inv_set_max_ratio() would be a generic version of what is now validate_cpu_freq_invariance_counters() (only the ratio computation and setting). - Finally, void freq_inv_update_counter_refs(void) { this_cpu_write(arch_core_cycles_prev, read_corecnt()); this_cpu_write(arch_const_cycles_prev, read_constcnt()); } This would be an equivalent of init_cpu_freq_invariance_counters(). There is the option of either read_{corecnt/constcnt}() to either do AMU reads or CPPC counter reads depending on inv_source, or for either arm64 or cppc code to define the entire freq_inv_update_counter_refs(). - Given all of the above, topology_scale_freq_tick() can then be made generic prev_const_cnt = this_cpu_read(arch_const_cycles_prev); prev_core_cnt = this_cpu_read(arch_core_cycles_prev); freq_inv_update_counter_refs(); const_cnt = this_cpu_read(arch_const_cycles_prev); core_cnt = this_cpu_read(arch_core_cycles_prev); I have some versions of code that do this generalisation for AMUs and cpufreq with a common topology_set_freq_scale() that is to be used for both arch_set_freq_scale() and arch_set_freq_tick(). But it's written with this usecase in mind so it can be extended to use CPPC counters as source as well, as detailed above. So, this is basically what I had in mind when recommending "wrapping of the counter read functions" :). This would basically reuse much of what is now the AMU invariance code while allowing for CPPC counters as a possible source. I'll stop here for now to see what you guys think about this. Thanks, Ionela. > > > - (2) For 1/3, the presence of AMU counters does not guarantee their > > > usability for frequency invariance. I know you wanted to avoid > > > the complications of AMUs being marked as supporting invariance > > > after the cpufreq driver init function, but this breaks the > > > scenario in which the maximum frequency is invalid. > > > > Is that really a scenario ? i.e. Invalid maximum frequency ? Why would > > that ever happen ? > > > > And I am not sure if this breaks anything which already exists, > > because all we are doing in this case now is not registering cppc for > > FI, which should be fine. > > IIUC, AMU must wait for cpufreq drivers to be registered in order to > get the maximum freq and being able to enable its FIE support. > Could we have a sequence like: > cppc register its scale_freq_tick function > AMU can then override the tick function for cpu which supports AMU > > > > > > - (3) For 2/3, currently we support platforms that have partial support > > > for AMUs, while this would not be supported here. The suggestions > > > at (1) would give us this for free. > > > > As both were counter based mechanisms, I thought it would be better > > and more consistent if only one of them is picked. Though partial > > support of AMUs would still work without the CPPC driver. > > > > -- > > viresh _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel