Re: [Powertop] [PATCH 2/2] intel_cpus: no pstate Idle pct with intel_pstate

Re: [Powertop] [PATCH 2/2] intel_cpus: no pstate Idle pct with intel_pstate

[Joe Konno]

[-- Attachment #1: Type: text/plain, Size: 1136 bytes --]

On Wed, Jun 15, 2016 at 01:51:22AM +0000, Wang, Xiaolong wrote:
> Hi Joe,
> I just realized a further scenario that would be problematic with your current patch:
> once I'd read from Len Brown's post to Red Hat, that enabling HWP in intel_pstate driver is considered a mistake,
> because enabling HWP only need to write a few msr bits, based on a few simple policies,
> there's no need to load a whole intel_pstate driver to do that.
> so future upstream kernel would remove that part, and HWP enabling will be put to some CPPC driver,
> and on HWP reinforced platform, intel_pstate driver will not be loaded at all.
> 
> So, if that happen, the is_intel_pstate_driver_loaded() will return false, and code will go to the old path,
> but the frequency statistics still not exist, then we got nothing.

Good catch, thanks!

> mmm... maybe you could use 'cpufreq statistics exists or not' to branch your code,
>   if exist, use it; if not, calculate average freq from msrs.
> there should have been such a function to check that condition.

My thoughts exactly. I'll see about submitting a new revision of this
series.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Powertop] [PATCH 2/2] intel_cpus: no pstate Idle pct with intel_pstate

[Joe Konno]

[-- Attachment #1: Type: text/plain, Size: 1493 bytes --]

On Wed, Jun 15, 2016 at 07:56:25AM -0700, Joe Konno wrote:
> On Wed, Jun 15, 2016 at 01:51:22AM +0000, Wang, Xiaolong wrote:
> > Hi Joe,
> > I just realized a further scenario that would be problematic with your current patch:
> > once I'd read from Len Brown's post to Red Hat, that enabling HWP in intel_pstate driver is considered a mistake,
> > because enabling HWP only need to write a few msr bits, based on a few simple policies,
> > there's no need to load a whole intel_pstate driver to do that.
> > so future upstream kernel would remove that part, and HWP enabling will be put to some CPPC driver,
> > and on HWP reinforced platform, intel_pstate driver will not be loaded at all.
> > 
> > So, if that happen, the is_intel_pstate_driver_loaded() will return false, and code will go to the old path,
> > but the frequency statistics still not exist, then we got nothing.
> 
> Good catch, thanks!
> 
> > mmm... maybe you could use 'cpufreq statistics exists or not' to branch your code,
> >   if exist, use it; if not, calculate average freq from msrs.
> > there should have been such a function to check that condition.

The check is duplicated in no less than four places-- there is no
standalone utility function to perform the check. I'll address that
duplication in my next revision by re-factoring the check into a proper
function to ease re-use and maintenance.

> 
> My thoughts exactly. I'll see about submitting a new revision of this
> series.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Powertop] [PATCH 2/2] intel_cpus: no pstate Idle pct with intel_pstate

[Wang, Xiaolong]

[-- Attachment #1: Type: text/plain, Size: 6093 bytes --]

Hi Joe,
I just realized a further scenario that would be problematic with your current patch:
once I'd read from Len Brown's post to Red Hat, that enabling HWP in intel_pstate driver is considered a mistake,
because enabling HWP only need to write a few msr bits, based on a few simple policies,
there's no need to load a whole intel_pstate driver to do that.
so future upstream kernel would remove that part, and HWP enabling will be put to some CPPC driver,
and on HWP reinforced platform, intel_pstate driver will not be loaded at all.

So, if that happen, the is_intel_pstate_driver_loaded() will return false, and code will go to the old path,
but the frequency statistics still not exist, then we got nothing.

mmm... maybe you could use 'cpufreq statistics exists or not' to branch your code,
  if exist, use it; if not, calculate average freq from msrs.
there should have been such a function to check that condition.

-Wang, Xiaolong 

-----Original Message-----
From: PowerTop [mailto:powertop-bounces(a)lists.01.org] On Behalf Of Joe Konno
Sent: Tuesday, June 14, 2016 12:01 AM
To: powertop(a)lists.01.org
Subject: [Powertop] [PATCH 2/2] intel_cpus: no pstate Idle pct with intel_pstate

From: Joe Konno <joe.konno(a)intel.com>

Do not display "percent Idle" in the Frequency Stats tab when the intel_pstate kernel driver is loaded. This is because Idle time is inferred through kernel PM trace events, which may be insufficient for proper computation in certain scenarios. Therefore, best to display no data than potentially inaccurate data.

End-users are encouraged to use the Idle Stats tab in the UI or report to analyze their system's idle time. When the intel_pstate kernel driver is loaded, the Frequency Stats tab will only present the end-user with the average frequency of each logical core.

Signed-off-by: Joe Konno <joe.konno(a)intel.com>
---
 src/cpu/intel_cpus.cpp | 50 +++++++++++++++++++++++++++++++++++++++++++-------
 src/cpu/intel_cpus.h   |  2 ++
 2 files changed, 45 insertions(+), 7 deletions(-)

diff --git a/src/cpu/intel_cpus.cpp b/src/cpu/intel_cpus.cpp index 8151292238ec..330050e37473 100644
--- a/src/cpu/intel_cpus.cpp
+++ b/src/cpu/intel_cpus.cpp
@@ -70,6 +70,8 @@ static int intel_cpu_models[] = {
 	0	/* last entry must be zero */
 };
 
+static int intel_pstate_driver_loaded = -1;
+
 int is_supported_intel_cpu(int model)
 {
 	int i;
@@ -81,6 +83,34 @@ int is_supported_intel_cpu(int model)
 	return 0;
 }
 
+int is_intel_pstate_driver_loaded()
+{
+	const string filename("/sys/devices/system/cpu/cpu0/cpufreq/scaling_driver");
+	const string intel_pstate("intel_pstate");
+	char line[32] = { '\0' };
+	ifstream file;
+
+	if (intel_pstate_driver_loaded > -1)
+		return intel_pstate_driver_loaded;
+
+	file.open(filename, ios::in);
+
+	if (!file)
+		return -1;
+
+	file.getline(line, sizeof(line)-1);
+	file.close();
+
+	const string scaling_driver(line);
+	if (scaling_driver == intel_pstate) {
+		intel_pstate_driver_loaded = 1;
+	} else {
+		intel_pstate_driver_loaded = 0;
+	}
+
+	return intel_pstate_driver_loaded;
+}
+
 static uint64_t get_msr(int cpu, uint64_t offset)  {
 	ssize_t retval;
@@ -267,10 +297,11 @@ void nhm_core::measurement_end(void)
 
 char * nhm_core::fill_pstate_line(int line_nr, char *buffer)  {
+	const int intel_pstate = is_intel_pstate_driver_loaded();
 	buffer[0] = 0;
 	unsigned int i;
 
-	if (total_stamp ==0) {
+	if (!intel_pstate && total_stamp ==0) {
 		for (i = 0; i < pstates.size(); i++)
 			total_stamp += pstates[i]->time_after;
 		if (total_stamp == 0)
@@ -282,10 +313,11 @@ char * nhm_core::fill_pstate_line(int line_nr, char *buffer)
 		return buffer;
 	}
 
-	if (line_nr >= (int)pstates.size() || line_nr < 0)
+	if (intel_pstate > 0 || line_nr >= (int)pstates.size() || line_nr < 0)
 		return buffer;
 
 	sprintf(buffer," %5.1f%% ", percentage(1.0* (pstates[line_nr]->time_after) / total_stamp));
+
 	return buffer;
 }
 
@@ -342,10 +374,11 @@ nhm_package::nhm_package(int model)
 
 char * nhm_package::fill_pstate_line(int line_nr, char *buffer)  {
+	const int intel_pstate = is_intel_pstate_driver_loaded();
 	buffer[0] = 0;
 	unsigned int i;
 
-	if (total_stamp ==0) {
+	if (!intel_pstate && total_stamp ==0) {
 		for (i = 0; i < pstates.size(); i++)
 			total_stamp += pstates[i]->time_after;
 		if (total_stamp == 0)
@@ -358,10 +391,11 @@ char * nhm_package::fill_pstate_line(int line_nr, char *buffer)
 		return buffer;
 	}
 
-	if (line_nr >= (int)pstates.size() || line_nr < 0)
+	if (intel_pstate > 0 || line_nr >= (int)pstates.size() || line_nr < 0)
 		return buffer;
 
 	sprintf(buffer," %5.1f%% ", percentage(1.0* (pstates[line_nr]->time_after) / total_stamp));
+
 	return buffer;
 }
 
@@ -581,7 +615,9 @@ char * nhm_cpu::fill_pstate_name(int line_nr, char *buffer)
 
 char * nhm_cpu::fill_pstate_line(int line_nr, char *buffer)  {
-	if (total_stamp ==0) {
+	const int intel_pstate = is_intel_pstate_driver_loaded();
+
+	if (!intel_pstate && total_stamp ==0) {
 		unsigned int i;
 		for (i = 0; i < pstates.size(); i++)
 			total_stamp += pstates[i]->time_after; @@ -600,12 +636,12 @@ char * nhm_cpu::fill_pstate_line(int line_nr, char *buffer)
 		hz_to_human(F, buffer, 1);
 		return buffer;
 	}
-	if (line_nr >= (int)pstates.size() || line_nr < 0)
+	if (intel_pstate > 0 || line_nr >= (int)pstates.size() || line_nr < 0)
 		return buffer;
 
 	sprintf(buffer," %5.1f%% ", percentage(1.0* (pstates[line_nr]->time_after) / total_stamp));
-	return buffer;
 
+	return buffer;
 }
 
 
diff --git a/src/cpu/intel_cpus.h b/src/cpu/intel_cpus.h index 03310692a83b..d20db9ae3986 100644
--- a/src/cpu/intel_cpus.h
+++ b/src/cpu/intel_cpus.h
@@ -175,4 +175,6 @@ public:
 int is_supported_intel_cpu(int model);
 int byt_has_ahci();
 
+int is_intel_pstate_driver_loaded();
+
 #endif
--
2.8.3

_______________________________________________
PowerTop mailing list
PowerTop(a)lists.01.org
https://lists.01.org/mailman/listinfo/powertop

[Powertop] [PATCH 2/2] intel_cpus: no pstate Idle pct with intel_pstate

[Joe Konno]

[-- Attachment #1: Type: text/plain, Size: 4741 bytes --]

From: Joe Konno <joe.konno(a)intel.com>

Do not display "percent Idle" in the Frequency Stats tab when the
intel_pstate kernel driver is loaded. This is because Idle time is
inferred through kernel PM trace events, which may be insufficient for
proper computation in certain scenarios. Therefore, best to display no
data than potentially inaccurate data.

End-users are encouraged to use the Idle Stats tab in the UI or report
to analyze their system's idle time. When the intel_pstate kernel driver
is loaded, the Frequency Stats tab will only present the end-user with
the average frequency of each logical core.

Signed-off-by: Joe Konno <joe.konno(a)intel.com>
---
 src/cpu/intel_cpus.cpp | 50 +++++++++++++++++++++++++++++++++++++++++++-------
 src/cpu/intel_cpus.h   |  2 ++
 2 files changed, 45 insertions(+), 7 deletions(-)

diff --git a/src/cpu/intel_cpus.cpp b/src/cpu/intel_cpus.cpp
index 8151292238ec..330050e37473 100644
--- a/src/cpu/intel_cpus.cpp
+++ b/src/cpu/intel_cpus.cpp
@@ -70,6 +70,8 @@ static int intel_cpu_models[] = {
 	0	/* last entry must be zero */
 };
 
+static int intel_pstate_driver_loaded = -1;
+
 int is_supported_intel_cpu(int model)
 {
 	int i;
@@ -81,6 +83,34 @@ int is_supported_intel_cpu(int model)
 	return 0;
 }
 
+int is_intel_pstate_driver_loaded()
+{
+	const string filename("/sys/devices/system/cpu/cpu0/cpufreq/scaling_driver");
+	const string intel_pstate("intel_pstate");
+	char line[32] = { '\0' };
+	ifstream file;
+
+	if (intel_pstate_driver_loaded > -1)
+		return intel_pstate_driver_loaded;
+
+	file.open(filename, ios::in);
+
+	if (!file)
+		return -1;
+
+	file.getline(line, sizeof(line)-1);
+	file.close();
+
+	const string scaling_driver(line);
+	if (scaling_driver == intel_pstate) {
+		intel_pstate_driver_loaded = 1;
+	} else {
+		intel_pstate_driver_loaded = 0;
+	}
+
+	return intel_pstate_driver_loaded;
+}
+
 static uint64_t get_msr(int cpu, uint64_t offset)
 {
 	ssize_t retval;
@@ -267,10 +297,11 @@ void nhm_core::measurement_end(void)
 
 char * nhm_core::fill_pstate_line(int line_nr, char *buffer)
 {
+	const int intel_pstate = is_intel_pstate_driver_loaded();
 	buffer[0] = 0;
 	unsigned int i;
 
-	if (total_stamp ==0) {
+	if (!intel_pstate && total_stamp ==0) {
 		for (i = 0; i < pstates.size(); i++)
 			total_stamp += pstates[i]->time_after;
 		if (total_stamp == 0)
@@ -282,10 +313,11 @@ char * nhm_core::fill_pstate_line(int line_nr, char *buffer)
 		return buffer;
 	}
 
-	if (line_nr >= (int)pstates.size() || line_nr < 0)
+	if (intel_pstate > 0 || line_nr >= (int)pstates.size() || line_nr < 0)
 		return buffer;
 
 	sprintf(buffer," %5.1f%% ", percentage(1.0* (pstates[line_nr]->time_after) / total_stamp));
+
 	return buffer;
 }
 
@@ -342,10 +374,11 @@ nhm_package::nhm_package(int model)
 
 char * nhm_package::fill_pstate_line(int line_nr, char *buffer)
 {
+	const int intel_pstate = is_intel_pstate_driver_loaded();
 	buffer[0] = 0;
 	unsigned int i;
 
-	if (total_stamp ==0) {
+	if (!intel_pstate && total_stamp ==0) {
 		for (i = 0; i < pstates.size(); i++)
 			total_stamp += pstates[i]->time_after;
 		if (total_stamp == 0)
@@ -358,10 +391,11 @@ char * nhm_package::fill_pstate_line(int line_nr, char *buffer)
 		return buffer;
 	}
 
-	if (line_nr >= (int)pstates.size() || line_nr < 0)
+	if (intel_pstate > 0 || line_nr >= (int)pstates.size() || line_nr < 0)
 		return buffer;
 
 	sprintf(buffer," %5.1f%% ", percentage(1.0* (pstates[line_nr]->time_after) / total_stamp));
+
 	return buffer;
 }
 
@@ -581,7 +615,9 @@ char * nhm_cpu::fill_pstate_name(int line_nr, char *buffer)
 
 char * nhm_cpu::fill_pstate_line(int line_nr, char *buffer)
 {
-	if (total_stamp ==0) {
+	const int intel_pstate = is_intel_pstate_driver_loaded();
+
+	if (!intel_pstate && total_stamp ==0) {
 		unsigned int i;
 		for (i = 0; i < pstates.size(); i++)
 			total_stamp += pstates[i]->time_after;
@@ -600,12 +636,12 @@ char * nhm_cpu::fill_pstate_line(int line_nr, char *buffer)
 		hz_to_human(F, buffer, 1);
 		return buffer;
 	}
-	if (line_nr >= (int)pstates.size() || line_nr < 0)
+	if (intel_pstate > 0 || line_nr >= (int)pstates.size() || line_nr < 0)
 		return buffer;
 
 	sprintf(buffer," %5.1f%% ", percentage(1.0* (pstates[line_nr]->time_after) / total_stamp));
-	return buffer;
 
+	return buffer;
 }
 
 
diff --git a/src/cpu/intel_cpus.h b/src/cpu/intel_cpus.h
index 03310692a83b..d20db9ae3986 100644
--- a/src/cpu/intel_cpus.h
+++ b/src/cpu/intel_cpus.h
@@ -175,4 +175,6 @@ public:
 int is_supported_intel_cpu(int model);
 int byt_has_ahci();
 
+int is_intel_pstate_driver_loaded();
+
 #endif
-- 
2.8.3