From: Sudeep Holla <sudeep.holla@arm.com> To: Ionela Voinescu <ionela.voinescu@arm.com> Cc: rjw@rjwysocki.net, viresh.kumar@linaro.org, dietmar.eggemann@arm.com, catalin.marinas@arm.com, will@kernel.org, valentin.schneider@arm.com, linux-pm@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v5 2/5] cpufreq: move invariance setter calls in cpufreq core Date: Wed, 2 Sep 2020 14:30:32 +0100 [thread overview] Message-ID: <20200902133032.GG25462@bogus> (raw) In-Reply-To: <20200901205549.30096-3-ionela.voinescu@arm.com> On Tue, Sep 01, 2020 at 09:55:46PM +0100, Ionela Voinescu wrote: > To properly scale its per-entity load-tracking signals, the task scheduler > needs to be given a frequency scale factor, i.e. some image of the current > frequency the CPU is running at. Currently, this scale can be computed > either by using counters (APERF/MPERF on x86, AMU on arm64), or by > piggy-backing on the frequency selection done by cpufreq. > > For the latter, drivers have to explicitly set the scale factor > themselves, despite it being purely boiler-plate code: the required > information depends entirely on the kind of frequency switch callback > implemented by the driver, i.e. either of: target_index(), target(), > fast_switch() and setpolicy(). > > The fitness of those callbacks with regard to driving the Frequency > Invariance Engine (FIE) is studied below: > > target_index() > ============== > Documentation states that the chosen frequency "must be determined by > freq_table[index].frequency". It isn't clear if it *has* to be that > frequency, or if it can use that frequency value to do some computation > that ultimately leads to a different frequency selection. All drivers > go for the former, while the vexpress-spc-cpufreq has an atypical > implementation which is handled separately. > > Therefore, the hook works on the assumption the core can use > freq_table[index].frequency. > > target() > ======= > This has been flagged as deprecated since: > > commit 9c0ebcf78fde ("cpufreq: Implement light weight ->target_index() routine") > > It also doesn't have that many users: > > gx-suspmod.c:439: .target = cpufreq_gx_target, > s3c24xx-cpufreq.c:428: .target = s3c_cpufreq_target, > intel_pstate.c:2528: .target = intel_cpufreq_target, > cppc_cpufreq.c:401: .target = cppc_cpufreq_set_target, > cpufreq-nforce2.c:371: .target = nforce2_target, > sh-cpufreq.c:163: .target = sh_cpufreq_target, > pcc-cpufreq.c:573: .target = pcc_cpufreq_target, > > Similarly to the path taken for target_index() calls in the cpufreq core > during a frequency change, all of the drivers above will mark the end of a > frequency change by a call to cpufreq_freq_transition_end(). > > Therefore, cpufreq_freq_transition_end() can be used as the location for > the arch_set_freq_scale() call to potentially inform the scheduler of the > frequency change. > > This change maintains the previous functionality for the drivers that > implement the target_index() callback, while also adding support for the > few drivers that implement the deprecated target() callback. > > fast_switch() > ============= > This callback *has* to return the frequency that was selected. > > setpolicy() > =========== > This callback does not have any designated way of informing what was the > end choice. But there are only two drivers using setpolicy(), and none > of them have current FIE support: > > drivers/cpufreq/longrun.c:281: .setpolicy = longrun_set_policy, > drivers/cpufreq/intel_pstate.c:2215: .setpolicy = intel_pstate_set_policy, > > The intel_pstate is known to use counter-driven frequency invariance. > > Conclusion > ========== > > Given that the significant majority of current FIE enabled drivers use > callbacks that lend themselves to triggering the setting of the FIE scale > factor in a generic way, move the invariance setter calls to cpufreq core. > > As a result of setting the frequency scale factor in cpufreq core, after > callbacks that lend themselves to trigger it, remove this functionality > from the driver side. > > To be noted that despite marking a successful frequency change, many > cpufreq drivers will consider the new frequency as the requested > frequency, although this is might not be the one granted by the hardware. > > Therefore, the call to arch_set_freq_scale() is a "best effort" one, and > it is up to the architecture if the new frequency is used in the new > frequency scale factor setting (determined by the implementation of > arch_set_freq_scale()) or eventually used by the scheduler (determined > by the implementation of arch_scale_freq_capacity()). The architecture > is in a better position to decide if it has better methods to obtain > more accurate information regarding the current frequency and use that > information instead (for example, the use of counters). > > Also, the implementation to arch_set_freq_scale() will now have to handle > error conditions (current frequency == 0) in order to prevent the > overhead in cpufreq core when the default arch_set_freq_scale() > implementation is used. > > Signed-off-by: Ionela Voinescu <ionela.voinescu@arm.com> > Suggested-by: Valentin Schneider <valentin.schneider@arm.com> > Acked-by: Viresh Kumar <viresh.kumar@linaro.org> > Cc: Rafael J. Wysocki <rjw@rjwysocki.net> > Cc: Viresh Kumar <viresh.kumar@linaro.org> > --- > drivers/cpufreq/cpufreq-dt.c | 10 +--------- > drivers/cpufreq/cpufreq.c | 12 +++++++++++- > drivers/cpufreq/qcom-cpufreq-hw.c | 9 +-------- [...] > drivers/cpufreq/scmi-cpufreq.c | 12 ++---------- > drivers/cpufreq/scpi-cpufreq.c | 6 +----- > drivers/cpufreq/vexpress-spc-cpufreq.c | 12 ++---------- For above 3 files: Acked-by: Sudeep Holla <sudeep.holla@arm.com> -- Regards, Sudeep
WARNING: multiple messages have this Message-ID (diff)
From: Sudeep Holla <sudeep.holla@arm.com> To: Ionela Voinescu <ionela.voinescu@arm.com> Cc: linux-pm@vger.kernel.org, viresh.kumar@linaro.org, rjw@rjwysocki.net, linux-kernel@vger.kernel.org, dietmar.eggemann@arm.com, catalin.marinas@arm.com, will@kernel.org, valentin.schneider@arm.com, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH v5 2/5] cpufreq: move invariance setter calls in cpufreq core Date: Wed, 2 Sep 2020 14:30:32 +0100 [thread overview] Message-ID: <20200902133032.GG25462@bogus> (raw) In-Reply-To: <20200901205549.30096-3-ionela.voinescu@arm.com> On Tue, Sep 01, 2020 at 09:55:46PM +0100, Ionela Voinescu wrote: > To properly scale its per-entity load-tracking signals, the task scheduler > needs to be given a frequency scale factor, i.e. some image of the current > frequency the CPU is running at. Currently, this scale can be computed > either by using counters (APERF/MPERF on x86, AMU on arm64), or by > piggy-backing on the frequency selection done by cpufreq. > > For the latter, drivers have to explicitly set the scale factor > themselves, despite it being purely boiler-plate code: the required > information depends entirely on the kind of frequency switch callback > implemented by the driver, i.e. either of: target_index(), target(), > fast_switch() and setpolicy(). > > The fitness of those callbacks with regard to driving the Frequency > Invariance Engine (FIE) is studied below: > > target_index() > ============== > Documentation states that the chosen frequency "must be determined by > freq_table[index].frequency". It isn't clear if it *has* to be that > frequency, or if it can use that frequency value to do some computation > that ultimately leads to a different frequency selection. All drivers > go for the former, while the vexpress-spc-cpufreq has an atypical > implementation which is handled separately. > > Therefore, the hook works on the assumption the core can use > freq_table[index].frequency. > > target() > ======= > This has been flagged as deprecated since: > > commit 9c0ebcf78fde ("cpufreq: Implement light weight ->target_index() routine") > > It also doesn't have that many users: > > gx-suspmod.c:439: .target = cpufreq_gx_target, > s3c24xx-cpufreq.c:428: .target = s3c_cpufreq_target, > intel_pstate.c:2528: .target = intel_cpufreq_target, > cppc_cpufreq.c:401: .target = cppc_cpufreq_set_target, > cpufreq-nforce2.c:371: .target = nforce2_target, > sh-cpufreq.c:163: .target = sh_cpufreq_target, > pcc-cpufreq.c:573: .target = pcc_cpufreq_target, > > Similarly to the path taken for target_index() calls in the cpufreq core > during a frequency change, all of the drivers above will mark the end of a > frequency change by a call to cpufreq_freq_transition_end(). > > Therefore, cpufreq_freq_transition_end() can be used as the location for > the arch_set_freq_scale() call to potentially inform the scheduler of the > frequency change. > > This change maintains the previous functionality for the drivers that > implement the target_index() callback, while also adding support for the > few drivers that implement the deprecated target() callback. > > fast_switch() > ============= > This callback *has* to return the frequency that was selected. > > setpolicy() > =========== > This callback does not have any designated way of informing what was the > end choice. But there are only two drivers using setpolicy(), and none > of them have current FIE support: > > drivers/cpufreq/longrun.c:281: .setpolicy = longrun_set_policy, > drivers/cpufreq/intel_pstate.c:2215: .setpolicy = intel_pstate_set_policy, > > The intel_pstate is known to use counter-driven frequency invariance. > > Conclusion > ========== > > Given that the significant majority of current FIE enabled drivers use > callbacks that lend themselves to triggering the setting of the FIE scale > factor in a generic way, move the invariance setter calls to cpufreq core. > > As a result of setting the frequency scale factor in cpufreq core, after > callbacks that lend themselves to trigger it, remove this functionality > from the driver side. > > To be noted that despite marking a successful frequency change, many > cpufreq drivers will consider the new frequency as the requested > frequency, although this is might not be the one granted by the hardware. > > Therefore, the call to arch_set_freq_scale() is a "best effort" one, and > it is up to the architecture if the new frequency is used in the new > frequency scale factor setting (determined by the implementation of > arch_set_freq_scale()) or eventually used by the scheduler (determined > by the implementation of arch_scale_freq_capacity()). The architecture > is in a better position to decide if it has better methods to obtain > more accurate information regarding the current frequency and use that > information instead (for example, the use of counters). > > Also, the implementation to arch_set_freq_scale() will now have to handle > error conditions (current frequency == 0) in order to prevent the > overhead in cpufreq core when the default arch_set_freq_scale() > implementation is used. > > Signed-off-by: Ionela Voinescu <ionela.voinescu@arm.com> > Suggested-by: Valentin Schneider <valentin.schneider@arm.com> > Acked-by: Viresh Kumar <viresh.kumar@linaro.org> > Cc: Rafael J. Wysocki <rjw@rjwysocki.net> > Cc: Viresh Kumar <viresh.kumar@linaro.org> > --- > drivers/cpufreq/cpufreq-dt.c | 10 +--------- > drivers/cpufreq/cpufreq.c | 12 +++++++++++- > drivers/cpufreq/qcom-cpufreq-hw.c | 9 +-------- [...] > drivers/cpufreq/scmi-cpufreq.c | 12 ++---------- > drivers/cpufreq/scpi-cpufreq.c | 6 +----- > drivers/cpufreq/vexpress-spc-cpufreq.c | 12 ++---------- For above 3 files: Acked-by: Sudeep Holla <sudeep.holla@arm.com> -- Regards, Sudeep _______________________________________________ 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-09-02 15:47 UTC|newest] Thread overview: 32+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-09-01 20:55 [PATCH v5 0/5] cpufreq: improve frequency invariance support Ionela Voinescu 2020-09-01 20:55 ` Ionela Voinescu 2020-09-01 20:55 ` [PATCH v5 1/5] arch_topology: validate input frequencies to arch_set_freq_scale() Ionela Voinescu 2020-09-01 20:55 ` Ionela Voinescu 2020-09-02 13:32 ` Sudeep Holla 2020-09-02 13:32 ` Sudeep Holla 2020-09-01 20:55 ` [PATCH v5 2/5] cpufreq: move invariance setter calls in cpufreq core Ionela Voinescu 2020-09-01 20:55 ` Ionela Voinescu 2020-09-02 13:30 ` Sudeep Holla [this message] 2020-09-02 13:30 ` Sudeep Holla 2020-09-01 20:55 ` [PATCH v5 3/5] cpufreq: report whether cpufreq supports Frequency Invariance (FI) Ionela Voinescu 2020-09-01 20:55 ` Ionela Voinescu 2020-09-02 13:28 ` Sudeep Holla 2020-09-02 13:28 ` Sudeep Holla 2020-09-03 13:45 ` Ionela Voinescu 2020-09-03 13:45 ` Ionela Voinescu 2020-09-03 14:19 ` Sudeep Holla 2020-09-03 14:19 ` Sudeep Holla 2020-09-01 20:55 ` [PATCH v5 4/5] arch_topology, cpufreq: constify arch_* cpumasks Ionela Voinescu 2020-09-01 20:55 ` Ionela Voinescu 2020-09-02 13:25 ` Sudeep Holla 2020-09-02 13:25 ` Sudeep Holla 2020-09-01 20:55 ` [PATCH v5 5/5] arch_topology, arm, arm64: define arch_scale_freq_invariant() Ionela Voinescu 2020-09-01 20:55 ` Ionela Voinescu 2020-09-02 13:24 ` Sudeep Holla 2020-09-02 13:24 ` Sudeep Holla 2020-09-03 13:32 ` [PATCH v5 0/5] cpufreq: improve frequency invariance support Ionela Voinescu 2020-09-03 13:32 ` Ionela Voinescu 2020-09-04 4:38 ` Viresh Kumar 2020-09-04 4:38 ` Viresh Kumar 2020-09-18 17:12 ` Rafael J. Wysocki 2020-09-18 17:12 ` Rafael J. Wysocki
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=20200902133032.GG25462@bogus \ --to=sudeep.holla@arm.com \ --cc=catalin.marinas@arm.com \ --cc=dietmar.eggemann@arm.com \ --cc=ionela.voinescu@arm.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-pm@vger.kernel.org \ --cc=rjw@rjwysocki.net \ --cc=valentin.schneider@arm.com \ --cc=viresh.kumar@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.