Re: [Powertop] [PATCH] Fix running failure when > 69 CPUs for open file limitation

Re: [Powertop] [PATCH] Fix running failure when > 69 CPUs for open file limitation

[Sergey Senozhatsky]

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

On (05/29/13 11:44), Youquan Song wrote:
> Powertop 2.3 fails to run on machine with > 69 CPUs because system open files
> limitation of one process is 1024 default,  While powertop will open one file
> for every monitored perf_event (at least 15) each CPU.
> 
> Like on 80 CPUs Westmere-EX machine, powertop will fail to run with below:
> 
> PowerTOP v2.3 needs the kernel to support the 'perf' subsystem
> as well as support for trace points in the kernel:
> CONFIG_PERF_EVENTS=y
> CONFIG_PERF_COUNTERS=y
> CONFIG_TRACEPOINTS=y
> CONFIG_TRACING=y
> 
> This patch is to change RLIMIT_NOFILE from default (1024) to max limition.
> 
> 

Hello,
I'd like to ask you to try out the following patch. We leak fd in read_file().




------8<--------8<--------

- Do not leak file descriptor in perf_bundle read_file().
- Clear perf event (unmap memory and close fd) on event destruction.


Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky(a)gmail.com>

---

diff --git a/src/perf/perf.cpp b/src/perf/perf.cpp
index 35b4017..e919741 100644
--- a/src/perf/perf.cpp
+++ b/src/perf/perf.cpp
@@ -167,6 +167,7 @@ perf_event::~perf_event(void)
 	if (perf_event::pevent->ref_count == 1) {
 		pevent_free(perf_event::pevent);
 		perf_event::pevent = NULL;
+		clear();
 	} else
 		pevent_unref(perf_event::pevent);
 }
diff --git a/src/perf/perf_bundle.cpp b/src/perf/perf_bundle.cpp
index 38e1e91..8d480e8 100644
--- a/src/perf/perf_bundle.cpp
+++ b/src/perf/perf_bundle.cpp
@@ -123,6 +123,7 @@ static char * read_file(const char *file)
 		buffer[len] = '\0';
 	}
 out:
+	close(fd);
 	return buffer;
 }
 

Re: [Powertop] [PATCH] Fix running failure when > 69 CPUs for open file limitation

[Sergey Senozhatsky]

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

On (06/04/13 17:31), Youquan Song wrote:
> > 
> > how about, say, 2048? unlimited resource steals possibility to detect 
> > error/bug/leak/etc.
> 
> I do not like it. If there are more CPUs on larger machine and if we
> want to monitor more performance event for each CPU, the 2048 settting will
>  be another issue.
> 

we may get the number of CPUs, multiply it by events number + probably add some 
extra fds, and set it as a limit. this is not a really big issue, though. my point 
is that limited resource gives us better chances to detect resource leak in 
powertop, however I agree that we just may fireup valgrind for that purpose.


	-ss

> We just set it to the rlim_cur to rlim_max for the powertop application
> and to cater to powertop requirement as large as possible.
> 
> > and let's make rlimit change after checkroot() call.
> > 
> 
> Good point. Agree. Here is the modified patch.
> 
> 
> diff --git a/src/main.cpp b/src/main.cpp
> index 0883424..d24a3db 100644
> --- a/src/main.cpp
> +++ b/src/main.cpp
> @@ -36,6 +36,7 @@
>  #include <getopt.h>
>  #include <unistd.h>
>  #include <locale.h>
> +#include <sys/resource.h>
>  
>  #include "cpu/cpu.h"
>  #include "process/process.h"
> @@ -283,11 +284,17 @@ static void powertop_init(void)
>  	static char initialized = 0;
>  	int ret;
>  	struct statfs st_fs;
> +	struct rlimit rlmt;
>  
>  	if (initialized)
>  		return;
>  
>  	checkroot();
> +
> +	getrlimit (RLIMIT_NOFILE, &rlmt);
> +	rlmt.rlim_cur = rlmt.rlim_max;
> +	setrlimit (RLIMIT_NOFILE, &rlmt);
> +
>  	ret = system("/sbin/modprobe cpufreq_stats > /dev/null 2>&1");
>  	ret = system("/sbin/modprobe msr > /dev/null 2>&1");
>  	statfs("/sys/kernel/debug", &st_fs);
> 
>  
> 

Re: [Powertop] [PATCH] Fix running failure when > 69 CPUs for open file limitation

[Sergey Senozhatsky]

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

On (06/04/13 17:22), Youquan Song wrote:
> > 
> > Hello,
> > I'd like to ask you to try out the following patch. We leak fd in read_file().
> > 
> 
> No. below patch does not fix the issue. The issue happen at open too
> many files at beginning.
> 

Well, the patch is a separate issue not addressing the rlimit problem directly. 
We just leak fd in read_file().

	-ss


> Thanks
> -Youquan
> 
> 
> > 
> > 
> > ------8<--------8<--------
> > 
> > - Do not leak file descriptor in perf_bundle read_file().
> > - Clear perf event (unmap memory and close fd) on event destruction.
> > 
> > 
> > Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky(a)gmail.com>
> > 
> > ---
> > 
> > diff --git a/src/perf/perf.cpp b/src/perf/perf.cpp
> > index 35b4017..e919741 100644
> > --- a/src/perf/perf.cpp
> > +++ b/src/perf/perf.cpp
> > @@ -167,6 +167,7 @@ perf_event::~perf_event(void)
> >  	if (perf_event::pevent->ref_count == 1) {
> >  		pevent_free(perf_event::pevent);
> >  		perf_event::pevent = NULL;
> > +		clear();
> >  	} else
> >  		pevent_unref(perf_event::pevent);
> >  }
> > diff --git a/src/perf/perf_bundle.cpp b/src/perf/perf_bundle.cpp
> > index 38e1e91..8d480e8 100644
> > --- a/src/perf/perf_bundle.cpp
> > +++ b/src/perf/perf_bundle.cpp
> > @@ -123,6 +123,7 @@ static char * read_file(const char *file)
> >  		buffer[len] = '\0';
> >  	}
> >  out:
> > +	close(fd);
> >  	return buffer;
> >  }
> >  
> > 
> 

Re: [Powertop] [PATCH] Fix running failure when > 69 CPUs for open file limitation

[Sergey Senozhatsky]

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

On (05/29/13 11:44), Youquan Song wrote:
> Powertop 2.3 fails to run on machine with > 69 CPUs because system open files
> limitation of one process is 1024 default,  While powertop will open one file
> for every monitored perf_event (at least 15) each CPU.
>

how about, say, 2048? unlimited resource steals possibility to detect 
error/bug/leak/etc.


and let's make rlimit change after checkroot() call.

	-ss

 
> Like on 80 CPUs Westmere-EX machine, powertop will fail to run with below:
> 
> PowerTOP v2.3 needs the kernel to support the 'perf' subsystem
> as well as support for trace points in the kernel:
> CONFIG_PERF_EVENTS=y
> CONFIG_PERF_COUNTERS=y
> CONFIG_TRACEPOINTS=y
> CONFIG_TRACING=y
> 
> This patch is to change RLIMIT_NOFILE from default (1024) to max limition.
> 
> 
> Signed-off-by: Youquan Song <youquan.song(a)intel.com>
> ---
>  src/main.cpp |    6 ++++++
>  1 files changed, 6 insertions(+), 0 deletions(-)
> 
> diff --git a/src/main.cpp b/src/main.cpp
> index 0883424..8cd97a2 100644
> --- a/src/main.cpp
> +++ b/src/main.cpp
> @@ -36,6 +36,7 @@
>  #include <getopt.h>
>  #include <unistd.h>
>  #include <locale.h>
> +#include <sys/resource.h>
>  
>  #include "cpu/cpu.h"
>  #include "process/process.h"
> @@ -283,9 +284,14 @@ static void powertop_init(void)
>  	static char initialized = 0;
>  	int ret;
>  	struct statfs st_fs;
> +	struct rlimit rlmt;
>  
>  	if (initialized)
>  		return;
> +
> +	getrlimit (RLIMIT_NOFILE, &rlmt);
> +	rlmt.rlim_cur = rlmt.rlim_max;
> +	setrlimit (RLIMIT_NOFILE, &rlmt);
>  
>  	checkroot();
>  	ret = system("/sbin/modprobe cpufreq_stats > /dev/null 2>&1");
>

[Powertop] [PATCH] Fix running failure when > 69 CPUs for open file limitation

[Youquan Song]

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

Powertop 2.3 fails to run on machine with > 69 CPUs because system open files
limitation of one process is 1024 default,  While powertop will open one file
for every monitored perf_event (at least 15) each CPU.

Like on 80 CPUs Westmere-EX machine, powertop will fail to run with below:

PowerTOP v2.3 needs the kernel to support the 'perf' subsystem
as well as support for trace points in the kernel:
CONFIG_PERF_EVENTS=y
CONFIG_PERF_COUNTERS=y
CONFIG_TRACEPOINTS=y
CONFIG_TRACING=y

This patch is to change RLIMIT_NOFILE from default (1024) to max limition.


Signed-off-by: Youquan Song <youquan.song(a)intel.com>
---
 src/main.cpp |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/src/main.cpp b/src/main.cpp
index 0883424..8cd97a2 100644
--- a/src/main.cpp
+++ b/src/main.cpp
@@ -36,6 +36,7 @@
 #include <getopt.h>
 #include <unistd.h>
 #include <locale.h>
+#include <sys/resource.h>
 
 #include "cpu/cpu.h"
 #include "process/process.h"
@@ -283,9 +284,14 @@ static void powertop_init(void)
 	static char initialized = 0;
 	int ret;
 	struct statfs st_fs;
+	struct rlimit rlmt;
 
 	if (initialized)
 		return;
+
+	getrlimit (RLIMIT_NOFILE, &rlmt);
+	rlmt.rlim_cur = rlmt.rlim_max;
+	setrlimit (RLIMIT_NOFILE, &rlmt);
 
 	checkroot();
 	ret = system("/sbin/modprobe cpufreq_stats > /dev/null 2>&1");
-- 
1.7.7.4