Re: [External] Re: [PATCH 1/2] perf sched timehist: Fix -g/--call-graph option failure

[Arnaldo Carvalho de Melo]

On Fri, Mar 29, 2024 at 09:08:01AM -0700, Ian Rogers wrote:
> On Thu, Mar 28, 2024 at 8:02 PM Yang Jihong <yangjihong@xxxxxxxxxxxxx> wrote:
> >
> > Hello,
> >
> > Sorry, due to the new email settings, the last reply email was in html
> > format, resend it now.
> >
> > On 3/29/24 00:02, Ian Rogers wrote:
> > > On Wed, Mar 27, 2024 at 10:59 PM Yang Jihong <yangjihong@xxxxxxxxxxxxx> wrote:
> > >>
> > >> When perf-sched enables the call-graph recording, sample_type of dummy
> > >> event does not have PERF_SAMPLE_CALLCHAIN, timehist_check_attr() checks
> > >> that the evsel does not have a callchain, and set show_callchain to 0.
> > >>
> > >> Currently perf sched timehist only saves callchain when processing
> > >> sched:sched_switch event, timehist_check_attr() only needs to determine
> > >> whether the event has PERF_SAMPLE_CALLCHAIN.
> > >>
> > >> Before:
> > >>    # perf sched record -g true
> > >>    [ perf record: Woken up 0 times to write data ]
> > >>    [ perf record: Captured and wrote 4.153 MB perf.data (7536 samples) ]
> > >>    # perf sched timehist
> > >>    Samples do not have callchains.
> > >>               time    cpu  task name                       wait time  sch delay   run time
> > >>                            [tid/pid]                          (msec)     (msec)     (msec)
> > >>    --------------- ------  ------------------------------  ---------  ---------  ---------
> > >>      147851.826019 [0000]  perf[285035]                        0.000      0.000      0.000
> > >>      147851.826029 [0000]  migration/0[15]                     0.000      0.003      0.009
> > >>      147851.826063 [0001]  perf[285035]                        0.000      0.000      0.000
> > >>      147851.826069 [0001]  migration/1[21]                     0.000      0.003      0.006
> > >>    <SNIP>
> > >>
> > >> After:
> > >>    # perf sched record -g true
> > >>    [ perf record: Woken up 1 times to write data ]
> > >>    [ perf record: Captured and wrote 2.572 MB perf.data (822 samples) ]
> > >>    # perf sched timehist
> > >>               time    cpu  task name                       wait time  sch delay   run time
> > >>                            [tid/pid]                          (msec)     (msec)     (msec)
> > >>    --------------- ------  ------------------------------  ---------  ---------  ---------
> > >>      144193.035164 [0000]  perf[277062]                        0.000      0.000      0.000    __traceiter_sched_switch <- __traceiter_sched_switch <- __sched_text_start <- preempt_schedule_common <- __cond_resched <- __wait_for_common <- wait_for_completion
> > >>      144193.035174 [0000]  migration/0[15]                     0.000      0.003      0.009    __traceiter_sched_switch <- __traceiter_sched_switch <- __sched_text_start <- smpboot_thread_fn <- kthread <- ret_from_fork
> > >>      144193.035207 [0001]  perf[277062]                        0.000      0.000      0.000    __traceiter_sched_switch <- __traceiter_sched_switch <- __sched_text_start <- preempt_schedule_common <- __cond_resched <- __wait_for_common <- wait_for_completion
> > >>      144193.035214 [0001]  migration/1[21]                     0.000      0.003      0.007    __traceiter_sched_switch <- __traceiter_sched_switch <- __sched_text_start <- smpboot_thread_fn <- kthread <- ret_from_fork
> > >> <SNIP>
> > >
> > > This looks good, should there be a Fixes tag for the sake of backports?
> > >
> > The direct cause is commit 9c95e4ef0657 ("perf evlist: Add
> > evlist__findnew_tracking_event() helper"). perf-record uses
> > evlist__add_aux_dummy() to replace evlist__add_dummy() to add a dummy
> > event. The difference is that evlist__add_aux_dummy() sets
> > no_aux_samples to true (this is expected behavior, for dummy event, no
> > need to sample aux data), resulting in evsel__config() not adding the
> > PERF_SAMPLE_CALLCHAIN bit to dummy's sample_type.
> >
> > In summary, the direct cause is the problem introduced by commit
> > 9c95e4ef0657 ("perf evlist: Add evlist__findnew_tracking_event()
> > helper"), but the root cause is the timehist_check_attr() logic problem,
> > The dummy event itself does not need to have PERF_SAMPLE_CALLCHAIN, so
> > there is no need to check it.
> >
> >
> > So, maybe add fixes-tag:
> >
> > Fixes: 9c95e4ef0657 ("perf evlist: Add evlist__findnew_tracking_event()
> > helper")
> >
> > If it is ok, I will send v2 version with this fixes-tag.
> 
> I think the maintainer can add the fixes tag when they add the reviewed-by tag:

I usually do this, but if the submitter does it I'll have just to check
that it is the right tag, helps a bit in processing.

- Arnaldo
 
> Reviewed-by: Ian Rogers <irogers@xxxxxxxxxx>
> 
> Thanks,
> Ian
> 
> > Thanks,
> > Yang