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

[Namhyung Kim <namhyung@kernel.org>]

Hi Arnaldo and Stephane,

On Thu, Sep 22, 2016 at 12:37 AM, Arnaldo Carvalho de Melo
<acme@redhat.com> 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?

I'm not sure it'd be good to allow filtering by something that is not
in the sort key.  As Stephane said, current filtering behavior (by
cpu, socket, comm, dso, pid, tid and symbol) only works correctly if
it's in the sort key AFAIK.

And the calculation of total period is based on periods of each hist
entry at the output resort stage.  So it needs to keep the entries
separately to calculate it correctly.  We have --percentage option to
show absolute/relative value when a filter applied.

I think it's a choice.  If we decide to support it, the logic
calculating the total period should be changed to consider the
early-filtered entries somehow.

If not, I suggest to use --hierarchy mode with including the sort key
as the first one.  In this case.  It'll still show absolute
percentages of global total period, but you can see the desired result
easily IMHO.  Please try something like this:

  $ perf report --hierarchy -s pid,cpu

Thanks,
Namhyung

>
> - Arnaldo
>
>> Thanks.
>>
>>
>> diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
>> index 715467d..c24de6e 100644
>> --- a/tools/perf/util/hist.c
>> +++ b/tools/perf/util/hist.c
>> @@ -1020,6 +1020,9 @@ int hist_entry_iter__add(struct hist_entry_iter
>> *iter, struct addr_location *al,
>>  {
>>         int err, err2;
>>
>> +       if (thread__is_filtered(al->thread))
>> +               return 0;
>> +
>>         err = sample__resolve_callchain(iter->sample,
>> &callchain_cursor, &iter->parent,
>>                                        iter->evsel, al, max_stack_depth);
>>         if (err)