From: Andrii Nakryiko <andrii.nakryiko@gmail.com> To: Namhyung Kim <namhyung@kernel.org> Cc: Tejun Heo <tj@kernel.org>, Zefan Li <lizefan.x@bytedance.com>, Johannes Weiner <hannes@cmpxchg.org>, cgroups <cgroups@vger.kernel.org>, Arnaldo Carvalho de Melo <acme@kernel.org>, Jiri Olsa <jolsa@kernel.org>, LKML <linux-kernel@vger.kernel.org>, linux-perf-users <linux-perf-users@vger.kernel.org>, Song Liu <songliubraving@fb.com>, bpf <bpf@vger.kernel.org> Subject: Re: [PATCH] perf stat: Support old kernels for bperf cgroup counting Date: Wed, 5 Oct 2022 15:36:37 -0700 [thread overview] Message-ID: <CAEf4Bza0dt6v8YLNvQbANH+O9E5naZZj1cc4H6j=KK9WQcym8w@mail.gmail.com> (raw) In-Reply-To: <CAM9d7ch8RUgf8V1hi=ccgV84XSfujuWtUKKgre8eQdGmtdiFLA@mail.gmail.com> On Fri, Sep 30, 2022 at 7:31 PM Namhyung Kim <namhyung@kernel.org> wrote: > > Hello, > > On Fri, Sep 30, 2022 at 3:48 PM Andrii Nakryiko > <andrii.nakryiko@gmail.com> wrote: > > > > On Wed, Sep 21, 2022 at 9:21 PM Namhyung Kim <namhyung@kernel.org> wrote: > > > > > > The recent change in the cgroup will break the backward compatiblity in > > > the BPF program. It should support both old and new kernels using BPF > > > CO-RE technique. > > > > > > Like the task_struct->__state handling in the offcpu analysis, we can > > > check the field name in the cgroup struct. > > > > > > Signed-off-by: Namhyung Kim <namhyung@kernel.org> > > > --- > > > Arnaldo, I think this should go through the cgroup tree since it depends > > > on the earlier change there. I don't think it'd conflict with other > > > perf changes but please let me know if you see any trouble, thanks! > > > > > > tools/perf/util/bpf_skel/bperf_cgroup.bpf.c | 29 ++++++++++++++++++++- > > > 1 file changed, 28 insertions(+), 1 deletion(-) > > > > > > diff --git a/tools/perf/util/bpf_skel/bperf_cgroup.bpf.c b/tools/perf/util/bpf_skel/bperf_cgroup.bpf.c > > > index 488bd398f01d..4fe61043de04 100644 > > > --- a/tools/perf/util/bpf_skel/bperf_cgroup.bpf.c > > > +++ b/tools/perf/util/bpf_skel/bperf_cgroup.bpf.c > > > @@ -43,12 +43,39 @@ struct { > > > __uint(value_size, sizeof(struct bpf_perf_event_value)); > > > } cgrp_readings SEC(".maps"); > > > > > > +/* new kernel cgroup definition */ > > > +struct cgroup___new { > > > + int level; > > > + struct cgroup *ancestors[]; > > > +} __attribute__((preserve_access_index)); > > > + > > > +/* old kernel cgroup definition */ > > > +struct cgroup___old { > > > + int level; > > > + u64 ancestor_ids[]; > > > +} __attribute__((preserve_access_index)); > > > + > > > const volatile __u32 num_events = 1; > > > const volatile __u32 num_cpus = 1; > > > > > > int enabled = 0; > > > int use_cgroup_v2 = 0; > > > > > > +static inline __u64 get_cgroup_v1_ancestor_id(struct cgroup *cgrp, int level) > > > +{ > > > + /* recast pointer to capture new type for compiler */ > > > + struct cgroup___new *cgrp_new = (void *)cgrp; > > > + > > > + if (bpf_core_field_exists(cgrp_new->ancestors)) { > > > + return BPF_CORE_READ(cgrp_new, ancestors[level], kn, id); > > > > have you checked generated BPF code for this ancestors[level] access? > > I'd expect CO-RE relocation for finding ancestors offset and then just > > normal + level * 8 arithmetic, but would be nice to confirm. Apart > > from this, looks good to me: > > > > Acked-by: Andrii Nakryiko <andrii@kernel.org> > > Thanks for your review! > > How can I check the generated code? Do you have something works with > skeletons or do I have to save the BPF object somehow during the build? > skeleton is generated from ELF BPF object file. You can do llvm-objdump -d <obj.bpf.o> to see instructions. Unfortunately you can't see BPF CO-RE relocations this way, you'd have to use something like my custom tool ([0]). But anyways, I checked locally similar code pattern and I think it's all good from BPF CO-RE perspective. I see appropriate relocations in all the necessary places. So this should work. Acked-by: Andrii Nakryiko <andrii@kernel.org> [0] https://github.com/anakryiko/btfdump > Thanks, > Namhyung
WARNING: multiple messages have this Message-ID (diff)
From: Andrii Nakryiko <andrii.nakryiko-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> To: Namhyung Kim <namhyung-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> Cc: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>, Zefan Li <lizefan.x-EC8Uxl6Npydl57MIdRCFDg@public.gmane.org>, Johannes Weiner <hannes-druUgvl0LCNAfugRpC6u6w@public.gmane.org>, cgroups <cgroups-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>, Arnaldo Carvalho de Melo <acme-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>, Jiri Olsa <jolsa-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>, LKML <linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>, linux-perf-users <linux-perf-users-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>, Song Liu <songliubraving-b10kYP2dOMg@public.gmane.org>, bpf <bpf-u79uwXL29TY76Z2rM5mHXA@public.gmane.org> Subject: Re: [PATCH] perf stat: Support old kernels for bperf cgroup counting Date: Wed, 5 Oct 2022 15:36:37 -0700 [thread overview] Message-ID: <CAEf4Bza0dt6v8YLNvQbANH+O9E5naZZj1cc4H6j=KK9WQcym8w@mail.gmail.com> (raw) In-Reply-To: <CAM9d7ch8RUgf8V1hi=ccgV84XSfujuWtUKKgre8eQdGmtdiFLA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> On Fri, Sep 30, 2022 at 7:31 PM Namhyung Kim <namhyung-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote: > > Hello, > > On Fri, Sep 30, 2022 at 3:48 PM Andrii Nakryiko > <andrii.nakryiko-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote: > > > > On Wed, Sep 21, 2022 at 9:21 PM Namhyung Kim <namhyung-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote: > > > > > > The recent change in the cgroup will break the backward compatiblity in > > > the BPF program. It should support both old and new kernels using BPF > > > CO-RE technique. > > > > > > Like the task_struct->__state handling in the offcpu analysis, we can > > > check the field name in the cgroup struct. > > > > > > Signed-off-by: Namhyung Kim <namhyung-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> > > > --- > > > Arnaldo, I think this should go through the cgroup tree since it depends > > > on the earlier change there. I don't think it'd conflict with other > > > perf changes but please let me know if you see any trouble, thanks! > > > > > > tools/perf/util/bpf_skel/bperf_cgroup.bpf.c | 29 ++++++++++++++++++++- > > > 1 file changed, 28 insertions(+), 1 deletion(-) > > > > > > diff --git a/tools/perf/util/bpf_skel/bperf_cgroup.bpf.c b/tools/perf/util/bpf_skel/bperf_cgroup.bpf.c > > > index 488bd398f01d..4fe61043de04 100644 > > > --- a/tools/perf/util/bpf_skel/bperf_cgroup.bpf.c > > > +++ b/tools/perf/util/bpf_skel/bperf_cgroup.bpf.c > > > @@ -43,12 +43,39 @@ struct { > > > __uint(value_size, sizeof(struct bpf_perf_event_value)); > > > } cgrp_readings SEC(".maps"); > > > > > > +/* new kernel cgroup definition */ > > > +struct cgroup___new { > > > + int level; > > > + struct cgroup *ancestors[]; > > > +} __attribute__((preserve_access_index)); > > > + > > > +/* old kernel cgroup definition */ > > > +struct cgroup___old { > > > + int level; > > > + u64 ancestor_ids[]; > > > +} __attribute__((preserve_access_index)); > > > + > > > const volatile __u32 num_events = 1; > > > const volatile __u32 num_cpus = 1; > > > > > > int enabled = 0; > > > int use_cgroup_v2 = 0; > > > > > > +static inline __u64 get_cgroup_v1_ancestor_id(struct cgroup *cgrp, int level) > > > +{ > > > + /* recast pointer to capture new type for compiler */ > > > + struct cgroup___new *cgrp_new = (void *)cgrp; > > > + > > > + if (bpf_core_field_exists(cgrp_new->ancestors)) { > > > + return BPF_CORE_READ(cgrp_new, ancestors[level], kn, id); > > > > have you checked generated BPF code for this ancestors[level] access? > > I'd expect CO-RE relocation for finding ancestors offset and then just > > normal + level * 8 arithmetic, but would be nice to confirm. Apart > > from this, looks good to me: > > > > Acked-by: Andrii Nakryiko <andrii-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> > > Thanks for your review! > > How can I check the generated code? Do you have something works with > skeletons or do I have to save the BPF object somehow during the build? > skeleton is generated from ELF BPF object file. You can do llvm-objdump -d <obj.bpf.o> to see instructions. Unfortunately you can't see BPF CO-RE relocations this way, you'd have to use something like my custom tool ([0]). But anyways, I checked locally similar code pattern and I think it's all good from BPF CO-RE perspective. I see appropriate relocations in all the necessary places. So this should work. Acked-by: Andrii Nakryiko <andrii-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> [0] https://github.com/anakryiko/btfdump > Thanks, > Namhyung
next prev parent reply other threads:[~2022-10-05 22:36 UTC|newest] Thread overview: 47+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-07-29 19:05 [PATCH cgroup/for-5.20] cgroup: Replace cgroup->ancestor_ids[] with ->ancestors[] Tejun Heo 2022-07-29 19:05 ` Tejun Heo 2022-07-29 20:30 ` Namhyung Kim 2022-07-29 20:30 ` Namhyung Kim 2022-07-29 20:58 ` [PATCH v2 " Tejun Heo 2022-07-29 20:58 ` Tejun Heo 2022-07-29 22:42 ` Michal Koutný 2022-07-29 22:42 ` Michal Koutný 2022-07-29 23:02 ` Tejun Heo 2022-07-29 23:02 ` Tejun Heo 2022-07-29 23:10 ` [PATCH v3 " Tejun Heo 2022-08-15 21:17 ` Tejun Heo 2022-09-22 4:05 ` Namhyung Kim 2022-09-22 4:05 ` Namhyung Kim 2022-09-22 4:14 ` [PATCH] perf stat: Support old kernels for bperf cgroup counting Namhyung Kim 2022-09-22 4:14 ` Namhyung Kim 2022-09-24 3:22 ` Tejun Heo 2022-09-30 20:30 ` Namhyung Kim 2022-09-30 20:30 ` Namhyung Kim 2022-09-30 21:43 ` Jiri Olsa 2022-09-30 21:56 ` Namhyung Kim 2022-09-30 21:56 ` Namhyung Kim 2022-09-30 22:00 ` Arnaldo Carvalho de Melo 2022-09-30 22:00 ` Arnaldo Carvalho de Melo 2022-09-30 22:11 ` Namhyung Kim 2022-09-30 22:11 ` Namhyung Kim 2022-10-01 13:57 ` Jiri Olsa 2022-10-01 13:57 ` Jiri Olsa 2022-09-30 22:48 ` Andrii Nakryiko 2022-10-01 2:31 ` Namhyung Kim 2022-10-05 22:36 ` Andrii Nakryiko [this message] 2022-10-05 22:36 ` Andrii Nakryiko 2022-10-01 13:58 ` Jiri Olsa 2022-10-01 13:58 ` Jiri Olsa 2022-10-10 23:59 ` Tejun Heo 2022-10-11 5:24 ` Namhyung Kim 2022-10-11 5:24 ` Namhyung Kim 2022-10-11 5:28 ` [PATCH v2] " Namhyung Kim 2022-10-11 5:28 ` Namhyung Kim 2022-10-11 16:53 ` Tejun Heo 2022-10-14 13:27 ` Arnaldo Carvalho de Melo 2022-10-14 13:30 ` Arnaldo Carvalho de Melo 2022-10-14 13:30 ` Arnaldo Carvalho de Melo 2022-10-14 16:40 ` Tejun Heo 2022-10-14 16:40 ` Tejun Heo 2022-10-14 17:10 ` Arnaldo Carvalho de Melo 2022-10-14 17:10 ` Arnaldo Carvalho de Melo
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to='CAEf4Bza0dt6v8YLNvQbANH+O9E5naZZj1cc4H6j=KK9WQcym8w@mail.gmail.com' \ --to=andrii.nakryiko@gmail.com \ --cc=acme@kernel.org \ --cc=bpf@vger.kernel.org \ --cc=cgroups@vger.kernel.org \ --cc=hannes@cmpxchg.org \ --cc=jolsa@kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-perf-users@vger.kernel.org \ --cc=lizefan.x@bytedance.com \ --cc=namhyung@kernel.org \ --cc=songliubraving@fb.com \ --cc=tj@kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.