[PATCH] perf record: Fix BTF type checks in the off-cpu profiling

[PATCH] perf record: Fix BTF type checks in the off-cpu profiling

[Namhyung Kim]

The BTF func proto for a tracepoint has one more argument than the
actual tracepoint function since it has a context argument at the
begining.  So it should compare to 5 when the tracepoint has 4
arguments.

  typedef void (*btf_trace_sched_switch)(void *, bool, struct task_struct *, struct task_struct *, unsigned int);

Also, recent change in the perf tool would use a hand-written minimal
vmlinux.h to generate BTF in the skeleton.  So it won't have the info
of the tracepoint.  Anyway it should use the kernel's vmlinux BTF to
check the type in the kernel.

Fixes: b36888f71c85 ("perf record: Handle argument change in sched_switch")
Cc: Song Liu <song@kernel.org>
Cc: Hao Luo <haoluo@google.com>
CC: bpf@vger.kernel.org
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/bpf_off_cpu.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/bpf_off_cpu.c b/tools/perf/util/bpf_off_cpu.c
index 01f70b8e705a..21f4d9ba023d 100644
--- a/tools/perf/util/bpf_off_cpu.c
+++ b/tools/perf/util/bpf_off_cpu.c
@@ -98,7 +98,7 @@ static void off_cpu_finish(void *arg __maybe_unused)
 /* v5.18 kernel added prev_state arg, so it needs to check the signature */
 static void check_sched_switch_args(void)
 {
-	const struct btf *btf = bpf_object__btf(skel->obj);
+	const struct btf *btf = btf__load_vmlinux_btf();
 	const struct btf_type *t1, *t2, *t3;
 	u32 type_id;
 
@@ -116,7 +116,8 @@ static void check_sched_switch_args(void)
 		return;
 
 	t3 = btf__type_by_id(btf, t2->type);
-	if (t3 && btf_is_func_proto(t3) && btf_vlen(t3) == 4) {
+	/* btf_trace func proto has one more argument for the context */
+	if (t3 && btf_is_func_proto(t3) && btf_vlen(t3) == 5) {
 		/* new format: pass prev_state as 4th arg */
 		skel->rodata->has_prev_state = true;
 	}
-- 
2.42.0.515.g380fc7ccd1-goog

Re: [PATCH] perf record: Fix BTF type checks in the off-cpu profiling

[Ian Rogers]

On Fri, Sep 22, 2023 at 4:44 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> The BTF func proto for a tracepoint has one more argument than the
> actual tracepoint function since it has a context argument at the
> begining.  So it should compare to 5 when the tracepoint has 4
> arguments.
>
>   typedef void (*btf_trace_sched_switch)(void *, bool, struct task_struct *, struct task_struct *, unsigned int);
>
> Also, recent change in the perf tool would use a hand-written minimal
> vmlinux.h to generate BTF in the skeleton.  So it won't have the info
> of the tracepoint.  Anyway it should use the kernel's vmlinux BTF to
> check the type in the kernel.
>
> Fixes: b36888f71c85 ("perf record: Handle argument change in sched_switch")
> Cc: Song Liu <song@kernel.org>
> Cc: Hao Luo <haoluo@google.com>
> CC: bpf@vger.kernel.org
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>

Reviewed-by: Ian Rogers <irogers@google.com>

> ---
>  tools/perf/util/bpf_off_cpu.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/tools/perf/util/bpf_off_cpu.c b/tools/perf/util/bpf_off_cpu.c
> index 01f70b8e705a..21f4d9ba023d 100644
> --- a/tools/perf/util/bpf_off_cpu.c
> +++ b/tools/perf/util/bpf_off_cpu.c
> @@ -98,7 +98,7 @@ static void off_cpu_finish(void *arg __maybe_unused)
>  /* v5.18 kernel added prev_state arg, so it needs to check the signature */
>  static void check_sched_switch_args(void)
>  {
> -       const struct btf *btf = bpf_object__btf(skel->obj);
> +       const struct btf *btf = btf__load_vmlinux_btf();
>         const struct btf_type *t1, *t2, *t3;
>         u32 type_id;
>
> @@ -116,7 +116,8 @@ static void check_sched_switch_args(void)
>                 return;
>
>         t3 = btf__type_by_id(btf, t2->type);
> -       if (t3 && btf_is_func_proto(t3) && btf_vlen(t3) == 4) {
> +       /* btf_trace func proto has one more argument for the context */
> +       if (t3 && btf_is_func_proto(t3) && btf_vlen(t3) == 5) {
>                 /* new format: pass prev_state as 4th arg */

nit: does this comment need updating?

>                 skel->rodata->has_prev_state = true;
>         }
> --
> 2.42.0.515.g380fc7ccd1-goog
>

Re: [PATCH] perf record: Fix BTF type checks in the off-cpu profiling

[Namhyung Kim]

Hi Ian,

On Sun, Sep 24, 2023 at 11:03 AM Ian Rogers <irogers@google.com> wrote:
>
> On Fri, Sep 22, 2023 at 4:44 PM Namhyung Kim <namhyung@kernel.org> wrote:
> >
> > The BTF func proto for a tracepoint has one more argument than the
> > actual tracepoint function since it has a context argument at the
> > begining.  So it should compare to 5 when the tracepoint has 4
> > arguments.
> >
> >   typedef void (*btf_trace_sched_switch)(void *, bool, struct task_struct *, struct task_struct *, unsigned int);
> >
> > Also, recent change in the perf tool would use a hand-written minimal
> > vmlinux.h to generate BTF in the skeleton.  So it won't have the info
> > of the tracepoint.  Anyway it should use the kernel's vmlinux BTF to
> > check the type in the kernel.
> >
> > Fixes: b36888f71c85 ("perf record: Handle argument change in sched_switch")
> > Cc: Song Liu <song@kernel.org>
> > Cc: Hao Luo <haoluo@google.com>
> > CC: bpf@vger.kernel.org
> > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
>
> Reviewed-by: Ian Rogers <irogers@google.com>

Thanks for the review!

>
> > ---
> >  tools/perf/util/bpf_off_cpu.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/tools/perf/util/bpf_off_cpu.c b/tools/perf/util/bpf_off_cpu.c
> > index 01f70b8e705a..21f4d9ba023d 100644
> > --- a/tools/perf/util/bpf_off_cpu.c
> > +++ b/tools/perf/util/bpf_off_cpu.c
> > @@ -98,7 +98,7 @@ static void off_cpu_finish(void *arg __maybe_unused)
> >  /* v5.18 kernel added prev_state arg, so it needs to check the signature */
> >  static void check_sched_switch_args(void)
> >  {
> > -       const struct btf *btf = bpf_object__btf(skel->obj);
> > +       const struct btf *btf = btf__load_vmlinux_btf();
> >         const struct btf_type *t1, *t2, *t3;
> >         u32 type_id;
> >
> > @@ -116,7 +116,8 @@ static void check_sched_switch_args(void)
> >                 return;
> >
> >         t3 = btf__type_by_id(btf, t2->type);
> > -       if (t3 && btf_is_func_proto(t3) && btf_vlen(t3) == 4) {
> > +       /* btf_trace func proto has one more argument for the context */
> > +       if (t3 && btf_is_func_proto(t3) && btf_vlen(t3) == 5) {
> >                 /* new format: pass prev_state as 4th arg */
>
> nit: does this comment need updating?

No, it's the 4th arg and the sched_switch is called like.

        trace_sched_switch(sched_mode & SM_MASK_PREEMPT, prev, next,
prev_state);

Thanks,
Namhyung

Re: [PATCH] perf record: Fix BTF type checks in the off-cpu profiling

[Song Liu]

On Fri, Sep 22, 2023 at 4:44 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> The BTF func proto for a tracepoint has one more argument than the
> actual tracepoint function since it has a context argument at the
> begining.  So it should compare to 5 when the tracepoint has 4
> arguments.
>
>   typedef void (*btf_trace_sched_switch)(void *, bool, struct task_struct *, struct task_struct *, unsigned int);
>
> Also, recent change in the perf tool would use a hand-written minimal
> vmlinux.h to generate BTF in the skeleton.  So it won't have the info
> of the tracepoint.  Anyway it should use the kernel's vmlinux BTF to
> check the type in the kernel.
>
> Fixes: b36888f71c85 ("perf record: Handle argument change in sched_switch")
> Cc: Song Liu <song@kernel.org>
> Cc: Hao Luo <haoluo@google.com>
> CC: bpf@vger.kernel.org
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>

Acked-by: Song Liu <song@kernel.org>

Re: [PATCH] perf record: Fix BTF type checks in the off-cpu profiling

[Namhyung Kim]

On Wed, Sep 27, 2023 at 9:09 AM Song Liu <song@kernel.org> wrote:
>
> On Fri, Sep 22, 2023 at 4:44 PM Namhyung Kim <namhyung@kernel.org> wrote:
> >
> > The BTF func proto for a tracepoint has one more argument than the
> > actual tracepoint function since it has a context argument at the
> > begining.  So it should compare to 5 when the tracepoint has 4
> > arguments.
> >
> >   typedef void (*btf_trace_sched_switch)(void *, bool, struct task_struct *, struct task_struct *, unsigned int);
> >
> > Also, recent change in the perf tool would use a hand-written minimal
> > vmlinux.h to generate BTF in the skeleton.  So it won't have the info
> > of the tracepoint.  Anyway it should use the kernel's vmlinux BTF to
> > check the type in the kernel.
> >
> > Fixes: b36888f71c85 ("perf record: Handle argument change in sched_switch")
> > Cc: Song Liu <song@kernel.org>
> > Cc: Hao Luo <haoluo@google.com>
> > CC: bpf@vger.kernel.org
> > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
>
> Acked-by: Song Liu <song@kernel.org>

Applied to perf-tools-next, thanks!