This also paves the way to add more symbol lists to be filtered out,
spinlock anyone?
Cc: Frederic Weisbecker <[email protected]>
Cc: Mike Galbraith <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Paul Mackerras <[email protected]>
Cc: H. Peter Anvin <[email protected]>
Signed-off-by: Arnaldo Carvalho de Melo <[email protected]>
---
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
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 <[email protected]>
> Cc: Mike Galbraith <[email protected]>
> Cc: Peter Zijlstra <[email protected]>
> Cc: Paul Mackerras <[email protected]>
> Cc: H. Peter Anvin <[email protected]>
> Signed-off-by: Arnaldo Carvalho de Melo <[email protected]>
> ---
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
>
On Mon, Jul 27, 2009 at 5:10 PM, Arnaldo Carvalho de
Melo<[email protected]> 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.
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!
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
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<[email protected]> 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
On Tue, Jul 28, 2009 at 7:17 AM, Arnaldo Carvalho de
Melo<[email protected]> 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?