Received-SPF: pass (google.com: domain of linux-kernel+bounces-123205-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Precedence: bulk
MIME-Version: 1.0
References: <20240328055857.383180-1-yangjihong@bytedance.com>
In-Reply-To: <20240328055857.383180-1-yangjihong@bytedance.com>
From: Ian Rogers <irogers@google.com>
Date: Thu, 28 Mar 2024 09:02:31 -0700
Message-ID: <CAP-5=fXms1noWT8PXqBu89QogcwVsvAGpxq3Q_bNUYOYL7PpKA@mail.gmail.com>
Subject: Re: [PATCH 1/2] perf sched timehist: Fix -g/--call-graph option failure
To: Yang Jihong <yangjihong@bytedance.com>
Cc: peterz@infradead.org, mingo@redhat.com, acme@kernel.org, 
	namhyung@kernel.org, mark.rutland@arm.com, alexander.shishkin@linux.intel.com, 
	jolsa@kernel.org, adrian.hunter@intel.com, kan.liang@linux.intel.com, 
	james.clark@arm.com, linux-perf-users@vger.kernel.org, 
	linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Wed, Mar 27, 2024 at 10:59=E2=80=AFPM Yang Jihong <yangjihong@bytedance.=
com> 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?

Thanks,
Ian

> Signed-off-by: Yang Jihong <yangjihong@bytedance.com>
> ---
>  tools/perf/builtin-sched.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
> index b248c433529a..1bfb22347371 100644
> --- a/tools/perf/builtin-sched.c
> +++ b/tools/perf/builtin-sched.c
> @@ -2963,8 +2963,11 @@ static int timehist_check_attr(struct perf_sched *=
sched,
>                         return -1;
>                 }
>
> -               if (sched->show_callchain && !evsel__has_callchain(evsel)=
) {
> -                       pr_info("Samples do not have callchains.\n");
> +               /* only need to save callchain related to sched_switch ev=
ent */
> +               if (sched->show_callchain &&
> +                   evsel__name_is(evsel, "sched:sched_switch") &&
> +                   !evsel__has_callchain(evsel)) {
> +                       pr_info("Samples of sched_switch event do not hav=
e callchains.\n");
>                         sched->show_callchain =3D 0;
>                         symbol_conf.use_callchain =3D 0;
>                 }
> --
> 2.25.1
>