[RFC PATCH] perf report: Don't add filtered events to histogram

[RFC PATCH] perf report: Don't add filtered events to histogram

[Steve Capper]

If one filters events by a single comm in perf report via:
 perf report --no-children -c program-name

perf will elide the comm column as only one comm is filtered in. This
lack of distinction by comm can lead to events being merged in
add_hist_entry that have different comm's, including events that have
been filtered out and filtered in. Ultimately, this can lead to events
disappearing from the report.

If one instead invokes perf via:
 perf report --no-children -c program-name,random-made-up-name

Then the comm column remains and all the expected events are present.

This patch prevents filtered out events from being considered by
hist_entry_iter__add.

Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
Signed-off-by: Steve Capper <steve.capper@linaro.org>
---
Hi,
This patch fixes my problem, but I don't know enough perf to
confidently state that this will work everywhere (hence this being
marked as an RFC).

Please let me know if I've missed something and I'll be happy to
re-spin/test/submit.

Cheers,
-- 
Steve
---
 tools/perf/builtin-report.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
index 2f91094..3eaf46e 100644
--- a/tools/perf/builtin-report.c
+++ b/tools/perf/builtin-report.c
@@ -166,6 +166,9 @@ static int process_sample_event(struct perf_tool *tool,
 	if (al.map != NULL)
 		al.map->dso->hit = 1;
 
+	if (al.filtered)
+		return 0;
+
 	ret = hist_entry_iter__add(&iter, &al, evsel, sample, rep->max_stack,
 				   rep);
 	if (ret < 0)
-- 
2.1.0

Re: [RFC PATCH] perf report: Don't add filtered events to histogram

[Arnaldo Carvalho de Melo]

Em Tue, Apr 14, 2015 at 01:05:03PM +0100, Steve Capper escreveu:
> If one filters events by a single comm in perf report via:
>  perf report --no-children -c program-name
 
This seems right even without any explanation, wonder when was that this
bug was added or what is that we are missing here...

commit 2c86c7ca760634f09dcbd76069e5102b4de6f8f1
Author: Namhyung Kim <namhyung@kernel.org>
Date:   Mon Mar 17 18:18:54 2014 -0300

    perf report: Merge al->filtered with hist_entry->filtered
    
    I.e. don't drop al->filtered entries, create the hist_entries and use
    its ->filtered bitmap, that is kept with the same semantics for its
    bitmap, leaving the filtering to be done at the hist_entry level, i.e.
    in the UIs.
    
    This will allow zooming in/out the filters.
    
    Signed-off-by: Namhyung Kim <namhyung.kim@lge.com>
    Acked-by: Jiri Olsa <jolsa@redhat.com>
    Cc: Andi Kleen <andi@firstfloor.org>
    Cc: David Ahern <dsahern@gmail.com>
    Cc: Ingo Molnar <mingo@kernel.org>
    Cc: Jiri Olsa <jolsa@redhat.com>
    Cc: Namhyung Kim <namhyung.kim@lge.com>
    Cc: Paul Mackerras <paulus@samba.org>
    Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
    Link: http://lkml.kernel.org/n/tip-xeyhkepu7plw716lrtb0zlnu@git.kernel.org
    [ yanked this out of a previous patch ]
    Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>

diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
index c47bf586fcba..a74059f0c45f 100644
--- a/tools/perf/builtin-report.c
+++ b/tools/perf/builtin-report.c
@@ -231,7 +231,7 @@ static int process_sample_event(struct perf_tool
*tool,
                return -1;
        }
 
-       if (al.filtered || (rep->hide_unresolved && al.sym == NULL))
+       if (rep->hide_unresolved && al.sym == NULL)
                return 0;
 
        if (rep->cpu_list && !test_bit(sample->cpu, rep->cpu_bitmap))

--------------------------------------------------------------

So, yes, what you did was what was there in the first place, but now we
need to try to have the above cset in mind before proceeding...

- Arnaldo


> +++ b/tools/perf/builtin-report.c
> @@ -166,6 +166,9 @@ static int process_sample_event(struct perf_tool *tool,
>  	if (al.map != NULL)
>  		al.map->dso->hit = 1;
>  
> +	if (al.filtered)
> +		return 0;
> +
>  	ret = hist_entry_iter__add(&iter, &al, evsel, sample, rep->max_stack,
>  				   rep);
>  	if (ret < 0)
> -- 
> 2.1.0