| Message ID | 20220624231313.367909-2-namhyung@kernel.org (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | BPF |
| Headers | show |
| Series | perf tools: A couple of fixes for perf record --off-cpu (v1) | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
Em Fri, Jun 24, 2022 at 04:13:08PM -0700, Namhyung Kim escreveu: > Old kernels have task_struct which contains "state" field and newer > kernels have "__state". While the get_task_state() in the BPF code > handles that in some way, it assumed the current kernel has the new > definition and it caused a build error on old kernels. > > We should not assume anything and access them carefully. Do not use > the task struct directly and access them using new and old definitions > in a row. I added a: Fixes: edc41a1099c2d08c ("perf record: Enable off-cpu analysis with BPF") Ok? - Arnaldo > Reported-by: Ian Rogers <irogers@google.com> > Signed-off-by: Namhyung Kim <namhyung@kernel.org> > --- > tools/perf/util/bpf_skel/off_cpu.bpf.c | 20 ++++++++++++++------ > 1 file changed, 14 insertions(+), 6 deletions(-) > > diff --git a/tools/perf/util/bpf_skel/off_cpu.bpf.c b/tools/perf/util/bpf_skel/off_cpu.bpf.c > index 792ae2847080..cc6d7fd55118 100644 > --- a/tools/perf/util/bpf_skel/off_cpu.bpf.c > +++ b/tools/perf/util/bpf_skel/off_cpu.bpf.c > @@ -71,6 +71,11 @@ struct { > __uint(max_entries, 1); > } cgroup_filter SEC(".maps"); > > +/* new kernel task_struct definition */ > +struct task_struct___new { > + long __state; > +} __attribute__((preserve_access_index)); > + > /* old kernel task_struct definition */ > struct task_struct___old { > long state; > @@ -93,14 +98,17 @@ const volatile bool uses_cgroup_v1 = false; > */ > static inline int get_task_state(struct task_struct *t) > { > - if (bpf_core_field_exists(t->__state)) > - return BPF_CORE_READ(t, __state); > + /* recast pointer to capture new type for compiler */ > + struct task_struct___new *t_new = (void *)t; > > - /* recast pointer to capture task_struct___old type for compiler */ > - struct task_struct___old *t_old = (void *)t; > + if (bpf_core_field_exists(t_new->__state)) { > + return BPF_CORE_READ(t_new, __state); > + } else { > + /* recast pointer to capture old type for compiler */ > + struct task_struct___old *t_old = (void *)t; > > - /* now use old "state" name of the field */ > - return BPF_CORE_READ(t_old, state); > + return BPF_CORE_READ(t_old, state); > + } > } > > static inline __u64 get_cgroup_id(struct task_struct *t) > -- > 2.37.0.rc0.161.g10f37bed90-goog
On Tue, Jun 28, 2022 at 7:44 AM Arnaldo Carvalho de Melo <acme@kernel.org> wrote: > > Em Fri, Jun 24, 2022 at 04:13:08PM -0700, Namhyung Kim escreveu: > > Old kernels have task_struct which contains "state" field and newer > > kernels have "__state". While the get_task_state() in the BPF code > > handles that in some way, it assumed the current kernel has the new > > definition and it caused a build error on old kernels. > > > > We should not assume anything and access them carefully. Do not use > > the task struct directly and access them using new and old definitions > > in a row. > > I added a: > > Fixes: edc41a1099c2d08c ("perf record: Enable off-cpu analysis with BPF") > > Ok? Sure, thanks for doing this! Namhyung
diff --git a/tools/perf/util/bpf_skel/off_cpu.bpf.c b/tools/perf/util/bpf_skel/off_cpu.bpf.c index 792ae2847080..cc6d7fd55118 100644 --- a/tools/perf/util/bpf_skel/off_cpu.bpf.c +++ b/tools/perf/util/bpf_skel/off_cpu.bpf.c @@ -71,6 +71,11 @@ struct { __uint(max_entries, 1); } cgroup_filter SEC(".maps"); +/* new kernel task_struct definition */ +struct task_struct___new { + long __state; +} __attribute__((preserve_access_index)); + /* old kernel task_struct definition */ struct task_struct___old { long state; @@ -93,14 +98,17 @@ const volatile bool uses_cgroup_v1 = false; */ static inline int get_task_state(struct task_struct *t) { - if (bpf_core_field_exists(t->__state)) - return BPF_CORE_READ(t, __state); + /* recast pointer to capture new type for compiler */ + struct task_struct___new *t_new = (void *)t; - /* recast pointer to capture task_struct___old type for compiler */ - struct task_struct___old *t_old = (void *)t; + if (bpf_core_field_exists(t_new->__state)) { + return BPF_CORE_READ(t_new, __state); + } else { + /* recast pointer to capture old type for compiler */ + struct task_struct___old *t_old = (void *)t; - /* now use old "state" name of the field */ - return BPF_CORE_READ(t_old, state); + return BPF_CORE_READ(t_old, state); + } } static inline __u64 get_cgroup_id(struct task_struct *t)
Old kernels have task_struct which contains "state" field and newer kernels have "__state". While the get_task_state() in the BPF code handles that in some way, it assumed the current kernel has the new definition and it caused a build error on old kernels. We should not assume anything and access them carefully. Do not use the task struct directly and access them using new and old definitions in a row. Reported-by: Ian Rogers <irogers@google.com> Signed-off-by: Namhyung Kim <namhyung@kernel.org> --- tools/perf/util/bpf_skel/off_cpu.bpf.c | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-)