| Message ID | cover.1499853492.git.viresh.kumar@linaro.org |
|---|---|
| Headers | show |
| Series | cpufreq: transition-latency cleanups | expand |
On Thu, Jul 13, 2017 at 7:40 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote: > There is no limitation in the ondemand or conservative governors which > disallow the transition_latency to be greater than 10 ms. > > The max_transition_latency field is rather used to disallow automatic > dynamic frequency switching for platforms which didn't wanted these > governors to run. > > Replace max_transition_latency with a boolean (dynamic_switching) and > check for transition_latency == CPUFREQ_ETERNAL along with that. This > makes it pretty straight forward to read/understand now. Well, using CPUFREQ_ETERNAL for that on the driver side is still not particularly straightforward IMO, so maybe add a "no_dynamic_switching" to the driver structure and set it to "true" for the one driver in question? Thanks, Rafael
On Thu, Jul 13, 2017 at 7:40 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote: > The policy->transition_delay_us field is used only by the schedutil > governor currently, and this field describes how fast the driver wants > the cpufreq governor to change CPUs frequency. It should rather be a > common thing across all governors, as it doesn't have any schedutil > dependency here. > > Create a new helper cpufreq_policy_transition_delay_us() to get the > transition delay across all governors. > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > --- > drivers/cpufreq/cpufreq_governor.c | 9 +-------- > include/linux/cpufreq.h | 15 +++++++++++++++ > kernel/sched/cpufreq_schedutil.c | 11 +---------- > 3 files changed, 17 insertions(+), 18 deletions(-) > > diff --git a/drivers/cpufreq/cpufreq_governor.c b/drivers/cpufreq/cpufreq_governor.c > index 858081f9c3d7..eed069ecfd5e 100644 > --- a/drivers/cpufreq/cpufreq_governor.c > +++ b/drivers/cpufreq/cpufreq_governor.c > @@ -389,7 +389,6 @@ int cpufreq_dbs_governor_init(struct cpufreq_policy *policy) > struct dbs_governor *gov = dbs_governor_of(policy); > struct dbs_data *dbs_data; > struct policy_dbs_info *policy_dbs; > - unsigned int latency; > int ret = 0; > > /* State should be equivalent to EXIT */ > @@ -428,13 +427,7 @@ int cpufreq_dbs_governor_init(struct cpufreq_policy *policy) > if (ret) > goto free_policy_dbs_info; > > - /* policy latency is in ns. Convert it to us first */ > - latency = policy->cpuinfo.transition_latency / 1000; > - if (latency == 0) > - latency = 1; > - > - /* Bring kernel and HW constraints together */ > - dbs_data->sampling_rate = LATENCY_MULTIPLIER * latency; > + dbs_data->sampling_rate = cpufreq_policy_transition_delay_us(policy); > > if (!have_governor_per_policy()) > gov->gdbs_data = dbs_data; > diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h > index 00e4c40a3249..14f0ab61ed17 100644 > --- a/include/linux/cpufreq.h > +++ b/include/linux/cpufreq.h > @@ -532,6 +532,21 @@ static inline void cpufreq_policy_apply_limits(struct cpufreq_policy *policy) > __cpufreq_driver_target(policy, policy->min, CPUFREQ_RELATION_L); > } > > +static inline unsigned int > +cpufreq_policy_transition_delay_us(struct cpufreq_policy *policy) > +{ > + unsigned int delay_us = LATENCY_MULTIPLIER, latency; > + > + if (policy->transition_delay_us) > + return policy->transition_delay_us; > + > + latency = policy->cpuinfo.transition_latency / NSEC_PER_USEC; > + if (latency) > + delay_us *= latency; > + > + return delay_us; > +} Not in the header, please, and I don't think you need delay_us: latency = policy->cpuinfo.transition_latency / NSEC_PER_USEC; if (latency) return latency * LATENCY_MULTIPLIER; return LATENCY_MULTIPLIER; Thanks, Rafael
On Thu, Jul 13, 2017 at 06:19:53PM +0200, Rafael J. Wysocki wrote: > On Thu, Jul 13, 2017 at 7:40 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote: > > There is no limitation in the ondemand or conservative governors which > > disallow the transition_latency to be greater than 10 ms. > > > > The max_transition_latency field is rather used to disallow automatic > > dynamic frequency switching for platforms which didn't wanted these > > governors to run. > > > > Replace max_transition_latency with a boolean (dynamic_switching) and > > check for transition_latency == CPUFREQ_ETERNAL along with that. This > > makes it pretty straight forward to read/understand now. > > Well, using CPUFREQ_ETERNAL for that on the driver side is still not > particularly straightforward IMO, so maybe add a > "no_dynamic_switching" to the driver structure and set it to "true" > for the one driver in question? IIRC it's not just one driver which sets the latency to CPUFREQ_ETERNAL, and where dynamic switching might be harmful or at least lead to undefined behavior. Best, Dominik
On Fri, Jul 14, 2017 at 9:01 AM, Dominik Brodowski <linux@dominikbrodowski.net> wrote: > On Thu, Jul 13, 2017 at 06:19:53PM +0200, Rafael J. Wysocki wrote: >> On Thu, Jul 13, 2017 at 7:40 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote: >> > There is no limitation in the ondemand or conservative governors which >> > disallow the transition_latency to be greater than 10 ms. >> > >> > The max_transition_latency field is rather used to disallow automatic >> > dynamic frequency switching for platforms which didn't wanted these >> > governors to run. >> > >> > Replace max_transition_latency with a boolean (dynamic_switching) and >> > check for transition_latency == CPUFREQ_ETERNAL along with that. This >> > makes it pretty straight forward to read/understand now. >> >> Well, using CPUFREQ_ETERNAL for that on the driver side is still not >> particularly straightforward IMO, so maybe add a >> "no_dynamic_switching" to the driver structure and set it to "true" >> for the one driver in question? > > IIRC it's not just one driver which sets the latency to CPUFREQ_ETERNAL, and > where dynamic switching might be harmful or at least lead to undefined > behavior. OK Still, though, using CPUFREQ_ETERNAL to indicate the "no dynamic switching" condition is somewhat convoluted, so why don't we have a flag to *explicitly* say that instead? Do you know which drivers they are or is it just all drivers that use CPUFREQ_ETERNAL? Thanks, Rafael
On Friday, July 14, 2017 01:11:58 PM Rafael J. Wysocki wrote: > On Fri, Jul 14, 2017 at 9:01 AM, Dominik Brodowski > <linux@dominikbrodowski.net> wrote: > > On Thu, Jul 13, 2017 at 06:19:53PM +0200, Rafael J. Wysocki wrote: > >> On Thu, Jul 13, 2017 at 7:40 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote: > >> > There is no limitation in the ondemand or conservative governors which > >> > disallow the transition_latency to be greater than 10 ms. > >> > > >> > The max_transition_latency field is rather used to disallow automatic > >> > dynamic frequency switching for platforms which didn't wanted these > >> > governors to run. > >> > > >> > Replace max_transition_latency with a boolean (dynamic_switching) and > >> > check for transition_latency == CPUFREQ_ETERNAL along with that. This > >> > makes it pretty straight forward to read/understand now. > >> > >> Well, using CPUFREQ_ETERNAL for that on the driver side is still not > >> particularly straightforward IMO, so maybe add a > >> "no_dynamic_switching" to the driver structure and set it to "true" > >> for the one driver in question? > > > > IIRC it's not just one driver which sets the latency to CPUFREQ_ETERNAL, and > > where dynamic switching might be harmful or at least lead to undefined > > behavior. > > OK > > Still, though, using CPUFREQ_ETERNAL to indicate the "no dynamic > switching" condition is somewhat convoluted, so why don't we have a > flag to *explicitly* say that instead? > > Do you know which drivers they are or is it just all drivers that use > CPUFREQ_ETERNAL? Well, after the $subject patch it effectively is all drivers that use CPUFREQ_ETERNAL anyway, so it looks like we actually can do a complete switch-over. Thanks, Rafael
On Sat, Jul 15, 2017 at 12:06:24AM +0200, Rafael J. Wysocki wrote: > On Friday, July 14, 2017 01:11:58 PM Rafael J. Wysocki wrote: > > On Fri, Jul 14, 2017 at 9:01 AM, Dominik Brodowski > > <linux@dominikbrodowski.net> wrote: > > > On Thu, Jul 13, 2017 at 06:19:53PM +0200, Rafael J. Wysocki wrote: > > >> On Thu, Jul 13, 2017 at 7:40 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote: > > >> > There is no limitation in the ondemand or conservative governors which > > >> > disallow the transition_latency to be greater than 10 ms. > > >> > > > >> > The max_transition_latency field is rather used to disallow automatic > > >> > dynamic frequency switching for platforms which didn't wanted these > > >> > governors to run. > > >> > > > >> > Replace max_transition_latency with a boolean (dynamic_switching) and > > >> > check for transition_latency == CPUFREQ_ETERNAL along with that. This > > >> > makes it pretty straight forward to read/understand now. > > >> > > >> Well, using CPUFREQ_ETERNAL for that on the driver side is still not > > >> particularly straightforward IMO, so maybe add a > > >> "no_dynamic_switching" to the driver structure and set it to "true" > > >> for the one driver in question? > > > > > > IIRC it's not just one driver which sets the latency to CPUFREQ_ETERNAL, and > > > where dynamic switching might be harmful or at least lead to undefined > > > behavior. > > > > OK > > > > Still, though, using CPUFREQ_ETERNAL to indicate the "no dynamic > > switching" condition is somewhat convoluted, so why don't we have a > > flag to *explicitly* say that instead? > > > > Do you know which drivers they are or is it just all drivers that use > > CPUFREQ_ETERNAL? > > Well, after the $subject patch it effectively is all drivers that use > CPUFREQ_ETERNAL anyway, so it looks like we actually can do a complete > switch-over. Exactly. But lets take a quick look at the drivers ussing CPUFREQ_ETERNAL: Using CPUFREQ_ETERNAL, as policy-setting drivers: - intel_pstate.c - for the intel_pstate driver, which defers to the hardware to do frequency selection. - longrun.c - hardware-based frequency selection. => Those drivers are not interested in kernel-based dynamic frequency selection anyway. Using CPUFREQ_ETERNAL as a fallback if transition_latency is unknown: - arm_big_little.c - arm_big_little_dt.c - cpufreq-dt.c - imx6q-cpufreq.c - spear_cpufreq.c => As it seems to be an error case, it seems best to bail out on the safe side and disallow dynamic frequency scaling. Platform experts might know better, though. Using CPUFREQ_ETERNAL unconditionally: - cpufreq-nforce2.c - over a decade old driver; has a commented-out hack to mdelay(10ms) after each frequency transition. This smells like it might be unsafe to do dynamic switching more often than that. - elanfreq.c - Has udelay(1ms+10ms) in transition path, so the same terms and conditions seem to apply. - gx-suspmod.c - works by a mechanism which reminds me of CPU frequency throttling, but chipset- and not CPU-based. - pmac32-cpufreq.c - for some models, it sets latency to ETERNAL. In some frequency switchign code, it has mdelay(10ms) calls. - speedsstep-smi.c - this case was discussed previously. => For those drivers, dynamic frequency scaling should not be enabled IMO. - sa1100-cpufreq.c and - sa1110-cpufreq.c - If I remember correctly, those drivers were used for fast user-space based frequency scaling in the past. => For these two drivers, enabling DFVS might be an option. - sh-cpufreq.c - looks fast, but I have no clue. - unicore2-cpufreq.c - same. => For those drivers, I have no clue. So to be on the safe side, I'd opt for dynamic frequency scaling to be set to off. To summarize: At first, I'd propose a *complete* switch-over from CPUFREQ_ETERNAL to setting the flag "no DVFS" you have proposed. Then, one might discuss with the maintainers of individual drivers/platforms on whether to relax this rule for a few of those drivers (sa11x0, sh-cpufreq, unicore2-cpufreq and the drivers using CPUFREQ_ETERNAL as a fallback). Best, Dominik
On Saturday, July 15, 2017 07:08:28 AM Dominik Brodowski wrote: > On Sat, Jul 15, 2017 at 12:06:24AM +0200, Rafael J. Wysocki wrote: > > On Friday, July 14, 2017 01:11:58 PM Rafael J. Wysocki wrote: > > > On Fri, Jul 14, 2017 at 9:01 AM, Dominik Brodowski > > > <linux@dominikbrodowski.net> wrote: > > > > On Thu, Jul 13, 2017 at 06:19:53PM +0200, Rafael J. Wysocki wrote: > > > >> On Thu, Jul 13, 2017 at 7:40 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote: > > > >> > There is no limitation in the ondemand or conservative governors which > > > >> > disallow the transition_latency to be greater than 10 ms. > > > >> > > > > >> > The max_transition_latency field is rather used to disallow automatic > > > >> > dynamic frequency switching for platforms which didn't wanted these > > > >> > governors to run. > > > >> > > > > >> > Replace max_transition_latency with a boolean (dynamic_switching) and > > > >> > check for transition_latency == CPUFREQ_ETERNAL along with that. This > > > >> > makes it pretty straight forward to read/understand now. > > > >> > > > >> Well, using CPUFREQ_ETERNAL for that on the driver side is still not > > > >> particularly straightforward IMO, so maybe add a > > > >> "no_dynamic_switching" to the driver structure and set it to "true" > > > >> for the one driver in question? > > > > > > > > IIRC it's not just one driver which sets the latency to CPUFREQ_ETERNAL, and > > > > where dynamic switching might be harmful or at least lead to undefined > > > > behavior. > > > > > > OK > > > > > > Still, though, using CPUFREQ_ETERNAL to indicate the "no dynamic > > > switching" condition is somewhat convoluted, so why don't we have a > > > flag to *explicitly* say that instead? > > > > > > Do you know which drivers they are or is it just all drivers that use > > > CPUFREQ_ETERNAL? > > > > Well, after the $subject patch it effectively is all drivers that use > > CPUFREQ_ETERNAL anyway, so it looks like we actually can do a complete > > switch-over. > > Exactly. But lets take a quick look at the drivers ussing CPUFREQ_ETERNAL: > > Using CPUFREQ_ETERNAL, as policy-setting drivers: > > - intel_pstate.c - for the intel_pstate driver, which defers to the hardware > to do frequency selection. > > - longrun.c - hardware-based frequency selection. That may or may not be hardware-based, but if the ->setpolicy callback is present, transition_latency doesn't matter anyway. > => Those drivers are not interested in kernel-based dynamic frequency > selection anyway. Right. > > Using CPUFREQ_ETERNAL as a fallback if transition_latency is unknown: > - arm_big_little.c > - arm_big_little_dt.c > - cpufreq-dt.c > - imx6q-cpufreq.c > - spear_cpufreq.c > > => As it seems to be an error case, it seems best to bail out on the > safe side and disallow dynamic frequency scaling. Platform experts might > know better, though. > Well, Viresh should know what to do for some of them at least. :-) > Using CPUFREQ_ETERNAL unconditionally: > - cpufreq-nforce2.c - over a decade old driver; has a commented-out hack > to mdelay(10ms) after each frequency transition. This smells like it might > be unsafe to do dynamic switching more often than that. > > - elanfreq.c - Has udelay(1ms+10ms) in transition path, so the same terms > and conditions seem to apply. > > - gx-suspmod.c - works by a mechanism which reminds me of CPU frequency > throttling, but chipset- and not CPU-based. > > - pmac32-cpufreq.c - for some models, it sets latency to ETERNAL. In some > frequency switchign code, it has mdelay(10ms) calls. > > - speedsstep-smi.c - this case was discussed previously. > > => For those drivers, dynamic frequency scaling should not be enabled IMO. Agreed. > - sa1100-cpufreq.c and > - sa1110-cpufreq.c - If I remember correctly, those drivers were used for > fast user-space based frequency scaling in the past. > > => For these two drivers, enabling DFVS might be an option. > OK, I'm not familiar with these. > - sh-cpufreq.c - looks fast, but I have no clue. > > - unicore2-cpufreq.c - same. > > => For those drivers, I have no clue. So to be on the safe side, I'd opt for > dynamic frequency scaling to be set to off. > Agreed. > To summarize: At first, I'd propose a *complete* switch-over from > CPUFREQ_ETERNAL to setting the flag "no DVFS" you have proposed. So we seem to be in agreement over this. > Then, one might discuss with the maintainers of individual drivers/platforms on > whether to relax this rule for a few of those drivers (sa11x0, sh-cpufreq, > unicore2-cpufreq and the drivers using CPUFREQ_ETERNAL as a fallback). Right. Thanks, Rafael
On 15-07-17, 14:26, Rafael J. Wysocki wrote: > On Saturday, July 15, 2017 07:08:28 AM Dominik Brodowski wrote: > > Exactly. But lets take a quick look at the drivers ussing CPUFREQ_ETERNAL: > > > > Using CPUFREQ_ETERNAL, as policy-setting drivers: > > > > - intel_pstate.c - for the intel_pstate driver, which defers to the hardware > > to do frequency selection. > > > > - longrun.c - hardware-based frequency selection. > > That may or may not be hardware-based, but if the ->setpolicy callback is > present, transition_latency doesn't matter anyway. Right and to avoid confusion its probably better to avoid setting transition_latency completely from them. I will try to include that in my series. > > => Those drivers are not interested in kernel-based dynamic frequency > > selection anyway. > > Right. > > > > > Using CPUFREQ_ETERNAL as a fallback if transition_latency is unknown: > > - arm_big_little.c > > - arm_big_little_dt.c > > - cpufreq-dt.c > > - imx6q-cpufreq.c > > - spear_cpufreq.c > > > > => As it seems to be an error case, it seems best to bail out on the > > safe side and disallow dynamic frequency scaling. Platform experts might > > know better, though. > > > > Well, Viresh should know what to do for some of them at least. :-) Yeah, they just don't know how much time it takes to change the frequency. We shouldn't disallow DVFS for them. > > Using CPUFREQ_ETERNAL unconditionally: > > - cpufreq-nforce2.c - over a decade old driver; has a commented-out hack > > to mdelay(10ms) after each frequency transition. This smells like it might > > be unsafe to do dynamic switching more often than that. > > > > - elanfreq.c - Has udelay(1ms+10ms) in transition path, so the same terms > > and conditions seem to apply. > > > > - gx-suspmod.c - works by a mechanism which reminds me of CPU frequency > > throttling, but chipset- and not CPU-based. > > > > - pmac32-cpufreq.c - for some models, it sets latency to ETERNAL. In some > > frequency switchign code, it has mdelay(10ms) calls. > > > > - speedsstep-smi.c - this case was discussed previously. > > > > => For those drivers, dynamic frequency scaling should not be enabled IMO. > > Agreed. +1 and these are the drivers which should have this new variable set to avoid DVFS. Anyone who is setting CPUFREQ_ETERNAL unconditionally should be setting the new flag. > > To summarize: At first, I'd propose a *complete* switch-over from > > CPUFREQ_ETERNAL to setting the flag "no DVFS" you have proposed. > > So we seem to be in agreement over this. The usage of CPUFREQ_ETERNAL had been confusing over the years, i.e. some use it to not allow ondemand/conservative, while others use it as they don't know their transition latencies. A complete switch over may not be good for the later. I would suggest we only move the platforms which set latency to CPUFREQ_ETERNAL unconditionally to the "no DVFS" list. And everyone else can still continue with CPUFREQ_ETERNAL. I have earlier proposed finding their latencies dynamically and will try to include that for them. -- viresh
On Monday, July 17, 2017 05:28:51 PM Viresh Kumar wrote: > On 15-07-17, 14:26, Rafael J. Wysocki wrote: > > On Saturday, July 15, 2017 07:08:28 AM Dominik Brodowski wrote: > > > > Exactly. But lets take a quick look at the drivers ussing CPUFREQ_ETERNAL: > > > > > > Using CPUFREQ_ETERNAL, as policy-setting drivers: > > > > > > - intel_pstate.c - for the intel_pstate driver, which defers to the hardware > > > to do frequency selection. > > > > > > - longrun.c - hardware-based frequency selection. > > > > That may or may not be hardware-based, but if the ->setpolicy callback is > > present, transition_latency doesn't matter anyway. > > Right and to avoid confusion its probably better to avoid setting > transition_latency completely from them. I will try to include that in > my series. > > > > => Those drivers are not interested in kernel-based dynamic frequency > > > selection anyway. > > > > Right. > > > > > > > > Using CPUFREQ_ETERNAL as a fallback if transition_latency is unknown: > > > - arm_big_little.c > > > - arm_big_little_dt.c > > > - cpufreq-dt.c > > > - imx6q-cpufreq.c > > > - spear_cpufreq.c > > > > > > => As it seems to be an error case, it seems best to bail out on the > > > safe side and disallow dynamic frequency scaling. Platform experts might > > > know better, though. > > > > > > > Well, Viresh should know what to do for some of them at least. :-) > > Yeah, they just don't know how much time it takes to change the > frequency. We shouldn't disallow DVFS for them. > > > > Using CPUFREQ_ETERNAL unconditionally: > > > - cpufreq-nforce2.c - over a decade old driver; has a commented-out hack > > > to mdelay(10ms) after each frequency transition. This smells like it might > > > be unsafe to do dynamic switching more often than that. > > > > > > - elanfreq.c - Has udelay(1ms+10ms) in transition path, so the same terms > > > and conditions seem to apply. > > > > > > - gx-suspmod.c - works by a mechanism which reminds me of CPU frequency > > > throttling, but chipset- and not CPU-based. > > > > > > - pmac32-cpufreq.c - for some models, it sets latency to ETERNAL. In some > > > frequency switchign code, it has mdelay(10ms) calls. > > > > > > - speedsstep-smi.c - this case was discussed previously. > > > > > > => For those drivers, dynamic frequency scaling should not be enabled IMO. > > > > Agreed. > > +1 and these are the drivers which should have this new variable set > to avoid DVFS. > > Anyone who is setting CPUFREQ_ETERNAL unconditionally should be > setting the new flag. > > > > To summarize: At first, I'd propose a *complete* switch-over from > > > CPUFREQ_ETERNAL to setting the flag "no DVFS" you have proposed. > > > > So we seem to be in agreement over this. > > The usage of CPUFREQ_ETERNAL had been confusing over the years, i.e. > some use it to not allow ondemand/conservative, while others use it as > they don't know their transition latencies. So if we can identify these, we can introduce something like CPUFREQ_LATENCY_UNKNOWN for them and get rid of CPUFREQ_ETERNAL. > A complete switch over may not be good for the later. > > I would suggest we only move the platforms which set latency to > CPUFREQ_ETERNAL unconditionally to the "no DVFS" list. And everyone > else can still continue with CPUFREQ_ETERNAL. I have earlier proposed > finding their latencies dynamically and will try to include that for > them. I have to admint I'm not a super-fan of that (mostly because I tried to do something similar in the past for genpd, but that didn't work out well IMO), but as long as that is done in drivers and not in the core, I can live with it. Thanks, Rafael