Re: [PATCH] perf script: Fix overrun issue for dynamically-allocated pmu type number

[Jin, Yao]

Hi Adrian,

On 11/26/2020 4:36 PM, Adrian Hunter wrote:
> On 26/11/20 9:06 am, Jin, Yao wrote:
> > Hi Adrian,
> >
> > On 11/26/2020 2:51 PM, Adrian Hunter wrote:
> > > On 26/11/20 5:24 am, Jin Yao wrote:
> > > > When unpacking the event which is from dynamic pmu, the array
> > > > output[OUTPUT_TYPE_MAX] may be overrun. For example, type number of
> > > > SKL uncore_imc is 10, but OUTPUT_TYPE_MAX is 7 now (OUTPUT_TYPE_MAX =
> > > > PERF_TYPE_MAX + 1).
> > > >
> > > > /* In builtin-script.c */
> > > > process_event()
> > > > {
> > > >      unsigned int type = output_type(attr->type);
> > > >
> > > >      if (output[type].fields == 0)
> > > >          return;
> > > > }
> > > >
> > > > output[10] is overrun.
> > > >
> > > > Create a type OUTPUT_TYPE_OTHER for dynamic pmu events, then
> > > > output_type(attr->type) will return OUTPUT_TYPE_OTHER here.
> > > >
> > > > Note that if PERF_TYPE_MAX ever changed, then there would be a conflict
> > > > between old perf.data files that had a dynamicaliy allocated PMU number
> > > > that would then be the same as a fixed PERF_TYPE.
> > > >
> > > > Example:
> > > >
> > > > perf record --switch-events -C 0 -e
> > > > "{cpu-clock,uncore_imc/data_reads/,uncore_imc/data_writes/}:SD" -a --
> > > > sleep 1
> > > > perf script
> > > >
> > > > Before:
> > > >            swapper     0 [000] 1479253.987551:     277766
> > > > cpu-clock:  ffffffff9d4ddb6f cpuidle_enter_state+0xdf ([kernel.kallsyms])
> > > >            swapper     0 [000] 1479253.987797:     246709
> > > > cpu-clock:  ffffffff9d4ddb6f cpuidle_enter_state+0xdf ([kernel.kallsyms])
> > > >            swapper     0 [000] 1479253.988127:     329883
> > > > cpu-clock:  ffffffff9d4ddb6f cpuidle_enter_state+0xdf ([kernel.kallsyms])
> > > >            swapper     0 [000] 1479253.988273:     146393
> > > > cpu-clock:  ffffffff9d4ddb6f cpuidle_enter_state+0xdf ([kernel.kallsyms])
> > > >            swapper     0 [000] 1479253.988523:     249977
> > > > cpu-clock:  ffffffff9d4ddb6f cpuidle_enter_state+0xdf ([kernel.kallsyms])
> > > >            swapper     0 [000] 1479253.988877:     354090
> > > > cpu-clock:  ffffffff9d4ddb6f cpuidle_enter_state+0xdf ([kernel.kallsyms])
> > > >            swapper     0 [000] 1479253.989023:     145940
> > > > cpu-clock:  ffffffff9d4ddb6f cpuidle_enter_state+0xdf ([kernel.kallsyms])
> > > >            swapper     0 [000] 1479253.989383:     359856
> > > > cpu-clock:  ffffffff9d4ddb6f cpuidle_enter_state+0xdf ([kernel.kallsyms])
> > > >            swapper     0 [000] 1479253.989523:     140082
> > > > cpu-clock:  ffffffff9d4ddb6f cpuidle_enter_state+0xdf ([kernel.kallsyms])
> > > >
> > > > After:
> > > >            swapper     0 [000] 1397040.402011:     272384
> > > > cpu-clock:  ffffffff9d4ddb6f cpuidle_enter_state+0xdf ([kernel.kallsyms])
> > > >            swapper     0 [000] 1397040.402011:       5396
> > > > uncore_imc/data_reads/:
> > > >            swapper     0 [000] 1397040.402011:        967
> > > > uncore_imc/data_writes/:
> > > >            swapper     0 [000] 1397040.402259:     249153
> > > > cpu-clock:  ffffffff9d4ddb6f cpuidle_enter_state+0xdf ([kernel.kallsyms])
> > > >            swapper     0 [000] 1397040.402259:       7231
> > > > uncore_imc/data_reads/:
> > > >            swapper     0 [000] 1397040.402259:       1297
> > > > uncore_imc/data_writes/:
> > > >            swapper     0 [000] 1397040.402508:     249108
> > > > cpu-clock:  ffffffff9d4ddb6f cpuidle_enter_state+0xdf ([kernel.kallsyms])
> > > >            swapper     0 [000] 1397040.402508:       5333
> > > > uncore_imc/data_reads/:
> > > >            swapper     0 [000] 1397040.402508:       1008
> > > > uncore_imc/data_writes/:
> > > >
> > > > Fixes: 1405720d4f26 ("perf script: Add 'synth' event type for synthesized
> > > > events")
> > >
> > > It does not look to me like the problem was introduced by that commit.  Are
> > > you sure this Fixes tag is correct?
> >
> > Commit 1405720d4f26 added the change:
> >
> > @@ -1215,8 +1253,9 @@ static void process_event(struct perf_script *script,
> >   {
> >          struct thread *thread = al->thread;
> >          struct perf_event_attr *attr = &evsel->attr;
> > +       unsigned int type = output_type(attr->type);
> >
> > -       if (output[attr->type].fields == 0)
> > +       if (output[type].fields == 0)
> >                  return;
>
> That is a nop if attr->type != PERF_TYPE_SYNTH
> Given that PERF_TYPE_SYNTH is (INT_MAX + 1), it is a nop for all kernel
> dynamically allocated PMU numbers.
>
> > But of course, we can also say the original "output[attr->type].fields"
> > introduced the issue, I'm not sure. Maybe Arnaldo can help to make the
> > decision. :)
>
> I think perf script has always had this problem.

Since perf-script has always had this problem, I'm OK to remove the fixes tag from the patch 
description.

Thanks
Jin Yao