Re: [BUG] perf report --pid not reporting correctly

[Stephane Eranian <eranian@google.com>]

On Wed, Sep 21, 2016 at 9:34 AM, Jiri Olsa <jolsa@redhat.com> wrote:
> On Wed, Sep 21, 2016 at 12:37:53PM -0300, Arnaldo Carvalho de Melo wrote:
>> Em Tue, Sep 20, 2016 at 06:29:59PM -0700, Stephane Eranian escreveu:
>> > Hi Arnaldo,
>> >
>> > I ran into an issue trying to use the --pid filtering option of perf report.
>> >
>> > I do a system-wide collection and then I want to narrow down the
>> > reporting to a specific process:
>> >
>> > $ perf record -a -e cycles:pp sleep 10
>> > $ perf report --sort cpu,comm --pid X
>> >
>> > Where X is a process sampled during the run (easy to catch with perf report -D)
>> > If you do it this way, it works, but if you do:
>> >
>> > $ perf report --sort cpu --pid X
>> >
>> > Then you get an empty output.
>> >
>> > I suspect it has to do with the way hist entries are added to the
>> > histogram and aggregated. If the first event for a sort criteria is
>> > not coming from pid X, it will
>> > still be added in the histogram. if pid X aggregates to the same
>> > sample criteria, then you will lose the pid information. And then
>> > later when you try to apply the filter,
>> > it will mark the hist entry as FILTERED because it does not have a matching pid
>> > and nothing will be printed.
>> > I suspect you want to apply the filtering upfront for pid. It will
>> > only add to the histograms matching samples. It changes the
>> > percentages you will see. They will
>> > only report the breakdown for the pid.
>> >
>> > I have a quick hack to do upfront filtering which does something as
>> > follows but I am not sure this is the correct way of doing this.
>> >
>> > Let me know what you think.
>>
>> From a first look I think this makes sense, i.e. we should do the first
>> round of filtering, one that trows away stuff, for things in the command
>> line, when creating the histogram entries.
>>
>> Later, as we have now, we can apply further filters for non-collapsed
>> fields of hist_entry.
>>
>> Jiri, Namhyung, are you ok with this?
>
> Stephan is correct with analysis, but I think we need to add both non/filtered
> entries in, because we provide that 'F' key for non/filtered counts switch in tui
>
> how about something like below
>
> thanks,
> jirka
>
> ---
> diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
> index b02992efb513..659e0357be68 100644
> --- a/tools/perf/util/hist.c
> +++ b/tools/perf/util/hist.c
> @@ -536,6 +536,14 @@ static struct hist_entry *hists__findnew_entry(struct hists *hists,
>                                 map__put(he->ms.map);
>                                 he->ms.map = map__get(entry->ms.map);
>                         }
> +
> +                       /*
> +                        * We have at least one entry in which is not
> +                        * filtered, we want to display the entry.
> +                        */
> +                       if (he->filtered && !entry->filtered)
> +                               he->filtered = 0;
> +
>                         goto out;
>                 }
>
Works for me. So with this approach the % shown with --pid still
represents % of total samples and not just for that pid.
I think this is okay as long as this is documented and understood by users.
Thanks.