Re: [Powertop] patch: tell user if the system is running out of FDs

Re: [Powertop] patch: tell user if the system is running out of FDs

[Jaroslav Skarvada]

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



----- 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/4cbb957605818dc7e4b7932dafac0aad5ed0b87a
> 
> (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

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: powertop-2.4-unlimit-fds.patch --]
[-- Type: text/x-patch, Size: 730 bytes --]

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);

Re: [Powertop] patch: tell user if the system is running out of FDs

[Sergey Senozhatsky]

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

On (10/08/13 08:56), Jaroslav Skarvada wrote:
> Date: Tue, 8 Oct 2013 08:56:43 -0400 (EDT)
> From: Jaroslav Skarvada <jskarvad(a)redhat.com>
> To: Sergey Senozhatsky <sergey.senozhatsky(a)gmail.com>
> 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>
> > >   */
> > >  #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

fair enough. I'm gonna apply it through the -next tree (the
patch requires some modification to apply).

thanks.

	-ss

Re: [Powertop] patch: tell user if the system is running out of FDs

[Jaroslav Skarvada]

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



----- 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>
> >   */
> >  #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

JS

Re: [Powertop] patch: tell user if the system is running out of FDs

[Sergey Senozhatsky]

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

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>
>   */
>  #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()?

	-ss

> +		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);

Re: [Powertop] patch: tell user if the system is running out of FDs

[Jaroslav Skarvada]

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

> 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

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: powertop-2.4-unlimit-fds.patch --]
[-- Type: text/x-patch, Size: 1335 bytes --]

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@linux.intel.com>
  */
 #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);

Re: [Powertop] patch: tell user if the system is running out of FDs

[Sergey Senozhatsky]

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

On (10/07/13 09:12), Jaroslav Skarvada wrote:
> ----- 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/4cbb957605818dc7e4b7932dafac0aad5ed0b87a
> > 
> > (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,

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;
> +	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: tell user if the system is running out of FDs

[Sergey Senozhatsky]

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

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/4cbb957605818dc7e4b7932dafac0aad5ed0b87a

(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

> 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

[Powertop] patch: tell user if the system is running out of FDs

[Jaroslav Skarvada]

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

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

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: powertop-2.4-fd-limit-err.patch --]
[-- Type: text/x-patch, Size: 1510 bytes --]

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) {