Hi, complex servers can run out of FDs easily (due to perf). In such case tell user that more FDs are needed instead of the generic message (that kernel doesn't support the perf)

regards Jaroslav

diff -up powertop-2.4/src/perf/perf.cpp.orig powertop-2.4/src/perf/perf.cpp --- powertop-2.4/src/perf/perf.cpp.orig 2013-01-31 16:39:27.000000000 -0500 +++ powertop-2.4/src/perf/perf.cpp 2013-09-19 10:36:02.298839248 -0400 @@ -26,6 +26,7 @@ #include <iostream> #include <fstream> +#include <errno.h> #include <unistd.h> #include <stdio.h> #include <stdlib.h> @@ -72,6 +73,7 @@ void perf_event::create_perf_event(char { struct perf_event_attr attr; int ret; + int err; struct { __u64 count; @@ -107,10 +109,15 @@ void perf_event::create_perf_event(char perf_fd = sys_perf_event_open(&attr, -1, _cpu, -1, 0); if (perf_fd < 0) { + err = errno; reset_display(); - fprintf(stderr, _("PowerTOP %s needs the kernel to support the 'perf' subsystem\n"), POWERTOP_VERSION); - fprintf(stderr, _("as well as support for trace points in the kernel:\n")); - fprintf(stderr, "CONFIG_PERF_EVENTS=y\nCONFIG_PERF_COUNTERS=y\nCONFIG_TRACEPOINTS=y\nCONFIG_TRACING=y\n"); + if (err == EMFILE) + fprintf(stderr, _("Too many open files, please increase the limit of open file descriptors.\n")); + else { + fprintf(stderr, _("PowerTOP %s needs the kernel to support the 'perf' subsystem\n"), POWERTOP_VERSION); + fprintf(stderr, _("as well as support for trace points in the kernel:\n")); + fprintf(stderr, "CONFIG_PERF_EVENTS=y\nCONFIG_PERF_COUNTERS=y\nCONFIG_TRACEPOINTS=y\nCONFIG_TRACING=y\n"); + } exit(EXIT_FAILURE); } if (read(perf_fd, &read_data, sizeof(read_data)) == -1) {

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

----- Original Message ----- > On (09/19/13 11:23), Jaroslav Skarvada wrote: > > Hi, > > > > complex servers can run out of FDs easily (due to perf). > > In such case tell user that more FDs are needed instead > > of the generic message (that kernel doesn't support > > the perf) > > Hello Jaroslav, > Thank you for your patch. > > Youquan Song proposed to change RLIMIT_NOFILE in "[PATCH] Fix running > failure when > 69 CPUs for open file limitation" some time ago > (v1 of the patch > https://lists.01.org/pipermail/powertop/2013-May/000850.html) > > after some thinking I've merged Youquan's patch to powertop2-next tree > https://github.com/sergey-senozhatsky/powertop2-next/commit/4cbb957605818... > > (not upstreamed yet). > > while I don't really want to unlimit fds, at the same time from the user's > point of view it'd better to handle it `transparently'. I think most likely > user > will react with `ulimit -n unlimited' to any powertop rlimited fds message, > since > he/she really does not control (+does not care about) the number of opened > fds. > > thanks, > -ss > Hi,

I used Youquan's patch, but we have noticed that it can only overcome soft limits, but not hard limits. E.g. on Fedora (and probably others) there is a soft limit of 1024 and a hard limit of 4096, so if powertop opens at least 15 file descriptors per CPU, then this will fail on a system with >273 CPUs.

In case you think we can also temporally increase the hard limit (as we already have the privilege), the attached patch will do it. In case you think the hard limits set by the system administrator are untouchable (e.g. violation of system resources hard limits could result in a bad effects like explosion in a nuclear power plant :), the previously sent patch that fixes the error message needs to be also used

regards Jaroslav

diff --git a/src/main.cpp b/src/main.cpp index 0883424..e0e8a71 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -36,6 +36,8 @@ #include <getopt.h> #include <unistd.h> #include <locale.h> +#include <sys/resource.h> +#include <linux/limits.h> #include "cpu/cpu.h" #include "process/process.h" @@ -283,11 +285,16 @@ static void powertop_init(void) static char initialized = 0; int ret; struct statfs st_fs; + struct rlimit rlmt; if (initialized) return; checkroot(); + + rlmt.rlim_cur = rlmt.rlim_max = NR_OPEN; + 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);

> Hello, > > > I used Youquan's patch, but we have noticed that it can only overcome soft > > limits, > > but not hard limits. E.g. on Fedora (and probably others) there is a soft > > limit > > of 1024 and a hard limit of 4096, so if powertop opens at least 15 file > > descriptors > > per CPU, then this will fail on a system with >273 CPUs. > > that's a pretty big system > > > In case you think we can also temporally increase the hard limit (as we > > already have > > the privilege), the attached patch will do it. In case you think the hard > > limits set by > > the system administrator are untouchable (e.g. violation of system > > resources hard limits > > could result in a bad effects like explosion in a nuclear power plant :), > > the > > previously sent patch that fixes the error message needs to be also used > > hm, need to think about this for a moment. both your patches make sense. > I'd probably go with `temporally increase the hard limit' solution, to make > someones life easier. what do you, guys, think? > > > -ss > > > regards > > > > Jaroslav > > > diff --git a/src/main.cpp b/src/main.cpp > > index 0883424..e0e8a71 100644 > > --- a/src/main.cpp > > +++ b/src/main.cpp > > @@ -36,6 +36,8 @@ > > #include <getopt.h> > > #include <unistd.h> > > #include <locale.h> > > +#include <sys/resource.h> > > +#include <linux/limits.h> > > > > #include "cpu/cpu.h" > > #include "process/process.h" > > @@ -283,11 +285,16 @@ static void powertop_init(void) > > static char initialized = 0; > > int ret; > > struct statfs st_fs; > > + struct rlimit rlmt; > > > > if (initialized) > > return; > > > > checkroot(); > > + > > + rlmt.rlim_cur = rlmt.rlim_max = NR_OPEN; This seems to be wrong. On my system NR_OPEN is 1024, but /proc/sys/fs/nr_open gives me 1048576. It seems the linux kernel hardcodes 1024 * 1024 as the default kernel limit. This default limit can be raised to: sysctl_nr_open_max = min((size_t)INT_MAX, ~(size_t)0/sizeof(void *)) & -BITS_PER_LONG; which is 2 Gi - 63 on my 64 bit system. I think we shouldn't touch/increase this. It seems there is no header definition for these limits. I am proposing new patch, which reads the current kernel limit from the /proc. I am not sure about the portability, but at least it shouldn't break anything. Finally I think, the first patch could be applied together with this patch (probably with the changed message) to notify user that he/she reached the FDs limit, because there is still some limit. This could also occur if some process would decrease the /proc/sys/fs/nr_open after it is read by powertop and before the ulimit is set regards Jaroslav

diff --git a/src/main.cpp b/src/main.cpp index 0883424..16b1613 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -28,6 +28,7 @@ * Arjan van de Ven <arjan(a)linux.intel.com&gt; */ #include <iostream> +#include <fstream> #include <stdlib.h> #include <stdio.h> #include <time.h> @@ -36,6 +37,7 @@ #include <getopt.h> #include <unistd.h> #include <locale.h> +#include <sys/resource.h> #include "cpu/cpu.h" #include "process/process.h" @@ -60,6 +62,8 @@ #define DEBUGFS_MAGIC 0x64626720 +#define NR_OPEN_DEF 1024 * 1024 int debug_learning = 0; unsigned time_out = 20; int leave_powertop = 0; @@ -278,16 +282,35 @@ static void checkroot() { } +static int get_nr_open(void) { + int nr_open = NR_OPEN_DEF; + ifstream file; + + file.open("/proc/sys/fs/nr_open", ios::in); + if (file) { + file >> nr_open; + if (!file) + nr_open = NR_OPEN_DEF;

+ file.close(); + } + return nr_open; +} + static void powertop_init(void) { static char initialized = 0; int ret; struct statfs st_fs; + struct rlimit rlmt; if (initialized) return; checkroot(); + + rlmt.rlim_cur = rlmt.rlim_max = get_nr_open(); + 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);

Date: Tue, 8 Oct 2013 08:56:43 -0400 (EDT) From: Jaroslav Skarvada <jskarvad(a)redhat.com&gt; To: Sergey Senozhatsky <sergey.senozhatsky(a)gmail.com&gt; Cc: powertop(a)ml01.01.org Subject: Re: [Powertop] patch: tell user if the system is running out of FDs X-Mailer: Zimbra 8.0.3_GA_5664 (ZimbraWebClient - FF24 (Linux)/8.0.3_GA_5664) ----- Original Message ----- > On (10/08/13 08:08), Jaroslav Skarvada wrote: > > > Hello, > > > > > > > I used Youquan's patch, but we have noticed that it can only overcome > > > > soft > > > > limits, > > > > but not hard limits. E.g. on Fedora (and probably others) there is a > > > > soft > > > > limit > > > > of 1024 and a hard limit of 4096, so if powertop opens at least 15 file > > > > descriptors > > > > per CPU, then this will fail on a system with >273 CPUs. > > > > > > that's a pretty big system > > > > > > > In case you think we can also temporally increase the hard limit (as we > > > > already have > > > > the privilege), the attached patch will do it. In case you think the > > > > hard > > > > limits set by > > > > the system administrator are untouchable (e.g. violation of system > > > > resources hard limits > > > > could result in a bad effects like explosion in a nuclear power plant > > > > :), > > > > the > > > > previously sent patch that fixes the error message needs to be also > > > > used > > > > > > hm, need to think about this for a moment. both your patches make sense. > > > I'd probably go with `temporally increase the hard limit' solution, to > > > make > > > someones life easier. what do you, guys, think? > > > > > > > > > -ss > > > > > > > regards > > > > > > > > Jaroslav > > > > > > > diff --git a/src/main.cpp b/src/main.cpp > > > > index 0883424..e0e8a71 100644 > > > > --- a/src/main.cpp > > > > +++ b/src/main.cpp > > > > @@ -36,6 +36,8 @@ > > > > #include <getopt.h> > > > > #include <unistd.h> > > > > #include <locale.h> > > > > +#include <sys/resource.h> > > > > +#include <linux/limits.h> > > > > > > > > #include "cpu/cpu.h" > > > > #include "process/process.h" > > > > @@ -283,11 +285,16 @@ static void powertop_init(void) > > > > static char initialized = 0; > > > > int ret; > > > > struct statfs st_fs; > > > > + struct rlimit rlmt; > > > > > > > > if (initialized) > > > > return; > > > > > > > > checkroot(); > > > > + > > > > + rlmt.rlim_cur = rlmt.rlim_max = NR_OPEN; > > > > This seems to be wrong. On my system NR_OPEN is 1024, but > > /proc/sys/fs/nr_open gives me 1048576. It seems the linux kernel > > hardcodes 1024 * 1024 as the default kernel limit. This default > > limit can be raised to: > > > > sysctl_nr_open_max = min((size_t)INT_MAX, ~(size_t)0/sizeof(void *)) & > > -BITS_PER_LONG; > > > > which is 2 Gi - 63 on my 64 bit system. I think we shouldn't > > touch/increase this. It seems there is no header definition for these > > limits. > > > > I am proposing new patch, which reads the current kernel limit from the > > /proc. I am not sure about the portability, but at least it shouldn't > > break anything. > > > > Finally I think, the first patch could be applied together with this patch > > (probably with the changed message) to notify user that he/she reached > > the FDs limit, because there is still some limit. This could also > > occur if some process would decrease the /proc/sys/fs/nr_open after > > it is read by powertop and before the ulimit is set > > > > regards > > > > Jaroslav > > > diff --git a/src/main.cpp b/src/main.cpp > > index 0883424..16b1613 100644 > > --- a/src/main.cpp > > +++ b/src/main.cpp > > @@ -28,6 +28,7 @@ > > * Arjan van de Ven <arjan(a)linux.intel.com&gt; > > */ > > #include <iostream> > > +#include <fstream> > > #include <stdlib.h> > > #include <stdio.h> > > #include <time.h> > > @@ -36,6 +37,7 @@ > > #include <getopt.h> > > #include <unistd.h> > > #include <locale.h> > > +#include <sys/resource.h> > > > > #include "cpu/cpu.h" > > #include "process/process.h" > > @@ -60,6 +62,8 @@ > > > > #define DEBUGFS_MAGIC 0x64626720 > > > > +#define NR_OPEN_DEF 1024 * 1024 > > int debug_learning = 0; > > unsigned time_out = 20; > > int leave_powertop = 0; > > @@ -278,16 +282,35 @@ static void checkroot() { > > > > } > > > > +static int get_nr_open(void) { > > + int nr_open = NR_OPEN_DEF; > > + ifstream file; > > + > > + file.open("/proc/sys/fs/nr_open", ios::in); > > + if (file) { > > + file >> nr_open; > > + if (!file) > > + nr_open = NR_OPEN_DEF; > > > what if /proc/sys/fs/nr_open is less than 4096 or (NR_OPEN_DEF)? > you probaly want max()? I think you cannot set ulimit beyond /proc/sys/fs/nr_open. You need to increase /proc/sys/fs/nr_open first, which is something we should avoid