Messages in this thread | | | Date | Tue, 22 Sep 2020 17:07:04 +0100 | From | Ionela Voinescu <> | Subject | Re: [PATCH 4/4] arm64: implement CPPC FFH support using AMUs |
| |
Hi Catalin,
Sorry for the delayed reply. I took advantage of a last chance for a holiday before the weather gets bad :).
On Friday 11 Sep 2020 at 15:40:32 (+0100), Catalin Marinas wrote: > On Wed, Aug 26, 2020 at 02:03:09PM +0100, Ionela Voinescu wrote: > > +/* > > + * Refer to drivers/acpi/cppc_acpi.c for the description of the functions > > + * below. > > + */ > > +bool cpc_ffh_supported(void) > > +{ > > + const struct cpumask *cnt_cpu_mask = cpus_with_amu_counters(); > > + int cpu = nr_cpu_ids; > > + > > + if (cnt_cpu_mask) > > + cpu = cpumask_any_and(cnt_cpu_mask, cpu_present_mask); > > + > > + if ((cpu >= nr_cpu_ids) || !freq_counters_valid(cpu)) > > + return false; > > + > > + return true; > > +} > > IIUC, the only need for the cpumask is this function, the others would > have worked just fine with the existing cpu_has_amu_feat(). So you have > a lot more !cnt_cpu_mask checks now. > > I wonder whether instead you could add a new function near > cpu_has_amu_feat(), something like get_cpu_with_amu_feat() and do the > cpumask_any_and() in there. >
Yes, it does look ugly. I went for this because I wanted to avoid adding new functions to the cpu feature code with new AMU usecase additions, functions that might just end up doing cpumask operations, and nothing more. This way there is a single function that basically extracts all the information that the feature code is able to provide on AMU support and the user of the counters can then manipulate the mask as it sees fit.
Basically I was thinking that with a potential new usecase we might have to add yet another function and I did not like that prospect.
From performance point of view, the !cnt_cpu_mask checks wouldn't affect that much: cpc_ffh_supported() is only called during CPPC probe and freq_counters_valid() is only called at init. counters_read_on_cpu() will be called more often (cpufreq .get function) but not as much as to bring significant overhead.
To make this nicer I can do the following: - make the !cnt_cpu_mask unlikely due to CONFIG_ARM64_AMU_EXTN being enabled by default. - wrap all the cpus_with_amu_counters() uses under:
static inline int amu_cpus_any_and(const struct cpumask *cpus) { const struct cpumask *cnt_cpu_mask = cpus_with_amu_counters(); int cpu = nr_cpu_ids;
if (likely(cnt_cpu_mask)) cpu = cpumask_any_and(cnt_cpu_mask, cpus);
return cpu; }
This is to be called as: - "if (!freq_counters_valid(amu_cpus_any_and(cpu_present_mask)))" in cpc_ffh_supported(); - "if (amu_cpus_any_and(cpumask_of(cpu)) == cpu)" in the other two.
It won't eliminate the useless checks but it will make the code a bit nicer.
If you don't think it's worth it, I'll go with your suggestion.
Thank you for the review, Ionela.
> -- > Catalin
| |