[Powertop] [PATCH] V2: Fix C states and P states presentation issue.

[Powertop] [PATCH] V2: Fix C states and P states presentation issue.

[Austin Zhang]

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

If using '10' in this 'for' loop, for P states, once the processor
provides more than 10 P states, those additional ones cannot been
presented; and for C states, once the processor provides maximize
C states level more than 9, for example, it only has POLL, C1, C3,
C6, C10, the additional one like C10 also cannot be presented.

Now we get the loop numbers from the system information we had got
before.

Changes from V1:
Using std:max to simplify codes

Signed-off-by: Austin Zhang <zhang.austin(a)gmail.com>
---
 src/cpu/cpu.cpp |   36 +++++++++++++++++++++++++++++-------
 1 files changed, 29 insertions(+), 7 deletions(-)

diff --git a/src/cpu/cpu.cpp b/src/cpu/cpu.cpp
index 7f3af69..1bae255 100644
--- a/src/cpu/cpu.cpp
+++ b/src/cpu/cpu.cpp
@@ -402,8 +402,16 @@ void report_display_cpu_cstates(void)
 {
 	char buffer[512], buffer2[512];
 	unsigned int package, core, cpu;
-	int line;
+	int line, cstates_num;
 	class abstract_cpu *_package, * _core, * _cpu;
+	unsigned int i, j;
+
+	for (i = 0, cstates_num = 0; i < all_cpus.size(); i++) {
+		if (all_cpus[i])
+			for (j = 0; j < all_cpus[i]->cstates.size(); j++)
+				cstates_num = std::max(cstates_num,
+						(all_cpus[i]->cstates[j])->line_level);
+	}
 
 	report.begin_section(SECTION_CPUIDLE);
 	report.add_header("Processor Idle state report");
@@ -421,7 +429,7 @@ void report_display_cpu_cstates(void)
 			if (!_core)
 				continue;
 
-			for (line = LEVEL_HEADER; line < 10; line++) {
+			for (line = LEVEL_HEADER; line <= cstates_num; line++) {
 				bool first_cpu = true;
 
 				if (!_package->has_cstate_level(line))
@@ -628,14 +636,29 @@ void impl_w_display_cpu_states(int state)
 	char buffer[128];
 	char linebuf[1024];
 	unsigned int package, core, cpu;
-	int line;
+	int line, loop, cstates_num, pstates_num;
 	class abstract_cpu *_package, * _core, * _cpu;
 	int ctr = 0;
+	unsigned int i, j;
+
+	for (i = 0, cstates_num = 0, pstates_num = 0; i < all_cpus.size(); i++) {
+		if (!all_cpus[i])
+			continue;
 
-	if (state == PSTATE)
+		pstates_num = std::max<int>(pstates_num, all_cpus[i]->pstates.size());
+
+		for (j = 0; j < all_cpus[i]->cstates.size(); j++)
+			cstates_num = std::max(cstates_num,
+						(all_cpus[i]->cstates[j])->line_level);
+	}
+
+	if (state == PSTATE) {
 		win = get_ncurses_win("Frequency stats");
-	else
+		loop = pstates_num;
+	} else {
 		win = get_ncurses_win("Idle stats");
+		loop = cstates_num;
+	}
 
 	if (!win)
 		return;
@@ -656,8 +679,7 @@ void impl_w_display_cpu_states(int state)
 			if (!_core->has_pstates() && state == PSTATE)
 				continue;
 
-
-			for (line = LEVEL_HEADER; line < 10; line++) {
+			for (line = LEVEL_HEADER; line <= loop; line++) {
 				int first = 1;
 				ctr = 0;
 				linebuf[0] = 0;
-- 
1.7.5.4

Re: [Powertop] [PATCH] V2: Fix C states and P states presentation issue.

[Paul Menzel]

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

Dear Kristen, dear Austin,


Austin, thank you for the patch and Kristen, thank you for committing
it.

Am Donnerstag, den 31.01.2013, 15:39 -0800 schrieb Kristen Carlson Accardi:
> On Tue, 22 Jan 2013 19:43:32 +0800
> Austin Zhang <zhang.austin(a)gmail.com> wrote:

Austin, when sending patch iterations, please make sure to include the
version into the brackets: [PATCH v2]. Otherwise `git am` will not
remove it.

[…]

> > @@ -656,8 +679,7 @@ void impl_w_display_cpu_states(int state)
> >  			if (!_core->has_pstates() && state == PSTATE)
> >  				continue;
> >  
> > -
> > -			for (line = LEVEL_HEADER; line < 10; line++)
> > {
> > +			for (line = LEVEL_HEADER; line <= loop;
> > line++) { int first = 1;
> >  				ctr = 0;
> >  				linebuf[0] = 0;

Kristen, please do not line wrap replies (at least code pastes) for
better legibility.

> Applied, thanks.

Kristen, please double check when applying a patch. As know the commit
history contains that »V2:« confusing people looking at the commit log.
That would be great.


Thanks,

Paul

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

Re: [Powertop] [PATCH] V2: Fix C states and P states presentation issue.

[Kristen Carlson Accardi]

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

On Tue, 22 Jan 2013 19:43:32 +0800
Austin Zhang <zhang.austin(a)gmail.com> wrote:

> If using '10' in this 'for' loop, for P states, once the processor
> provides more than 10 P states, those additional ones cannot been
> presented; and for C states, once the processor provides maximize
> C states level more than 9, for example, it only has POLL, C1, C3,
> C6, C10, the additional one like C10 also cannot be presented.
> 
> Now we get the loop numbers from the system information we had got
> before.
> 
> Changes from V1:
> Using std:max to simplify codes
> 
> Signed-off-by: Austin Zhang <zhang.austin(a)gmail.com>
> ---
>  src/cpu/cpu.cpp |   36 +++++++++++++++++++++++++++++-------
>  1 files changed, 29 insertions(+), 7 deletions(-)
> 
> diff --git a/src/cpu/cpu.cpp b/src/cpu/cpu.cpp
> index 7f3af69..1bae255 100644
> --- a/src/cpu/cpu.cpp
> +++ b/src/cpu/cpu.cpp
> @@ -402,8 +402,16 @@ void report_display_cpu_cstates(void)
>  {
>  	char buffer[512], buffer2[512];
>  	unsigned int package, core, cpu;
> -	int line;
> +	int line, cstates_num;
>  	class abstract_cpu *_package, * _core, * _cpu;
> +	unsigned int i, j;
> +
> +	for (i = 0, cstates_num = 0; i < all_cpus.size(); i++) {
> +		if (all_cpus[i])
> +			for (j = 0; j < all_cpus[i]->cstates.size();
> j++)
> +				cstates_num = std::max(cstates_num,
> +
> (all_cpus[i]->cstates[j])->line_level);
> +	}
>  
>  	report.begin_section(SECTION_CPUIDLE);
>  	report.add_header("Processor Idle state report");
> @@ -421,7 +429,7 @@ void report_display_cpu_cstates(void)
>  			if (!_core)
>  				continue;
>  
> -			for (line = LEVEL_HEADER; line < 10; line++)
> {
> +			for (line = LEVEL_HEADER; line <=
> cstates_num; line++) { bool first_cpu = true;
>  
>  				if
> (!_package->has_cstate_level(line)) @@ -628,14 +636,29 @@ void
> impl_w_display_cpu_states(int state) char buffer[128];
>  	char linebuf[1024];
>  	unsigned int package, core, cpu;
> -	int line;
> +	int line, loop, cstates_num, pstates_num;
>  	class abstract_cpu *_package, * _core, * _cpu;
>  	int ctr = 0;
> +	unsigned int i, j;
> +
> +	for (i = 0, cstates_num = 0, pstates_num = 0; i <
> all_cpus.size(); i++) {
> +		if (!all_cpus[i])
> +			continue;
>  
> -	if (state == PSTATE)
> +		pstates_num = std::max<int>(pstates_num,
> all_cpus[i]->pstates.size()); +
> +		for (j = 0; j < all_cpus[i]->cstates.size(); j++)
> +			cstates_num = std::max(cstates_num,
> +
> (all_cpus[i]->cstates[j])->line_level);
> +	}
> +
> +	if (state == PSTATE) {
>  		win = get_ncurses_win("Frequency stats");
> -	else
> +		loop = pstates_num;
> +	} else {
>  		win = get_ncurses_win("Idle stats");
> +		loop = cstates_num;
> +	}
>  
>  	if (!win)
>  		return;
> @@ -656,8 +679,7 @@ void impl_w_display_cpu_states(int state)
>  			if (!_core->has_pstates() && state == PSTATE)
>  				continue;
>  
> -
> -			for (line = LEVEL_HEADER; line < 10; line++)
> {
> +			for (line = LEVEL_HEADER; line <= loop;
> line++) { int first = 1;
>  				ctr = 0;
>  				linebuf[0] = 0;

Applied, thanks.

Kristen

Re: [Powertop] [PATCH] V2: Fix C states and P states presentation issue.

[Igor Zhbanov]

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

Sergey Senozhatsky wrote:

> just for note, perhaps we can do the same for pstates_num in report_display_cpu_pstates(),
> to keep it similar.

I don't have any objection. It could be the same for pstates.
I didn't send the patch for cstates because was not sure that there
are architectures with more than 10 cstates. Austin pointed that IA is.
So both pstates and cstates handling could be fixed in the same way.

-- 
Best regards,
Igor Zhbanov,
Technical Leader,
phone: +7 (495) 797 25 00 ext 3806
e-mail: i.zhbanov(a)samsung.com

Mobile group, Moscow R&D center, Samsung Electronics
12 Dvintsev street, building 1
127018, Moscow, Russian Federation

Re: [Powertop] [PATCH] V2: Fix C states and P states presentation issue.

[Sergey Senozhatsky]

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

On (01/22/13 19:43), Austin Zhang wrote:
> Date: Tue, 22 Jan 2013 19:43:32 +0800
> From: Austin Zhang <zhang.austin(a)gmail.com>
> To: powertop(a)lists.01.org
> Subject: [Powertop] [PATCH] V2: Fix C states and P states presentation
>  issue.
> X-Mailer: git-send-email 1.7.5.4
> 
> If using '10' in this 'for' loop, for P states, once the processor
> provides more than 10 P states, those additional ones cannot been
> presented; and for C states, once the processor provides maximize
> C states level more than 9, for example, it only has POLL, C1, C3,
> C6, C10, the additional one like C10 also cannot be presented.
> 
> Now we get the loop numbers from the system information we had got
> before.
> 
> Changes from V1:
> Using std:max to simplify codes
> 
> Signed-off-by: Austin Zhang <zhang.austin(a)gmail.com>
> ---
>  src/cpu/cpu.cpp |   36 +++++++++++++++++++++++++++++-------
>  1 files changed, 29 insertions(+), 7 deletions(-)
> 
> diff --git a/src/cpu/cpu.cpp b/src/cpu/cpu.cpp
> index 7f3af69..1bae255 100644
> --- a/src/cpu/cpu.cpp
> +++ b/src/cpu/cpu.cpp
> @@ -402,8 +402,16 @@ void report_display_cpu_cstates(void)
>  {
>  	char buffer[512], buffer2[512];
>  	unsigned int package, core, cpu;
> -	int line;
> +	int line, cstates_num;
>  	class abstract_cpu *_package, * _core, * _cpu;
> +	unsigned int i, j;
> +
> +	for (i = 0, cstates_num = 0; i < all_cpus.size(); i++) {
> +		if (all_cpus[i])
> +			for (j = 0; j < all_cpus[i]->cstates.size(); j++)
> +				cstates_num = std::max(cstates_num,
> +						(all_cpus[i]->cstates[j])->line_level);
> +	}
>  
>  	report.begin_section(SECTION_CPUIDLE);
>  	report.add_header("Processor Idle state report");
> @@ -421,7 +429,7 @@ void report_display_cpu_cstates(void)
>  			if (!_core)
>  				continue;
>  
> -			for (line = LEVEL_HEADER; line < 10; line++) {
> +			for (line = LEVEL_HEADER; line <= cstates_num; line++) {
>  				bool first_cpu = true;
>  
>  				if (!_package->has_cstate_level(line))
> @@ -628,14 +636,29 @@ void impl_w_display_cpu_states(int state)
>  	char buffer[128];
>  	char linebuf[1024];
>  	unsigned int package, core, cpu;
> -	int line;
> +	int line, loop, cstates_num, pstates_num;
>  	class abstract_cpu *_package, * _core, * _cpu;
>  	int ctr = 0;
> +	unsigned int i, j;
> +
> +	for (i = 0, cstates_num = 0, pstates_num = 0; i < all_cpus.size(); i++) {
> +		if (!all_cpus[i])
> +			continue;
>  
> -	if (state == PSTATE)
> +		pstates_num = std::max<int>(pstates_num, all_cpus[i]->pstates.size());
> +


just for note, perhaps we can do the same for pstates_num in report_display_cpu_pstates(),
to keep it similar.

	-ss


> +		for (j = 0; j < all_cpus[i]->cstates.size(); j++)
> +			cstates_num = std::max(cstates_num,
> +						(all_cpus[i]->cstates[j])->line_level);
> +	}
> +
> +	if (state == PSTATE) {
>  		win = get_ncurses_win("Frequency stats");
> -	else
> +		loop = pstates_num;
> +	} else {
>  		win = get_ncurses_win("Idle stats");
> +		loop = cstates_num;
> +	}
>  
>  	if (!win)
>  		return;
> @@ -656,8 +679,7 @@ void impl_w_display_cpu_states(int state)
>  			if (!_core->has_pstates() && state == PSTATE)
>  				continue;
>  
> -
> -			for (line = LEVEL_HEADER; line < 10; line++) {
> +			for (line = LEVEL_HEADER; line <= loop; line++) {
>  				int first = 1;
>  				ctr = 0;
>  				linebuf[0] = 0;
> -- 
> 1.7.5.4
> 
> _______________________________________________
> PowerTop mailing list
> PowerTop(a)lists.01.org
> https://lists.01.org/mailman/listinfo/powertop
> 

Re: [Powertop] [PATCH] V2: Fix C states and P states presentation issue.

[Sergey Senozhatsky]

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

On (01/22/13 19:43), Austin Zhang wrote:
> If using '10' in this 'for' loop, for P states, once the processor
> provides more than 10 P states, those additional ones cannot been
> presented; and for C states, once the processor provides maximize
> C states level more than 9, for example, it only has POLL, C1, C3,
> C6, C10, the additional one like C10 also cannot be presented.
> 
> Now we get the loop numbers from the system information we had got
> before.
> 
> Changes from V1:
> Using std:max to simplify codes
> 
> Signed-off-by: Austin Zhang <zhang.austin(a)gmail.com>
> ---


looks good to me.

	-ss

>  src/cpu/cpu.cpp |   36 +++++++++++++++++++++++++++++-------
>  1 files changed, 29 insertions(+), 7 deletions(-)
> 
> diff --git a/src/cpu/cpu.cpp b/src/cpu/cpu.cpp
> index 7f3af69..1bae255 100644
> --- a/src/cpu/cpu.cpp
> +++ b/src/cpu/cpu.cpp
> @@ -402,8 +402,16 @@ void report_display_cpu_cstates(void)
>  {
>  	char buffer[512], buffer2[512];
>  	unsigned int package, core, cpu;
> -	int line;
> +	int line, cstates_num;
>  	class abstract_cpu *_package, * _core, * _cpu;
> +	unsigned int i, j;
> +
> +	for (i = 0, cstates_num = 0; i < all_cpus.size(); i++) {
> +		if (all_cpus[i])
> +			for (j = 0; j < all_cpus[i]->cstates.size(); j++)
> +				cstates_num = std::max(cstates_num,
> +						(all_cpus[i]->cstates[j])->line_level);
> +	}
>  
>  	report.begin_section(SECTION_CPUIDLE);
>  	report.add_header("Processor Idle state report");
> @@ -421,7 +429,7 @@ void report_display_cpu_cstates(void)
>  			if (!_core)
>  				continue;
>  
> -			for (line = LEVEL_HEADER; line < 10; line++) {
> +			for (line = LEVEL_HEADER; line <= cstates_num; line++) {
>  				bool first_cpu = true;
>  
>  				if (!_package->has_cstate_level(line))
> @@ -628,14 +636,29 @@ void impl_w_display_cpu_states(int state)
>  	char buffer[128];
>  	char linebuf[1024];
>  	unsigned int package, core, cpu;
> -	int line;
> +	int line, loop, cstates_num, pstates_num;
>  	class abstract_cpu *_package, * _core, * _cpu;
>  	int ctr = 0;
> +	unsigned int i, j;
> +
> +	for (i = 0, cstates_num = 0, pstates_num = 0; i < all_cpus.size(); i++) {
> +		if (!all_cpus[i])
> +			continue;
>  
> -	if (state == PSTATE)
> +		pstates_num = std::max<int>(pstates_num, all_cpus[i]->pstates.size());
> +
> +		for (j = 0; j < all_cpus[i]->cstates.size(); j++)
> +			cstates_num = std::max(cstates_num,
> +						(all_cpus[i]->cstates[j])->line_level);
> +	}
> +
> +	if (state == PSTATE) {
>  		win = get_ncurses_win("Frequency stats");
> -	else
> +		loop = pstates_num;
> +	} else {
>  		win = get_ncurses_win("Idle stats");
> +		loop = cstates_num;
> +	}
>  
>  	if (!win)
>  		return;
> @@ -656,8 +679,7 @@ void impl_w_display_cpu_states(int state)
>  			if (!_core->has_pstates() && state == PSTATE)
>  				continue;
>  
> -
> -			for (line = LEVEL_HEADER; line < 10; line++) {
> +			for (line = LEVEL_HEADER; line <= loop; line++) {
>  				int first = 1;
>  				ctr = 0;
>  				linebuf[0] = 0;
> -- 
> 1.7.5.4
> 
> _______________________________________________
> PowerTop mailing list
> PowerTop(a)lists.01.org
> https://lists.01.org/mailman/listinfo/powertop
>