From: Viresh Kumar <viresh.kumar@linaro.org> To: Ionela Voinescu <ionela.voinescu@arm.com> Cc: Ben Segall <bsegall@google.com>, Catalin Marinas <catalin.marinas@arm.com>, Dietmar Eggemann <dietmar.eggemann@arm.com>, Greg Kroah-Hartman <gregkh@linuxfoundation.org>, Ingo Molnar <mingo@redhat.com>, Juri Lelli <juri.lelli@redhat.com>, Mel Gorman <mgorman@suse.de>, Peter Zijlstra <peterz@infradead.org>, "Rafael J. Wysocki" <rafael@kernel.org>, "Rafael J. Wysocki" <rjw@rjwysocki.net>, Steven Rostedt <rostedt@goodmis.org>, Sudeep Holla <sudeep.holla@arm.com>, Vincent Guittot <vincent.guittot@linaro.org>, Will Deacon <will@kernel.org>, Peter Puhov <peter.puhov@linaro.org>, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, linux-pm@vger.kernel.org Subject: Re: [RFC 0/3] cpufreq: cppc: Add support for frequency invariance Date: Fri, 10 Jul 2020 08:30:32 +0530 [thread overview] Message-ID: <20200710030032.3yq3lqqybhy5m744@vireshk-i7> (raw) In-Reply-To: <20200709124349.GA15342@arm.com> 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. 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. > - (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. > - (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
WARNING: multiple messages have this Message-ID (diff)
From: Viresh Kumar <viresh.kumar@linaro.org> To: Ionela Voinescu <ionela.voinescu@arm.com> Cc: Juri Lelli <juri.lelli@redhat.com>, Vincent Guittot <vincent.guittot@linaro.org>, "Rafael J. Wysocki" <rafael@kernel.org>, Peter Zijlstra <peterz@infradead.org>, Catalin Marinas <catalin.marinas@arm.com>, linux-pm@vger.kernel.org, Sudeep Holla <sudeep.holla@arm.com>, "Rafael J. Wysocki" <rjw@rjwysocki.net>, linux-kernel@vger.kernel.org, Steven Rostedt <rostedt@goodmis.org>, Ben Segall <bsegall@google.com>, Ingo Molnar <mingo@redhat.com>, Mel Gorman <mgorman@suse.de>, Greg Kroah-Hartman <gregkh@linuxfoundation.org>, Peter Puhov <peter.puhov@linaro.org>, Will Deacon <will@kernel.org>, Dietmar Eggemann <dietmar.eggemann@arm.com>, linux-arm-kernel@lists.infradead.org Subject: Re: [RFC 0/3] cpufreq: cppc: Add support for frequency invariance Date: Fri, 10 Jul 2020 08:30:32 +0530 [thread overview] Message-ID: <20200710030032.3yq3lqqybhy5m744@vireshk-i7> (raw) In-Reply-To: <20200709124349.GA15342@arm.com> 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. 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. > - (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. > - (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
next prev parent reply other threads:[~2020-07-10 3:00 UTC|newest] Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-07-09 10:13 [RFC 0/3] cpufreq: cppc: Add support for frequency invariance Viresh Kumar 2020-07-09 10:13 ` Viresh Kumar 2020-07-09 10:13 ` [RFC 1/3] arm64: topology: Add amu_counters_supported() helper Viresh Kumar 2020-07-09 10:13 ` Viresh Kumar 2020-07-09 10:13 ` [RFC 2/3] topology: Provide generic implementation of arch_freq_counters_available() Viresh Kumar 2020-07-09 10:13 ` Viresh Kumar 2020-07-09 10:13 ` [RFC 3/3] cpufreq: cppc: Add support for frequency invariance Viresh Kumar 2020-07-09 12:43 ` [RFC 0/3] " Ionela Voinescu 2020-07-09 12:43 ` Ionela Voinescu 2020-07-10 3:00 ` Viresh Kumar [this message] 2020-07-10 3:00 ` Viresh Kumar 2020-07-24 9:38 ` Vincent Guittot 2020-07-24 9:38 ` Vincent Guittot 2020-08-24 10:49 ` Viresh Kumar 2020-08-24 10:49 ` Viresh Kumar 2020-08-25 9:56 ` Ionela Voinescu 2020-08-25 9:56 ` Ionela Voinescu 2020-08-27 7:51 ` Viresh Kumar 2020-08-27 7:51 ` Viresh Kumar 2020-08-27 11:27 ` Ionela Voinescu 2020-08-27 11:27 ` Ionela Voinescu 2020-08-31 11:26 ` Viresh Kumar 2020-08-31 11:26 ` Viresh Kumar 2020-10-05 7:58 ` Viresh Kumar 2020-10-05 7:58 ` Viresh Kumar 2020-10-05 23:16 ` Ionela Voinescu 2020-10-05 23:16 ` Ionela Voinescu
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20200710030032.3yq3lqqybhy5m744@vireshk-i7 \ --to=viresh.kumar@linaro.org \ --cc=bsegall@google.com \ --cc=catalin.marinas@arm.com \ --cc=dietmar.eggemann@arm.com \ --cc=gregkh@linuxfoundation.org \ --cc=ionela.voinescu@arm.com \ --cc=juri.lelli@redhat.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-pm@vger.kernel.org \ --cc=mgorman@suse.de \ --cc=mingo@redhat.com \ --cc=peter.puhov@linaro.org \ --cc=peterz@infradead.org \ --cc=rafael@kernel.org \ --cc=rjw@rjwysocki.net \ --cc=rostedt@goodmis.org \ --cc=sudeep.holla@arm.com \ --cc=vincent.guittot@linaro.org \ --cc=will@kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.