All of lore.kernel.org
 help / color / mirror / Atom feed
* Re: [Powertop] [PATCH] cpufreq: only try to open files under cpuX
@ 2012-12-13 18:21 Sergey Senozhatsky
  0 siblings, 0 replies; 6+ messages in thread
From: Sergey Senozhatsky @ 2012-12-13 18:21 UTC (permalink / raw)
  To: powertop

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

On (12/13/12 09:43), Jaroslav Skarvada wrote:
> There are called many useless 'open' syscalls that always fail, like:
>   open("/sys/devices/system/cpu/uevent/cpufreq/scaling_governor", O_RDONLY) = -1 ENOTDIR (Not a directory)
> This patch filters them to 'likely' patterns, like e.g.:
> /sys/devices/system/cpu/cpuX/cpufreq/scaling_governor, where X is string
> starting with digit. For match cases it will add only small overhead.
> 
> Originally reported as:
> https://bugzilla.redhat.com/show_bug.cgi?id=886185
> 
> Signed-off-by: Jaroslav ??karvada <jskarvad(a)redhat.com>
> ---
>  src/devices/alsa.cpp       |  2 +-
>  src/devices/backlight.cpp  |  2 ++
>  src/devlist.cpp            |  3 ++-
>  src/tuning/cpufreq.cpp     | 11 ++++++-----
>  src/tuning/tuningsysfs.cpp |  5 +----
>  5 files changed, 12 insertions(+), 11 deletions(-)
>

looks good to me, thanks.

	-ss

 
> diff --git a/src/devices/alsa.cpp b/src/devices/alsa.cpp
> index 4f5d3f9..33a52f5 100644
> --- a/src/devices/alsa.cpp
> +++ b/src/devices/alsa.cpp
> @@ -167,7 +167,7 @@ void create_all_alsa(void)
>  		entry = readdir(dir);
>  		if (!entry)
>  			break;
> -		if (entry->d_name[0] == '.')
> +		if (strncmp(entry->d_name, "hwC", 3) != 0)
>  			continue;
>  		sprintf(filename, "/sys/class/sound/card0/%s/power_on_acct", entry->d_name);
>  
> diff --git a/src/devices/backlight.cpp b/src/devices/backlight.cpp
> index b8c9147..03aa5bc 100644
> --- a/src/devices/backlight.cpp
> +++ b/src/devices/backlight.cpp
> @@ -90,6 +90,8 @@ static int dpms_screen_on(void)
>  		if (!entry)
>  			break;
>  
> +		if (strncmp(entry->d_name, "card", 4) != 0)
> +			continue;
>  		sprintf(filename, "/sys/class/drm/card0/%s/enabled", entry->d_name);
>  		file.open(filename, ios::in);
>  		if (!file)
> diff --git a/src/devlist.cpp b/src/devlist.cpp
> index de5abff..633a568 100644
> --- a/src/devlist.cpp
> +++ b/src/devlist.cpp
> @@ -37,6 +37,7 @@
>  #include <sys/types.h>
>  #include <dirent.h>
>  #include <string.h>
> +#include <ctype.h>
>  
>  using namespace std;
>  
> @@ -117,7 +118,7 @@ void collect_open_devices(void)
>  			entry2 = readdir(dir2);
>  			if (!entry2)
>  				break;
> -			if (entry2->d_name[0] == '.')
> +			if (!isdigit(entry2->d_name[0]))
>  				continue;
>  			sprintf(filename, "/proc/%s/fd/%s", entry->d_name, entry2->d_name);
>  			memset(link, 0, 4096);
> diff --git a/src/tuning/cpufreq.cpp b/src/tuning/cpufreq.cpp
> index df245ad..e870559 100644
> --- a/src/tuning/cpufreq.cpp
> +++ b/src/tuning/cpufreq.cpp
> @@ -36,6 +36,7 @@
>  #include <dirent.h>
>  #include <errno.h>
>  #include <sys/stat.h>
> +#include <ctype.h>
>  
>  #include "../lib.h"
>  #include "cpufreq.h"
> @@ -72,7 +73,7 @@ int cpufreq_tunable::good_bad(void)
>  		return ret;
>  
>  	while ((dirent = readdir(dir))) {
> -		if (dirent->d_name[0]=='.')
> +		if (strncmp(dirent->d_name, "cpu", 3) != 0 || !isdigit(dirent->d_name[3]))
>  			continue;
>  		sprintf(filename, "/sys/devices/system/cpu/%s/cpufreq/scaling_governor", dirent->d_name);
>  		file = fopen(filename, "r");
> @@ -123,7 +124,7 @@ void cpufreq_tunable::toggle(void)
>  			return;
>  
>  		while ((dirent = readdir(dir))) {
> -			if (dirent->d_name[0]=='.')
> +			if (strncmp(dirent->d_name, "cpu", 3) != 0 || !isdigit(dirent->d_name[3]))
>  				continue;
>  			sprintf(filename, "/sys/devices/system/cpu/%s/cpufreq/scaling_governor", dirent->d_name);
>  			file = fopen(filename, "w");
> @@ -141,7 +142,7 @@ void cpufreq_tunable::toggle(void)
>  		return;
>  
>  	while ((dirent = readdir(dir))) {
> -		if (dirent->d_name[0]=='.')
> +		if (strncmp(dirent->d_name, "cpu", 3) != 0 || !isdigit(dirent->d_name[3]))
>  			continue;
>  		sprintf(filename, "/sys/devices/system/cpu/%s/cpufreq/scaling_governor", dirent->d_name);
>  		file = fopen(filename, "w");
> @@ -171,7 +172,7 @@ const char *cpufreq_tunable::toggle_script(void) {
>  			return NULL;
>  
>  		while ((dirent = readdir(dir))) {
> -			if (dirent->d_name[0]=='.')
> +			if (strncmp(dirent->d_name, "cpu", 3) != 0 || !isdigit(dirent->d_name[3]))
>  				continue;
>  			sprintf(filename, "/sys/devices/system/cpu/%s/cpufreq/scaling_governor", dirent->d_name);
>  			if (stat(filename, &statbuf) == -1)
> @@ -189,7 +190,7 @@ const char *cpufreq_tunable::toggle_script(void) {
>  		return NULL;
>  
>  	while ((dirent = readdir(dir))) {
> -		if (dirent->d_name[0]=='.')
> +		if (strncmp(dirent->d_name, "cpu", 3) != 0 || !isdigit(dirent->d_name[3]))
>  			continue;
>  		sprintf(filename, "/sys/devices/system/cpu/%s/cpufreq/scaling_governor", dirent->d_name);
>  		if (stat(filename, &statbuf) == -1)
> diff --git a/src/tuning/tuningsysfs.cpp b/src/tuning/tuningsysfs.cpp
> index 33d3786..ec1ca6b 100644
> --- a/src/tuning/tuningsysfs.cpp
> +++ b/src/tuning/tuningsysfs.cpp
> @@ -131,10 +131,7 @@ void add_sata_tunables(void)
>  		if (!entry)
>  			break;
>  
> -                if (strcmp(entry->d_name, ".") == 0)
> -                        continue;
> -
> -		if (strcmp(entry->d_name, "..") == 0)
> +		if (entry->d_name[0] == '.')
>  			continue;
>  
>  		sprintf(filename, "/sys/class/scsi_host/%s/link_power_management_policy", entry->d_name);
> -- 
> 1.7.11.7
> 

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


^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [Powertop] [PATCH] cpufreq: only try to open files under cpuX
@ 2012-12-13 14:43 Jaroslav Skarvada
  0 siblings, 0 replies; 6+ messages in thread
From: Jaroslav Skarvada @ 2012-12-13 14:43 UTC (permalink / raw)
  To: powertop

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

----- Original Message -----
> -
> > Hello,
> > thanks for your patch. however, I'm afraid it does not fix all the
> > cases. looking at src/tuning/cpufreq.cpp we
> > have several more places with exactly the same cpuX detection --
> > ::toggle*().
> > 
> NP, I will try to fix them all, I will post new version of the patch
> soon.
> 
> > could you also please remove 'strlen(dirent->d_name) < 4', looks
> > loke
> > a small overhead there.
> > 
> Right :)
> 
> Jaroslav

Hi,

new version of the patch is attached, I tried to fix all other apparent
cases

regards

Jaroslav

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Reduced-number-of-useless-open-syscalls.patch --]
[-- Type: text/x-patch, Size: 5120 bytes --]

From 63b20549280e606e5e73d5c9fe701c79a5abc6cf Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jaroslav=20=C5=A0karvada?= <jskarvad@redhat.com>
Date: Wed, 12 Dec 2012 16:19:56 +0100
Subject: [PATCH] Reduced number of useless 'open' syscalls
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

There are called many useless 'open' syscalls that always fail, like:
  open("/sys/devices/system/cpu/uevent/cpufreq/scaling_governor", O_RDONLY) = -1 ENOTDIR (Not a directory)
This patch filters them to 'likely' patterns, like e.g.:
/sys/devices/system/cpu/cpuX/cpufreq/scaling_governor, where X is string
starting with digit. For match cases it will add only small overhead.

Originally reported as:
https://bugzilla.redhat.com/show_bug.cgi?id=886185

Signed-off-by: Jaroslav Škarvada <jskarvad@redhat.com>
---
 src/devices/alsa.cpp       |  2 +-
 src/devices/backlight.cpp  |  2 ++
 src/devlist.cpp            |  3 ++-
 src/tuning/cpufreq.cpp     | 11 ++++++-----
 src/tuning/tuningsysfs.cpp |  5 +----
 5 files changed, 12 insertions(+), 11 deletions(-)

diff --git a/src/devices/alsa.cpp b/src/devices/alsa.cpp
index 4f5d3f9..33a52f5 100644
--- a/src/devices/alsa.cpp
+++ b/src/devices/alsa.cpp
@@ -167,7 +167,7 @@ void create_all_alsa(void)
 		entry = readdir(dir);
 		if (!entry)
 			break;
-		if (entry->d_name[0] == '.')
+		if (strncmp(entry->d_name, "hwC", 3) != 0)
 			continue;
 		sprintf(filename, "/sys/class/sound/card0/%s/power_on_acct", entry->d_name);
 
diff --git a/src/devices/backlight.cpp b/src/devices/backlight.cpp
index b8c9147..03aa5bc 100644
--- a/src/devices/backlight.cpp
+++ b/src/devices/backlight.cpp
@@ -90,6 +90,8 @@ static int dpms_screen_on(void)
 		if (!entry)
 			break;
 
+		if (strncmp(entry->d_name, "card", 4) != 0)
+			continue;
 		sprintf(filename, "/sys/class/drm/card0/%s/enabled", entry->d_name);
 		file.open(filename, ios::in);
 		if (!file)
diff --git a/src/devlist.cpp b/src/devlist.cpp
index de5abff..633a568 100644
--- a/src/devlist.cpp
+++ b/src/devlist.cpp
@@ -37,6 +37,7 @@
 #include <sys/types.h>
 #include <dirent.h>
 #include <string.h>
+#include <ctype.h>
 
 using namespace std;
 
@@ -117,7 +118,7 @@ void collect_open_devices(void)
 			entry2 = readdir(dir2);
 			if (!entry2)
 				break;
-			if (entry2->d_name[0] == '.')
+			if (!isdigit(entry2->d_name[0]))
 				continue;
 			sprintf(filename, "/proc/%s/fd/%s", entry->d_name, entry2->d_name);
 			memset(link, 0, 4096);
diff --git a/src/tuning/cpufreq.cpp b/src/tuning/cpufreq.cpp
index df245ad..e870559 100644
--- a/src/tuning/cpufreq.cpp
+++ b/src/tuning/cpufreq.cpp
@@ -36,6 +36,7 @@
 #include <dirent.h>
 #include <errno.h>
 #include <sys/stat.h>
+#include <ctype.h>
 
 #include "../lib.h"
 #include "cpufreq.h"
@@ -72,7 +73,7 @@ int cpufreq_tunable::good_bad(void)
 		return ret;
 
 	while ((dirent = readdir(dir))) {
-		if (dirent->d_name[0]=='.')
+		if (strncmp(dirent->d_name, "cpu", 3) != 0 || !isdigit(dirent->d_name[3]))
 			continue;
 		sprintf(filename, "/sys/devices/system/cpu/%s/cpufreq/scaling_governor", dirent->d_name);
 		file = fopen(filename, "r");
@@ -123,7 +124,7 @@ void cpufreq_tunable::toggle(void)
 			return;
 
 		while ((dirent = readdir(dir))) {
-			if (dirent->d_name[0]=='.')
+			if (strncmp(dirent->d_name, "cpu", 3) != 0 || !isdigit(dirent->d_name[3]))
 				continue;
 			sprintf(filename, "/sys/devices/system/cpu/%s/cpufreq/scaling_governor", dirent->d_name);
 			file = fopen(filename, "w");
@@ -141,7 +142,7 @@ void cpufreq_tunable::toggle(void)
 		return;
 
 	while ((dirent = readdir(dir))) {
-		if (dirent->d_name[0]=='.')
+		if (strncmp(dirent->d_name, "cpu", 3) != 0 || !isdigit(dirent->d_name[3]))
 			continue;
 		sprintf(filename, "/sys/devices/system/cpu/%s/cpufreq/scaling_governor", dirent->d_name);
 		file = fopen(filename, "w");
@@ -171,7 +172,7 @@ const char *cpufreq_tunable::toggle_script(void) {
 			return NULL;
 
 		while ((dirent = readdir(dir))) {
-			if (dirent->d_name[0]=='.')
+			if (strncmp(dirent->d_name, "cpu", 3) != 0 || !isdigit(dirent->d_name[3]))
 				continue;
 			sprintf(filename, "/sys/devices/system/cpu/%s/cpufreq/scaling_governor", dirent->d_name);
 			if (stat(filename, &statbuf) == -1)
@@ -189,7 +190,7 @@ const char *cpufreq_tunable::toggle_script(void) {
 		return NULL;
 
 	while ((dirent = readdir(dir))) {
-		if (dirent->d_name[0]=='.')
+		if (strncmp(dirent->d_name, "cpu", 3) != 0 || !isdigit(dirent->d_name[3]))
 			continue;
 		sprintf(filename, "/sys/devices/system/cpu/%s/cpufreq/scaling_governor", dirent->d_name);
 		if (stat(filename, &statbuf) == -1)
diff --git a/src/tuning/tuningsysfs.cpp b/src/tuning/tuningsysfs.cpp
index 33d3786..ec1ca6b 100644
--- a/src/tuning/tuningsysfs.cpp
+++ b/src/tuning/tuningsysfs.cpp
@@ -131,10 +131,7 @@ void add_sata_tunables(void)
 		if (!entry)
 			break;
 
-                if (strcmp(entry->d_name, ".") == 0)
-                        continue;
-
-		if (strcmp(entry->d_name, "..") == 0)
+		if (entry->d_name[0] == '.')
 			continue;
 
 		sprintf(filename, "/sys/class/scsi_host/%s/link_power_management_policy", entry->d_name);
-- 
1.7.11.7


^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [Powertop] [PATCH] cpufreq: only try to open files under cpuX
@ 2012-12-12 18:21 Arjan van de Ven
  0 siblings, 0 replies; 6+ messages in thread
From: Arjan van de Ven @ 2012-12-12 18:21 UTC (permalink / raw)
  To: powertop

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

On 12/12/2012 10:05 AM, Jaroslav Skarvada wrote:
> -
>> Hello,
>> thanks for your patch. however, I'm afraid it does not fix all the
>> cases. looking at src/tuning/cpufreq.cpp we
>> have several more places with exactly the same cpuX detection --
>> ::toggle*().
>>
> NP, I will try to fix them all, I will post new version of the patch soon.
>
>> could you also please remove 'strlen(dirent->d_name) < 4', looks loke
>> a small overhead there.
>>
> Right :)

it makes sense to clean this up indeed

now, I'm still wanting to rewrite the whole frequency side... it's not great code right now,
and makes assumptions that aren't going to be true going forward.
but until that's done.. by all means lets do improvements like this to the existing code.



^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [Powertop] [PATCH] cpufreq: only try to open files under cpuX
@ 2012-12-12 18:05 Jaroslav Skarvada
  0 siblings, 0 replies; 6+ messages in thread
From: Jaroslav Skarvada @ 2012-12-12 18:05 UTC (permalink / raw)
  To: powertop

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

-
> Hello,
> thanks for your patch. however, I'm afraid it does not fix all the
> cases. looking at src/tuning/cpufreq.cpp we
> have several more places with exactly the same cpuX detection --
> ::toggle*().
> 
NP, I will try to fix them all, I will post new version of the patch soon.

> could you also please remove 'strlen(dirent->d_name) < 4', looks loke
> a small overhead there.
> 
Right :)

Jaroslav

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [Powertop] [PATCH] cpufreq: only try to open files under cpuX
@ 2012-12-12 17:56 Sergey Senozhatsky
  0 siblings, 0 replies; 6+ messages in thread
From: Sergey Senozhatsky @ 2012-12-12 17:56 UTC (permalink / raw)
  To: powertop

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

On (12/12/12 17:16), Jaroslav Škarvada wrote:
> There are called many useless 'open' syscalls that always fail, like:
>   open("/sys/devices/system/cpu/uevent/cpufreq/scaling_governor", O_RDONLY) = -1 ENOTDIR (Not a directory)
> This patch filters this to /sys/devices/system/cpu/cpuX, where X is string
> starting with digit (from kernel sources it seems that X can only be integer).
> For match cases it will add only small overhead.
> 
> Originally reported as:
> https://bugzilla.redhat.com/show_bug.cgi?id=886185
> 
> Signed-off-by: Jaroslav Škarvada <jskarvad(a)redhat.com>
> ---
>  src/tuning/cpufreq.cpp | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/src/tuning/cpufreq.cpp b/src/tuning/cpufreq.cpp
> index df245ad..c448f4c 100644
> --- a/src/tuning/cpufreq.cpp
> +++ b/src/tuning/cpufreq.cpp
> @@ -36,6 +36,7 @@
>  #include <dirent.h>
>  #include <errno.h>
>  #include <sys/stat.h>
> +#include <ctype.h>
>  
>  #include "../lib.h"
>  #include "cpufreq.h"
> @@ -72,7 +73,7 @@ int cpufreq_tunable::good_bad(void)
>  		return ret;
>  
>  	while ((dirent = readdir(dir))) {
> -		if (dirent->d_name[0]=='.')
> +		if (strlen(dirent->d_name) < 4 || strncmp(dirent->d_name, "cpu", 3) != 0 || !isdigit(dirent->d_name[3]))
>  			continue;
>  		sprintf(filename, "/sys/devices/system/cpu/%s/cpufreq/scaling_governor", dirent->d_name);
>  		file = fopen(filename, "r");
> -- 
> 1.7.11.7
> 

Hello,
thanks for your patch. however, I'm afraid it does not fix all the cases. looking at src/tuning/cpufreq.cpp we
have several more places with exactly the same cpuX detection -- ::toggle*().

could you also please remove 'strlen(dirent->d_name) < 4', looks loke a small overhead there.

	-ss

^ permalink raw reply	[flat|nested] 6+ messages in thread
* [Powertop] [PATCH] cpufreq: only try to open files under cpuX
@ 2012-12-12 16:16 
  0 siblings, 0 replies; 6+ messages in thread
From:  @ 2012-12-12 16:16 UTC (permalink / raw)
  To: powertop

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

There are called many useless 'open' syscalls that always fail, like:
  open("/sys/devices/system/cpu/uevent/cpufreq/scaling_governor", O_RDONLY) = -1 ENOTDIR (Not a directory)
This patch filters this to /sys/devices/system/cpu/cpuX, where X is string
starting with digit (from kernel sources it seems that X can only be integer).
For match cases it will add only small overhead.

Originally reported as:
https://bugzilla.redhat.com/show_bug.cgi?id=886185

Signed-off-by: Jaroslav Škarvada <jskarvad(a)redhat.com>
---
 src/tuning/cpufreq.cpp | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/tuning/cpufreq.cpp b/src/tuning/cpufreq.cpp
index df245ad..c448f4c 100644
--- a/src/tuning/cpufreq.cpp
+++ b/src/tuning/cpufreq.cpp
@@ -36,6 +36,7 @@
 #include <dirent.h>
 #include <errno.h>
 #include <sys/stat.h>
+#include <ctype.h>
 
 #include "../lib.h"
 #include "cpufreq.h"
@@ -72,7 +73,7 @@ int cpufreq_tunable::good_bad(void)
 		return ret;
 
 	while ((dirent = readdir(dir))) {
-		if (dirent->d_name[0]=='.')
+		if (strlen(dirent->d_name) < 4 || strncmp(dirent->d_name, "cpu", 3) != 0 || !isdigit(dirent->d_name[3]))
 			continue;
 		sprintf(filename, "/sys/devices/system/cpu/%s/cpufreq/scaling_governor", dirent->d_name);
 		file = fopen(filename, "r");
-- 
1.7.11.7


^ permalink raw reply related	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-12-13 18:21 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-12-13 18:21 [Powertop] [PATCH] cpufreq: only try to open files under cpuX Sergey Senozhatsky
  -- strict thread matches above, loose matches on Subject: below --
2012-12-13 14:43 Jaroslav Skarvada
2012-12-12 18:21 Arjan van de Ven
2012-12-12 18:05 Jaroslav Skarvada
2012-12-12 17:56 Sergey Senozhatsky
2012-12-12 16:16 

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.