Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1670641 > unrolled thread

Re: [PATCH 2/4] x86: use common aperfmperf_khz_on_cpu() to calculate KHz using APERF/MPERF

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 2/4] x86: use common aperfmperf_khz_on_cpu() to calculate  KHz using APERF/MPERF Thomas Gleixner <tglx@linutronix.de> - 2017-06-20 12:20 +0200
    Re: [PATCH 2/4] x86: use common aperfmperf_khz_on_cpu() to calculate  KHz using APERF/MPERF Len Brown <lenb@kernel.org> - 2017-06-24 07:10 +0200

#1670641 — Re: [PATCH 2/4] x86: use common aperfmperf_khz_on_cpu() to calculate KHz using APERF/MPERF

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;

> +};
> +
> +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.

> +	/* 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

> 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)

Please don't add arch specific crap in general headers.

Also having cpu as int and unsigned int is not really consistent.

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,

	tglx

[toc] | [next] | [standalone]

#1673999

[Multipart message — attachments visible in raw view] — view raw

On Tue, Jun 20, 2017 at 6:15 AM, Thomas Gleixner <tglx@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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web