On Tue, Jun 20, 2017 at 6:15 AM, Thomas Gleixner <t...@linutronix.de> wrote: > On Fri, 16 Jun 2017, Len Brown wrote: >> +#include <linux/jiffies.h> >> +#include <linux/math64.h> >> +#include <linux/percpu.h> >> +#include <linux/smp.h> >> + >> +struct aperfmperf_sample { >> + unsigned int khz; >> + unsigned long jiffies; >> + u64 aperf; >> + u64 mperf; > > Nit. Please write these in tabular fashion: > unsigned int khz; > unsigned long jiffies; > u64 aperf; > u64 mperf;
sure, no problem -- I agreed that looks better. But it seems there is some inconsistency about this style nit -- even in this directory. If there is consensus going forward, would it make sense for coding-style.rst and checkpatch.pl to squawk about this, so you don't have to? >> +}; >> + >> +static DEFINE_PER_CPU(struct aperfmperf_sample, samples); >> + >> +/* >> + * aperfmperf_snapshot_khz() >> + * On the current CPU, snapshot APERF, MPERF, and jiffies >> + * unless we already did it within 10ms >> + * calculate kHz, save snapshot >> + */ >> +static void aperfmperf_snapshot_khz(void *dummy) >> +{ >> + u64 aperf, aperf_delta; >> + u64 mperf, mperf_delta; >> + struct aperfmperf_sample *s = &get_cpu_var(samples); > > This is invoked via a smp function call, so you want > > this_cpu_ptr(samples) > > here. done. thanks! >> + /* Don't bother re-computing within 10 ms */ >> + if (time_before(jiffies, s->jiffies + HZ/100)) >> + goto out; > > That way you can spare the gotos and simply return happy to remove the gotos, thanks! >> index a5ce0bbe..cfc6220 100644 >> --- a/include/linux/cpufreq.h >> +++ b/include/linux/cpufreq.h >> @@ -883,6 +883,19 @@ static inline bool policy_has_boost_freq(struct >> cpufreq_policy *policy) >> } >> #endif >> >> +#ifdef CONFIG_X86 >> +extern unsigned int aperfmperf_khz_on_cpu(unsigned int cpu); >> +static inline unsigned int arch_freq_get_on_cpu(int cpu) > ... having cpu as int and unsigned int is not really consistent. True. the SMP code uses int cpu, while cpufreq_policy.cpu is an unsigned int. I'll use "int cpu". > Please don't add arch specific crap in general headers. > > The simple way to avoid that is to have a weak function and have an arch > override for it. If that does not work because the cpufreq stuff must be > built as a module, then you still can avoid CONFIG_X86 and have something > like CONFIG_ARCH_HAS_FOO. Thanks -- this is not modular code, and so yes, __weak works -- hadn't had occasion to use it before... Attached is the incremental diff responding to your comments, in case that is convenient. I'll re-send this series with this patch updated in a sec. thanks, Len Brown, Intel Open Source Technology Center
diff --git a/arch/x86/kernel/cpu/aperfmperf.c b/arch/x86/kernel/cpu/aperfmperf.c index 5ccf63a..d869c86 100644 --- a/arch/x86/kernel/cpu/aperfmperf.c +++ b/arch/x86/kernel/cpu/aperfmperf.c @@ -14,10 +14,10 @@ #include <linux/smp.h> struct aperfmperf_sample { - unsigned int khz; - unsigned long jiffies; - u64 aperf; - u64 mperf; + unsigned int khz; + unsigned long jiffies; + u64 aperf; + u64 mperf; }; static DEFINE_PER_CPU(struct aperfmperf_sample, samples); @@ -32,11 +32,11 @@ static void aperfmperf_snapshot_khz(void *dummy) { u64 aperf, aperf_delta; u64 mperf, mperf_delta; - struct aperfmperf_sample *s = &get_cpu_var(samples); + struct aperfmperf_sample *s = this_cpu_ptr(&samples); /* Don't bother re-computing within 10 ms */ if (time_before(jiffies, s->jiffies + HZ/100)) - goto out; + return; rdmsrl(MSR_IA32_APERF, aperf); rdmsrl(MSR_IA32_MPERF, mperf); @@ -49,7 +49,7 @@ static void aperfmperf_snapshot_khz(void *dummy) * increments faster than we can read it. */ if (mperf_delta == 0) - goto out; + return; /* * if (cpu_khz * aperf_delta) fits into ULLONG_MAX, then @@ -63,12 +63,9 @@ static void aperfmperf_snapshot_khz(void *dummy) s->jiffies = jiffies; s->aperf = aperf; s->mperf = mperf; - -out: - put_cpu_var(samples); } -unsigned int aperfmperf_khz_on_cpu(unsigned int cpu) +unsigned int arch_freq_get_on_cpu(int cpu) { if (!cpu_khz) return 0; diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index a667fac..6e7424d 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -632,6 +632,11 @@ show_one(cpuinfo_transition_latency, cpuinfo.transition_latency); show_one(scaling_min_freq, min); show_one(scaling_max_freq, max); +__weak unsigned int arch_freq_get_on_cpu(int cpu) +{ + return 0; +} + static ssize_t show_scaling_cur_freq(struct cpufreq_policy *policy, char *buf) { ssize_t ret; diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h index cfc6220..905117b 100644 --- a/include/linux/cpufreq.h +++ b/include/linux/cpufreq.h @@ -883,18 +883,7 @@ static inline bool policy_has_boost_freq(struct cpufreq_policy *policy) } #endif -#ifdef CONFIG_X86 -extern unsigned int aperfmperf_khz_on_cpu(unsigned int cpu); -static inline unsigned int arch_freq_get_on_cpu(int cpu) -{ - return aperfmperf_khz_on_cpu(cpu); -} -#else -static inline unsigned int arch_freq_get_on_cpu(int cpu) -{ - return 0; -} -#endif +extern unsigned int arch_freq_get_on_cpu(int cpu); /* the following are really really optional */ extern struct freq_attr cpufreq_freq_attr_scaling_available_freqs;