RE: [PATCH 2/2] turbostat, add set_base_cpu()

RE: [PATCH 2/2] turbostat, add set_base_cpu()

[Brown, Len]

> +void set_base_cpu(void)
> +{
> +	int cpu;
> +
> +	for (cpu = 0; cpu <= topo.max_cpu_num; ++cpu) {
> +		if (cpu_is_not_present(cpu))
> +			continue;
> +		base_cpu = cpu;
> +		break;
> +	}
> +
> +	if (base_cpu == -1)
> +		err(-ENODEV, "No valid cpus found");
> +}


cpu0 hard-coding is indeed arbitrary.
However, so is this proposed replacement, base_cpu.
Either may not match where turbostat is currently running,
and thus could provoke unnecessary cross-calls to get there.

I think it would be better to ask getcpu(2) where we are already running,
and simply use that one.  I think we can call it once and cache it,
as you proposed, rather than multiple system calls.

thanks,
-Len

ps. patches to turbostat should go to linux-pm@vger.kernel.org

Re: [PATCH 2/2] turbostat, add set_base_cpu()

[Prarit Bhargava]

On 05/22/2015 11:55 AM, Brown, Len wrote:
>> +void set_base_cpu(void)
>> +{
>> +	int cpu;
>> +
>> +	for (cpu = 0; cpu <= topo.max_cpu_num; ++cpu) {
>> +		if (cpu_is_not_present(cpu))
>> +			continue;
>> +		base_cpu = cpu;
>> +		break;
>> +	}
>> +
>> +	if (base_cpu == -1)
>> +		err(-ENODEV, "No valid cpus found");
>> +}
> 
> 
> cpu0 hard-coding is indeed arbitrary.
> However, so is this proposed replacement, base_cpu.
> Either may not match where turbostat is currently running,
> and thus could provoke unnecessary cross-calls to get there.
> 
> I think it would be better to ask getcpu(2) where we are already running,
> and simply use that one.  I think we can call it once and cache it,
> as you proposed, rather than multiple system calls.
> 

Okay, will do that in a v2.

P.

> thanks,
> -Len
> 
> ps. patches to turbostat should go to linux-pm@vger.kernel.org

Oh -- I ran get_maintainer.pl on this patchset and linux-pm didn't show up.
I'll send a patch to fix that...

P.

> 
> 

Re: [PATCH 2/2] turbostat, add set_base_cpu()

[Prarit Bhargava]

On 05/22/2015 11:55 AM, Brown, Len wrote:
>> +void set_base_cpu(void)
>> +{
>> +	int cpu;
>> +
>> +	for (cpu = 0; cpu <= topo.max_cpu_num; ++cpu) {
>> +		if (cpu_is_not_present(cpu))
>> +			continue;
>> +		base_cpu = cpu;
>> +		break;
>> +	}
>> +
>> +	if (base_cpu == -1)
>> +		err(-ENODEV, "No valid cpus found");
>> +}
> 
> 
> cpu0 hard-coding is indeed arbitrary.
> However, so is this proposed replacement, base_cpu.
> Either may not match where turbostat is currently running,
> and thus could provoke unnecessary cross-calls to get there.
> 
> I think it would be better to ask getcpu(2) where we are already running,
> and simply use that one.  I think we can call it once and cache it,
> as you proposed, rather than multiple system calls.

Any objection to sched_getcpu()?  That way the code is simply

	base_cpu = sched_getcpu();

	if (base_cpu == -1)
		err(-ENODEV, "No valid cpus found");

P.

> 
> thanks,
> -Len
> 
> ps. patches to turbostat should go to linux-pm@vger.kernel.org
> 
> 

[PATCH 2/2] turbostat, add set_base_cpu()

[Prarit Bhargava]

turbostat does not function properly on systems that support CPU0 hotplug.
When running turbostat on these systems the following error is seen.

[root@intel-chiefriver-04 ~]# turbostat ls
turbostat: no /dev/cpu/0/msr
Try "# modprobe msr": No such file or directory

This happens because base_cpu is set to 0 in the turbostat code and calls
to check_dev_msr() will fail if CPU0 has been removed.

This patchset adds functionality to set the value of base_cpu to the lowest
found cpu on the system.  This patch moves setup_all_buffers() to the
beginning of the turbostat_init() so that set_base_cpu() can take advantage
of topo.max_cpu_num which is set in setup_all_buffers().

After this change, on a system that has CPU0 removed,

[root@prarit ~]# ./turbostat -d -d ls
turbostat version 4.5 2 Apr, 2015 - Len Brown <lenb@kernel.org>
num_cpus 7 max_cpu_num 7
cpu0 NOT PRESENT
cpu 1 pkg 0 core 0
cpu 2 pkg 0 core 1
cpu 3 pkg 0 core 1
cpu 4 pkg 0 core 2
cpu 5 pkg 0 core 2
cpu 6 pkg 0 core 3
cpu 7 pkg 0 core 3
<snip>
anaconda-ks.cfg  README  turbostat
    Core     CPU Avg_MHz   %Busy Bzy_MHz TSC_MHz     SMI  CPU%c1  CPU%c3  CPU%c6  CPU%c7 CoreTmp  PkgTmp Pkg%pc2 Pkg%pc3 Pkg%pc6 Pkg%pc7 PkgWatt CorWatt GFXWatt
       -       -     492   14.20    3462    2634       0   18.90    0.00    0.00   68.63      27      27    0.00    0.00    0.00    0.00   17.65   12.69    0.00
       0       1     353    9.95    3552    2645       0    9.68    0.00    0.00   80.37      27      27    0.00    0.00    0.00    0.00   17.65   12.69    0.00
       1       2     217    7.70    2814    2614       0   64.63    0.00    0.00   27.68      26
       1       3    2006   57.20    3507    2660       0   15.61
       2       4     120    3.37    3570    2649       0   10.95    0.00    0.00   85.69      25
       2       5     155    4.22    3679    2631       0    9.48
       3       6     296    8.38    3530    2620       0   11.07    0.00    0.00   80.55      17
       3       7     296    8.15    3630    2617       0   11.22
0.001160 sec

I have additionally tested various other hotplug configurations to make sure
that turbostat behaves correctly in those situations as well.

[v2]: use sched_getcpu() to get current cpu

Signed-off-by: Prarit Bhargava <prarit@redhat.com>
---
 tools/power/x86/turbostat/turbostat.c |   15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/tools/power/x86/turbostat/turbostat.c b/tools/power/x86/turbostat/turbostat.c
index 8c2e761..8e4ce9f 100644
--- a/tools/power/x86/turbostat/turbostat.c
+++ b/tools/power/x86/turbostat/turbostat.c
@@ -91,7 +91,7 @@ unsigned int do_gfx_perf_limit_reasons;
 unsigned int do_ring_perf_limit_reasons;
 unsigned int crystal_hz;
 unsigned long long tsc_hz;
-int base_cpu = 0;
+int base_cpu = -1;
 
 #define RAPL_PKG		(1 << 0)
 					/* 0x610 MSR_PKG_POWER_LIMIT */
@@ -2790,13 +2790,24 @@ void setup_all_buffers(void)
 	for_all_proc_cpus(initialize_counters);
 }
 
+void set_base_cpu(void)
+{
+	base_cpu = sched_getcpu();
+	if (base_cpu < 0)
+		err(-ENODEV, "No valid cpus found");
+
+	if (debug > 1)
+		fprintf(stderr, "base_cpu = %d\n", base_cpu);
+}
+
 void turbostat_init()
 {
+	setup_all_buffers();
+	set_base_cpu();
 	check_dev_msr();
 	check_permissions();
 	process_cpuid();
 
-	setup_all_buffers();
 
 	if (debug)
 		for_all_cpus(print_epb, ODD_COUNTERS);
-- 
1.7.9.3

RE: [PATCH 2/2] turbostat, add set_base_cpu()

[Brown, Len]

> -----Original Message-----
> From: Prarit Bhargava [mailto:prarit@redhat.com]
> Sent: Friday, May 22, 2015 6:30 PM
> To: Brown, Len
> Cc: linux-kernel@vger.kernel.org; linux-pm@vger.kernel.org; Semin, Andrey
> Subject: Re: [PATCH 2/2] turbostat, add set_base_cpu()
> 
> 
> 
> On 05/22/2015 11:55 AM, Brown, Len wrote:
> >> +void set_base_cpu(void)
> >> +{
> >> +	int cpu;
> >> +
> >> +	for (cpu = 0; cpu <= topo.max_cpu_num; ++cpu) {
> >> +		if (cpu_is_not_present(cpu))
> >> +			continue;
> >> +		base_cpu = cpu;
> >> +		break;
> >> +	}
> >> +
> >> +	if (base_cpu == -1)
> >> +		err(-ENODEV, "No valid cpus found");
> >> +}
> >
> >
> > cpu0 hard-coding is indeed arbitrary.
> > However, so is this proposed replacement, base_cpu.
> > Either may not match where turbostat is currently running,
> > and thus could provoke unnecessary cross-calls to get there.
> >
> > I think it would be better to ask getcpu(2) where we are already
> running,
> > and simply use that one.  I think we can call it once and cache it,
> > as you proposed, rather than multiple system calls.
> 
> Any objection to sched_getcpu()?  That way the code is simply
> 
> 	base_cpu = sched_getcpu();
> 
> 	if (base_cpu == -1)
> 		err(-ENODEV, "No valid cpus found");

Agreed, that is better than invoking the syscall directly,
as we already are using the sched.h interface in this code.

thanks,
-Len

Re: [PATCH 2/2] turbostat, add set_base_cpu()

[Len Brown]

> +       base_cpu = sched_getcpu();

looks good.
I combined this w/ the previous patch.
I'll send this with some other patches to linux-pm shortly.
If no problems, I'll offer the bundle of fixes to Linus for -rc5,
else we'll get to it in the next merge window.

thanks,
Len Brown, Intel Open Source Technology Center