[PATCH 1/1 tip] perf top: Move hard coded list to /etc/perfconfig/symbols.skip

[PATCH 1/1 tip] perf top: Move hard coded list to /etc/perfconfig/symbols.skip

[Arnaldo Carvalho de Melo]

This also paves the way to add more symbol lists to be filtered out,
spinlock anyone?

Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Mike Galbraith <efault@gmx.de>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Paul Mackerras <paulus@samba.org>
Cc: H. Peter Anvin <hpa@zytor.com>
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/Makefile       |    6 ++++--
 tools/perf/builtin-top.c  |   26 +++++++-------------------
 tools/perf/util/strlist.c |   10 ++++++++--
 3 files changed, 19 insertions(+), 23 deletions(-)

diff --git a/tools/perf/Makefile b/tools/perf/Makefile
index a5e9b87..385a1bc 100644
--- a/tools/perf/Makefile
+++ b/tools/perf/Makefile
@@ -193,11 +193,10 @@ template_dir = share/perf-core/templates
 htmldir = share/doc/perf-doc
 ifeq ($(prefix),/usr)
 sysconfdir = /etc
-ETC_PERFCONFIG = $(sysconfdir)/perfconfig
 else
 sysconfdir = $(prefix)/etc
-ETC_PERFCONFIG = etc/perfconfig
 endif
+ETC_PERFCONFIG = $(sysconfdir)/perfconfig
 lib = lib
 # DESTDIR=
 
@@ -596,6 +595,7 @@ PERL_PATH_SQ = $(subst ','\'',$(PERL_PATH))
 LIBS = $(PERFLIBS) $(EXTLIBS)
 
 BASIC_CFLAGS += -DSHA1_HEADER='$(SHA1_HEADER_SQ)' \
+		-DPERF_ETC_PATH='"$(ETC_PERFCONFIG)"' \
 	$(COMPAT_CFLAGS)
 LIB_OBJS += $(COMPAT_OBJS)
 
@@ -804,6 +804,8 @@ export perfexec_instdir
 install: all
 	$(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(bindir_SQ)'
 	$(INSTALL) perf$X '$(DESTDIR_SQ)$(bindir_SQ)'
+	$(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(ETC_PERFCONFIG_SQ)'
+	$(INSTALL) symbols.skip '$(DESTDIR_SQ)$(ETC_PERFCONFIG_SQ)'
 ifdef BUILT_INS
 	$(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(perfexec_instdir_SQ)'
 	$(INSTALL) $(BUILT_INS) '$(DESTDIR_SQ)$(perfexec_instdir_SQ)'
diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
index f139f1a..628ef29 100644
--- a/tools/perf/builtin-top.c
+++ b/tools/perf/builtin-top.c
@@ -20,6 +20,7 @@
 
 #include "perf.h"
 
+#include "util/strlist.h"
 #include "util/symbol.h"
 #include "util/color.h"
 #include "util/util.h"
@@ -279,24 +280,12 @@ static void *display_thread(void *arg __used)
 }
 
 /* Tag samples to be skipped. */
-static const char *skip_symbols[] = {
-	"default_idle",
-	"cpu_idle",
-	"enter_idle",
-	"exit_idle",
-	"mwait_idle",
-	"mwait_idle_with_hints",
-	"ppc64_runlatch_off",
-	"pseries_dedicated_idle_sleep",
-	NULL
-};
+static struct strlist *skip_symbols;
 
 static int symbol_filter(struct dso *self, struct symbol *sym)
 {
 	static int filter_match;
-	struct sym_entry *syme;
 	const char *name = sym->name;
-	int i;
 
 	/*
 	 * ppc64 uses function descriptors and appends a '.' to the
@@ -314,12 +303,9 @@ static int symbol_filter(struct dso *self, struct symbol *sym)
 	    strstr(name, "_text_end"))
 		return 1;
 
-	syme = dso__sym_priv(self, sym);
-	for (i = 0; skip_symbols[i]; i++) {
-		if (!strcmp(skip_symbols[i], name)) {
-			syme->skip = 1;
-			break;
-		}
+	if (strlist__has_entry(skip_symbols, name)) {
+		struct sym_entry *syme = dso__sym_priv(self, sym);
+		syme->skip = 1;
 	}
 
 	if (filter_match == 1) {
@@ -733,6 +719,8 @@ int cmd_top(int argc, const char **argv, const char *prefix __used)
 	if (delay_secs < 1)
 		delay_secs = 1;
 
+	skip_symbols = strlist__new(true, "file://" PERF_ETC_PATH "/symbols.skip");
+
 	parse_symbols();
 
 	/*
diff --git a/tools/perf/util/strlist.c b/tools/perf/util/strlist.c
index 7ad3817..d1d63f7 100644
--- a/tools/perf/util/strlist.c
+++ b/tools/perf/util/strlist.c
@@ -104,8 +104,14 @@ void strlist__remove(struct strlist *self, struct str_node *sn)
 
 bool strlist__has_entry(struct strlist *self, const char *entry)
 {
-	struct rb_node **p = &self->entries.rb_node;
-	struct rb_node *parent = NULL;
+	struct rb_node **p;
+	struct rb_node *parent;
+
+	if (self == NULL)
+		return false;
+
+	p = &self->entries.rb_node;
+	parent = NULL;
 
 	while (*p != NULL) {
 		struct str_node *sn;
-- 
1.6.2.5

Re: [PATCH 1/1 tip] perf top: Move hard coded list to /etc/perfconfig/symbols.skip

[Frederic Weisbecker]

On Mon, Jul 27, 2009 at 09:10:41PM -0300, Arnaldo Carvalho de Melo wrote:
> This also paves the way to add more symbol lists to be filtered out,
> spinlock anyone?
> 
> Cc: Frederic Weisbecker <fweisbec@gmail.com>
> Cc: Mike Galbraith <efault@gmx.de>
> Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
> Cc: Paul Mackerras <paulus@samba.org>
> Cc: H. Peter Anvin <hpa@zytor.com>
> Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
> ---



I guess it would be nice to also provide a way for non-privileged users
to tune it.

May be /etc/perfconfig/ and also ~/perfconfig/ ?




>  tools/perf/Makefile       |    6 ++++--
>  tools/perf/builtin-top.c  |   26 +++++++-------------------
>  tools/perf/util/strlist.c |   10 ++++++++--
>  3 files changed, 19 insertions(+), 23 deletions(-)
> 
> diff --git a/tools/perf/Makefile b/tools/perf/Makefile
> index a5e9b87..385a1bc 100644
> --- a/tools/perf/Makefile
> +++ b/tools/perf/Makefile
> @@ -193,11 +193,10 @@ template_dir = share/perf-core/templates
>  htmldir = share/doc/perf-doc
>  ifeq ($(prefix),/usr)
>  sysconfdir = /etc
> -ETC_PERFCONFIG = $(sysconfdir)/perfconfig
>  else
>  sysconfdir = $(prefix)/etc
> -ETC_PERFCONFIG = etc/perfconfig
>  endif
> +ETC_PERFCONFIG = $(sysconfdir)/perfconfig
>  lib = lib
>  # DESTDIR=
>  
> @@ -596,6 +595,7 @@ PERL_PATH_SQ = $(subst ','\'',$(PERL_PATH))
>  LIBS = $(PERFLIBS) $(EXTLIBS)
>  
>  BASIC_CFLAGS += -DSHA1_HEADER='$(SHA1_HEADER_SQ)' \
> +		-DPERF_ETC_PATH='"$(ETC_PERFCONFIG)"' \
>  	$(COMPAT_CFLAGS)
>  LIB_OBJS += $(COMPAT_OBJS)
>  
> @@ -804,6 +804,8 @@ export perfexec_instdir
>  install: all
>  	$(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(bindir_SQ)'
>  	$(INSTALL) perf$X '$(DESTDIR_SQ)$(bindir_SQ)'
> +	$(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(ETC_PERFCONFIG_SQ)'
> +	$(INSTALL) symbols.skip '$(DESTDIR_SQ)$(ETC_PERFCONFIG_SQ)'
>  ifdef BUILT_INS
>  	$(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(perfexec_instdir_SQ)'
>  	$(INSTALL) $(BUILT_INS) '$(DESTDIR_SQ)$(perfexec_instdir_SQ)'
> diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
> index f139f1a..628ef29 100644
> --- a/tools/perf/builtin-top.c
> +++ b/tools/perf/builtin-top.c
> @@ -20,6 +20,7 @@
>  
>  #include "perf.h"
>  
> +#include "util/strlist.h"
>  #include "util/symbol.h"
>  #include "util/color.h"
>  #include "util/util.h"
> @@ -279,24 +280,12 @@ static void *display_thread(void *arg __used)
>  }
>  
>  /* Tag samples to be skipped. */
> -static const char *skip_symbols[] = {
> -	"default_idle",
> -	"cpu_idle",
> -	"enter_idle",
> -	"exit_idle",
> -	"mwait_idle",
> -	"mwait_idle_with_hints",
> -	"ppc64_runlatch_off",
> -	"pseries_dedicated_idle_sleep",
> -	NULL
> -};
> +static struct strlist *skip_symbols;
>  
>  static int symbol_filter(struct dso *self, struct symbol *sym)
>  {
>  	static int filter_match;
> -	struct sym_entry *syme;
>  	const char *name = sym->name;
> -	int i;
>  
>  	/*
>  	 * ppc64 uses function descriptors and appends a '.' to the
> @@ -314,12 +303,9 @@ static int symbol_filter(struct dso *self, struct symbol *sym)
>  	    strstr(name, "_text_end"))
>  		return 1;
>  
> -	syme = dso__sym_priv(self, sym);
> -	for (i = 0; skip_symbols[i]; i++) {
> -		if (!strcmp(skip_symbols[i], name)) {
> -			syme->skip = 1;
> -			break;
> -		}
> +	if (strlist__has_entry(skip_symbols, name)) {
> +		struct sym_entry *syme = dso__sym_priv(self, sym);
> +		syme->skip = 1;
>  	}
>  
>  	if (filter_match == 1) {
> @@ -733,6 +719,8 @@ int cmd_top(int argc, const char **argv, const char *prefix __used)
>  	if (delay_secs < 1)
>  		delay_secs = 1;
>  
> +	skip_symbols = strlist__new(true, "file://" PERF_ETC_PATH "/symbols.skip");
> +
>  	parse_symbols();
>  
>  	/*
> diff --git a/tools/perf/util/strlist.c b/tools/perf/util/strlist.c
> index 7ad3817..d1d63f7 100644
> --- a/tools/perf/util/strlist.c
> +++ b/tools/perf/util/strlist.c
> @@ -104,8 +104,14 @@ void strlist__remove(struct strlist *self, struct str_node *sn)
>  
>  bool strlist__has_entry(struct strlist *self, const char *entry)
>  {
> -	struct rb_node **p = &self->entries.rb_node;
> -	struct rb_node *parent = NULL;
> +	struct rb_node **p;
> +	struct rb_node *parent;
> +
> +	if (self == NULL)
> +		return false;
> +
> +	p = &self->entries.rb_node;
> +	parent = NULL;
>  
>  	while (*p != NULL) {
>  		struct str_node *sn;
> -- 
> 1.6.2.5
> 

Re: [PATCH 1/1 tip] perf top: Move hard coded list to /etc/perfconfig/symbols.skip

[Arnaldo Carvalho de Melo]

Em Tue, Jul 28, 2009 at 05:18:21AM +0200, Frederic Weisbecker escreveu:
> On Mon, Jul 27, 2009 at 09:10:41PM -0300, Arnaldo Carvalho de Melo wrote:
> > This also paves the way to add more symbol lists to be filtered out,
> > spinlock anyone?

> I guess it would be nice to also provide a way for non-privileged users
> to tune it.
 
> May be /etc/perfconfig/ and also ~/perfconfig/ ?

perf will have the same configuration system that git has, so the answer
is yes, your suggestion will be implemented soon.

- Arnaldo

Re: [PATCH 1/1 tip] perf top: Move hard coded list to /etc/perfconfig/symbols.skip

[Ray Lee]

On Mon, Jul 27, 2009 at 5:10 PM, Arnaldo Carvalho de
Melo<acme@redhat.com> wrote:
> This also paves the way to add more symbol lists to be filtered out,
> spinlock anyone?

Generally, the way I've found is best to handle things like this is to either:

A. Have a fall-back list to use if the file isn't found, or
B. Start with the fall-back list of symbols to filter, then add to
them via /etc/... and ~/.perf/...

I like approaching things from the 'everything works even in a
broken/hasty installation' point of view, and it's paid off for me.
Sane defaults are a wonderfully useful thing.

Re: [PATCH 1/1 tip] perf top: Move hard coded list to /etc/perfconfig/symbols.skip

[Arnaldo Carvalho de Melo]

Em Mon, Jul 27, 2009 at 08:58:08PM -0700, Ray Lee escreveu:
> On Mon, Jul 27, 2009 at 5:10 PM, Arnaldo Carvalho de
> Melo<acme@redhat.com> wrote:
> > This also paves the way to add more symbol lists to be filtered out,
> > spinlock anyone?
> 
> Generally, the way I've found is best to handle things like this is to either:
> 
> A. Have a fall-back list to use if the file isn't found, or
> B. Start with the fall-back list of symbols to filter, then add to
> them via /etc/... and ~/.perf/...
> 
> I like approaching things from the 'everything works even in a
> broken/hasty installation' point of view, and it's paid off for me.
> Sane defaults are a wonderfully useful thing.

While in general we agree with you, in this case the only problem would
be that we would see a few functions on idle systems, i.e. not something
catastrophic.

- Arnaldo

Re: [PATCH 1/1 tip] perf top: Move hard coded list to /etc/perfconfig/symbols.skip

[Ray Lee]

On Tue, Jul 28, 2009 at 7:17 AM, Arnaldo Carvalho de
Melo<acme@redhat.com> wrote:
>> I like approaching things from the 'everything works even in a
>> broken/hasty installation' point of view, and it's paid off for me.
>> Sane defaults are a wonderfully useful thing.
>
> While in general we agree with you, in this case the only problem would
> be that we would see a few functions on idle systems, i.e. not something
> catastrophic.

This is userspace we're talking about here. Where's the harm in
getting things right by default?

Re: [PATCH 1/1 tip] perf top: Move hard coded list to /etc/perfconfig/symbols.skip

[Ingo Molnar]

* Ray Lee <ray-lk@madrabbit.org> wrote:

> On Mon, Jul 27, 2009 at 5:10 PM, Arnaldo Carvalho de
> Melo<acme@redhat.com> wrote:
> > This also paves the way to add more symbol lists to be filtered out,
> > spinlock anyone?
> 
> Generally, the way I've found is best to handle things like this is to either:
> 
> A. Have a fall-back list to use if the file isn't found, or
> B. Start with the fall-back list of symbols to filter, then add to
> them via /etc/... and ~/.perf/...
> 
> I like approaching things from the 'everything works even in a
> broken/hasty installation' point of view, and it's paid off for me.
> Sane defaults are a wonderfully useful thing.

Agreed.

Also, i'd not pollute /etc just yet, please lets define a good 
default built-in list like we have now, and perhaps enable the 
extension of it via .perf/config.

Tools should work well by default.

	Ingo

Re: [PATCH 1/1 tip] perf top: Move hard coded list to /etc/perfconfig/symbols.skip

[Arnaldo Carvalho de Melo]

Em Sun, Aug 02, 2009 at 09:55:33PM +0200, Ingo Molnar escreveu:
> 
> * Ray Lee <ray-lk@madrabbit.org> wrote:
> 
> > On Mon, Jul 27, 2009 at 5:10 PM, Arnaldo Carvalho de
> > Melo<acme@redhat.com> wrote:
> > > This also paves the way to add more symbol lists to be filtered out,
> > > spinlock anyone?
> > 
> > Generally, the way I've found is best to handle things like this is to either:
> > 
> > A. Have a fall-back list to use if the file isn't found, or
> > B. Start with the fall-back list of symbols to filter, then add to
> > them via /etc/... and ~/.perf/...
> > 
> > I like approaching things from the 'everything works even in a
> > broken/hasty installation' point of view, and it's paid off for me.
> > Sane defaults are a wonderfully useful thing.
> 
> Agreed.
> 
> Also, i'd not pollute /etc just yet, please lets define a good 
> default built-in list like we have now, and perhaps enable the 
> extension of it via .perf/config.
> 
> Tools should work well by default.

OK, I'll rework the patch and resubmit it.

- Arnaldo

Re: [PATCH 1/1 tip] perf top: Move hard coded list to /etc/perfconfig/symbols.skip

[Peter Zijlstra]

On Mon, 2009-07-27 at 21:10 -0300, Arnaldo Carvalho de Melo wrote:
> This also paves the way to add more symbol lists to be filtered out,
> spinlock anyone?

Doesn't seem like a good idea to filter out symbols that actually affect
performance -- such as spinlocks.

But yeah, the patch makes sense and allows people to mess up their own
environments :-)

Queued this and the previous one, Thanks!