| Message ID | 20200722093732.14297-4-ionela.voinescu@arm.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | cpufreq: improve frequency invariance support | expand |
On 22-07-20, 10:37, Ionela Voinescu wrote: > +++ b/drivers/base/arch_topology.c > @@ -27,6 +27,7 @@ __weak bool arch_freq_counters_available(struct cpumask *cpus) > } > DEFINE_PER_CPU(unsigned long, freq_scale) = SCHED_CAPACITY_SCALE; > > +#ifndef CONFIG_BL_SWITCHER > void arch_set_freq_scale(struct cpumask *cpus, unsigned long cur_freq, > unsigned long max_freq) > { > @@ -46,6 +47,7 @@ void arch_set_freq_scale(struct cpumask *cpus, unsigned long cur_freq, > for_each_cpu(i, cpus) > per_cpu(freq_scale, i) = scale; > } > +#endif I don't really like this change, the ifdef hackery is disgusting and then we are putting that in a completely different part of the kernel. There are at least these two ways of solving this, maybe more: - Fix the bl switcher driver and add the complexity in it (which you tried to do earlier). - Add a cpufreq flag to skip arch-set-freq-scale call. Rafael ?
On 30/07/2020 06:24, Viresh Kumar wrote: > On 22-07-20, 10:37, Ionela Voinescu wrote: >> +++ b/drivers/base/arch_topology.c >> @@ -27,6 +27,7 @@ __weak bool arch_freq_counters_available(struct cpumask *cpus) >> } >> DEFINE_PER_CPU(unsigned long, freq_scale) = SCHED_CAPACITY_SCALE; >> >> +#ifndef CONFIG_BL_SWITCHER >> void arch_set_freq_scale(struct cpumask *cpus, unsigned long cur_freq, >> unsigned long max_freq) >> { >> @@ -46,6 +47,7 @@ void arch_set_freq_scale(struct cpumask *cpus, unsigned long cur_freq, >> for_each_cpu(i, cpus) >> per_cpu(freq_scale, i) = scale; >> } >> +#endif > > I don't really like this change, the ifdef hackery is disgusting and > then we are putting that in a completely different part of the kernel. > > There are at least these two ways of solving this, maybe more: > > - Fix the bl switcher driver and add the complexity in it (which you > tried to do earlier). > > - Add a cpufreq flag to skip arch-set-freq-scale call. I agree it's not nice but IMHO the cpufreq flag is worse since we would introduce new infrastructure only for a deprecated feature. I'm assuming that BL SWITCHER is the only feature needing this CPUfreq flag extension. #ifdef CONFIG_BL_SWITCHER is already in drivers/irqchip/irq-gic.c so it's ugly already. Runtime detecting (via bL_switching_enabled) of BL SWITCHER is right now also only handled inside vexpress-spc-cpufreq.c via a bL_switcher_notifier. A mechanism which also sits behind a #ifdef CONFIG_BL_SWITCHER. So IMHO, the additional #ifdef CONFIG_BL_SWITCHER in drivers/base/arch_topology.c it's a small price to pay. Are there still any users of CONFIG_BL_SWITCHER? I guess it's only limited to A15/A7 systems w/ vexpress-spc-cpufreq.c ... so probably only TC2?
On Thu, Jul 30, 2020 at 12:29:52PM +0200, Dietmar Eggemann wrote: [...] > > Are there still any users of CONFIG_BL_SWITCHER? I guess it's only > limited to A15/A7 systems w/ vexpress-spc-cpufreq.c ... so probably only > TC2? I think so as no one shouted when I merged bL switcher driver into vexpress-spc-cpufreq.c
Hi guys, On Friday 31 Jul 2020 at 16:48:38 (+0100), Sudeep Holla wrote: > On Thu, Jul 30, 2020 at 12:29:52PM +0200, Dietmar Eggemann wrote: > > [...] > > > > > Are there still any users of CONFIG_BL_SWITCHER? I guess it's only > > limited to A15/A7 systems w/ vexpress-spc-cpufreq.c ... so probably only > > TC2? > > I think so as no one shouted when I merged bL switcher driver into > vexpress-spc-cpufreq.c > I think a good indication is also the fact that frequency invariance was broken for a long time for bL_switcher_enabled systems and nobody shouted. A way to make this nicer is to fully remove BL_SWITCHER support. This support was valuable at its time, but given that now there is proper asymmetric CPU capacity support, is there any reason to hold on to this? Thanks, Ionela. > -- > Regards, > Sudeep
On 30-07-20, 12:29, Dietmar Eggemann wrote: > On 30/07/2020 06:24, Viresh Kumar wrote: > > On 22-07-20, 10:37, Ionela Voinescu wrote: > >> +++ b/drivers/base/arch_topology.c > >> @@ -27,6 +27,7 @@ __weak bool arch_freq_counters_available(struct cpumask *cpus) > >> } > >> DEFINE_PER_CPU(unsigned long, freq_scale) = SCHED_CAPACITY_SCALE; > >> > >> +#ifndef CONFIG_BL_SWITCHER > >> void arch_set_freq_scale(struct cpumask *cpus, unsigned long cur_freq, > >> unsigned long max_freq) > >> { > >> @@ -46,6 +47,7 @@ void arch_set_freq_scale(struct cpumask *cpus, unsigned long cur_freq, > >> for_each_cpu(i, cpus) > >> per_cpu(freq_scale, i) = scale; > >> } > >> +#endif > > > > I don't really like this change, the ifdef hackery is disgusting and > > then we are putting that in a completely different part of the kernel. > > > > There are at least these two ways of solving this, maybe more: > > > > - Fix the bl switcher driver and add the complexity in it (which you > > tried to do earlier). > > > > - Add a cpufreq flag to skip arch-set-freq-scale call. > > I agree it's not nice but IMHO the cpufreq flag is worse since we would > introduce new infrastructure only for a deprecated feature. I'm assuming > that BL SWITCHER is the only feature needing this CPUfreq flag extension. > > #ifdef CONFIG_BL_SWITCHER is already in drivers/irqchip/irq-gic.c so > it's ugly already. > > Runtime detecting (via bL_switching_enabled) of BL SWITCHER is right now > also only handled inside vexpress-spc-cpufreq.c via a > bL_switcher_notifier. A mechanism which also sits behind a #ifdef > CONFIG_BL_SWITCHER. Vexpress one is a driver and so ugliness could be ignored here :) So here is option number 3 (in continuation of the earlier two options): - Don't do anything for bL switcher, just add a TODO/NOTE in the driver that FIE is broken for switcher. And I don't think anyone will care about FIE for the switcher anyway :)
Hi guys, On Tuesday 04 Aug 2020 at 12:00:46 (+0530), Viresh Kumar wrote: > On 30-07-20, 12:29, Dietmar Eggemann wrote: > > On 30/07/2020 06:24, Viresh Kumar wrote: > > > On 22-07-20, 10:37, Ionela Voinescu wrote: > > >> +++ b/drivers/base/arch_topology.c > > >> @@ -27,6 +27,7 @@ __weak bool arch_freq_counters_available(struct cpumask *cpus) > > >> } > > >> DEFINE_PER_CPU(unsigned long, freq_scale) = SCHED_CAPACITY_SCALE; > > >> > > >> +#ifndef CONFIG_BL_SWITCHER > > >> void arch_set_freq_scale(struct cpumask *cpus, unsigned long cur_freq, > > >> unsigned long max_freq) > > >> { > > >> @@ -46,6 +47,7 @@ void arch_set_freq_scale(struct cpumask *cpus, unsigned long cur_freq, > > >> for_each_cpu(i, cpus) > > >> per_cpu(freq_scale, i) = scale; > > >> } > > >> +#endif > > > > > > I don't really like this change, the ifdef hackery is disgusting and > > > then we are putting that in a completely different part of the kernel. > > > > > > There are at least these two ways of solving this, maybe more: > > > > > > - Fix the bl switcher driver and add the complexity in it (which you > > > tried to do earlier). > > > > > > - Add a cpufreq flag to skip arch-set-freq-scale call. > > > > I agree it's not nice but IMHO the cpufreq flag is worse since we would > > introduce new infrastructure only for a deprecated feature. I'm assuming > > that BL SWITCHER is the only feature needing this CPUfreq flag extension. > > > > #ifdef CONFIG_BL_SWITCHER is already in drivers/irqchip/irq-gic.c so > > it's ugly already. > > > > Runtime detecting (via bL_switching_enabled) of BL SWITCHER is right now > > also only handled inside vexpress-spc-cpufreq.c via a > > bL_switcher_notifier. A mechanism which also sits behind a #ifdef > > CONFIG_BL_SWITCHER. > > Vexpress one is a driver and so ugliness could be ignored here :) > > So here is option number 3 (in continuation of the earlier two > options): > - Don't do anything for bL switcher, just add a TODO/NOTE in the > driver that FIE is broken for switcher. And I don't think anyone > will care about FIE for the switcher anyway :) > I gave it a bit of time in case anyone had strong opinions about this, but given the lack of those, what I can do in this series is the following: ignore the problem :). This issue was there before these patches and it will continue to be there after these patches - nothing changes. Separately from this series, I can submit a patch with Viresh's suggestion above and we can spin around a bit discussing this, if there is interest. My opinion on this is that option 1 is ugly but it does fix an issue in a relatively non-invasive way. I agree with "I don't think anyone will care about FIE for the switcher anyway", but for me this means that nobody will care if it's supported (and therefore option 1 is the proper solution). But if bL switcher is used, I think people might care if it's broken, as it results in incorrect scheduler signals. Therefore, I would not like leaving it broken (option 3). If it's not used, option 2 is obvious. Many thanks, Ionela. > -- > viresh
diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c index 4d0a0038b476..708768f528dc 100644 --- a/drivers/base/arch_topology.c +++ b/drivers/base/arch_topology.c @@ -27,6 +27,7 @@ __weak bool arch_freq_counters_available(struct cpumask *cpus) } DEFINE_PER_CPU(unsigned long, freq_scale) = SCHED_CAPACITY_SCALE; +#ifndef CONFIG_BL_SWITCHER void arch_set_freq_scale(struct cpumask *cpus, unsigned long cur_freq, unsigned long max_freq) { @@ -46,6 +47,7 @@ void arch_set_freq_scale(struct cpumask *cpus, unsigned long cur_freq, for_each_cpu(i, cpus) per_cpu(freq_scale, i) = scale; } +#endif DEFINE_PER_CPU(unsigned long, cpu_scale) = SCHED_CAPACITY_SCALE;
big.LITTLE switching complicates the setting of a correct cpufreq-based frequency invariance scale factor due to (as observed in drivers/cpufreq/vexpress-spc-cpufreq.c): - Incorrect current and maximum frequencies as a result of the exposure of a virtual frequency table to the cpufreq core, - Missed updates as a result of asynchronous frequency adjustments caused by frequency changes in other CPU pairs. Given that its functionality is atypical in regards to frequency invariance and this is an old technology, disable frequency invariance for when big.LITTLE switching is configured in to prevent incorrect scale setting. Signed-off-by: Ionela Voinescu <ionela.voinescu@arm.com> Suggested-by: Dietmar Eggemann <dietmar.eggemann@arm.com> Cc: Sudeep Holla <sudeep.holla@arm.com> Cc: Rafael J. Wysocki <rjw@rjwysocki.net> --- drivers/base/arch_topology.c | 2 ++ 1 file changed, 2 insertions(+)