| Message ID | 20221011052808.282394-1-namhyung@kernel.org (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Delegated to: | BPF |
| Headers | show |
| Series | [v2] perf stat: Support old kernels for bperf cgroup counting | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
| bpf/vmtest-bpf-PR | fail | merge-conflict |
On Mon, Oct 10, 2022 at 10:28:08PM -0700, Namhyung Kim 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. > > Acked-by: Jiri Olsa <jolsa@kernel.org> > Acked-by: Andrii Nakryiko <andrii@kernel.org> > Signed-off-by: Namhyung Kim <namhyung@kernel.org> Applied to cgroup/for-6.1-fixes. Thanks.
Em Tue, Oct 11, 2022 at 06:53:43AM -1000, Tejun Heo escreveu: > On Mon, Oct 10, 2022 at 10:28:08PM -0700, Namhyung Kim 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. > > Acked-by: Jiri Olsa <jolsa@kernel.org> > > Acked-by: Andrii Nakryiko <andrii@kernel.org> > > Signed-off-by: Namhyung Kim <namhyung@kernel.org> > Applied to cgroup/for-6.1-fixes. Hey, I noticed that the perf build is broken for the tools/perf/util/bpf_skel/bperf_cgroup.bpf.c skell, so I tried using b4 on this Namhyung patch, it ended up getting a newer version, by Tejun, that mixes up kernel code and tooling, which, when I tried to apply upstream didn't work. Please try not to mix up kernel and tools/ changes in the same patch to avoid these issues. Also when changing tools/perf, please CC me. I'm now back trying to apply v2 of this patch to see if it fixes my build. Thanks, - Arnaldo
Em Fri, Oct 14, 2022 at 10:27:40AM -0300, Arnaldo Carvalho de Melo escreveu: > Em Tue, Oct 11, 2022 at 06:53:43AM -1000, Tejun Heo escreveu: > > On Mon, Oct 10, 2022 at 10:28:08PM -0700, Namhyung Kim 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. > > > > Acked-by: Jiri Olsa <jolsa@kernel.org> > > > Acked-by: Andrii Nakryiko <andrii@kernel.org> > > > Signed-off-by: Namhyung Kim <namhyung@kernel.org> > > > Applied to cgroup/for-6.1-fixes. > > Hey, I noticed that the perf build is broken for the > tools/perf/util/bpf_skel/bperf_cgroup.bpf.c skell, so I tried using b4 > on this Namhyung patch, it ended up getting a newer version, by Tejun, > that mixes up kernel code and tooling, which, when I tried to apply > upstream didn't work. > > Please try not to mix up kernel and tools/ changes in the same patch to > avoid these issues. > > Also when changing tools/perf, please CC me. > > I'm now back trying to apply v2 of this patch to see if it fixes my > build. Yeah, applying just Namhyung's v2 patch gets perf back building, I'll keep it there while processing the other patches so that I can test them all together. - Arnaldo
On Fri, Oct 14, 2022 at 10:27:40AM -0300, Arnaldo Carvalho de Melo wrote: > Hey, I noticed that the perf build is broken for the > tools/perf/util/bpf_skel/bperf_cgroup.bpf.c skell, so I tried using b4 > on this Namhyung patch, it ended up getting a newer version, by Tejun, > that mixes up kernel code and tooling, which, when I tried to apply > upstream didn't work. > > Please try not to mix up kernel and tools/ changes in the same patch to > avoid these issues. I didn't write a newer version of this patch. What are you talking about?
Em Fri, Oct 14, 2022 at 06:40:56AM -1000, Tejun Heo escreveu: > On Fri, Oct 14, 2022 at 10:27:40AM -0300, Arnaldo Carvalho de Melo wrote: > > Hey, I noticed that the perf build is broken for the > > tools/perf/util/bpf_skel/bperf_cgroup.bpf.c skell, so I tried using b4 > > on this Namhyung patch, it ended up getting a newer version, by Tejun, > > that mixes up kernel code and tooling, which, when I tried to apply > > upstream didn't work. > > Please try not to mix up kernel and tools/ changes in the same patch to > > avoid these issues. > I didn't write a newer version of this patch. What are you talking about? So, I saw this message from you in reply to Namhyung's v2 patch: -------------------------- Date: Tue, 11 Oct 2022 06:53:43 -1000 From: Tejun Heo <tj@kernel.org> Subject: Re: [PATCH v2] perf stat: Support old kernels for bperf cgroup counting To: Namhyung Kim <namhyung@kernel.org> Cc: Arnaldo Carvalho de Melo <acme@kernel.org>, Zefan Li <lizefan.x@bytedance.com>, Johannes Weiner <hannes@cmpxchg.org>, cgroups@vger.kernel.org, Jiri Olsa <jolsa@kernel.org>, LKML <linux-kernel@vger.kernel.org>, linux-perf-users@vger.kernel.org, Song Liu <songliubraving@fb.com>, bpf@vger.kernel.org, Andrii Nakryiko <andrii@kernel.org> Sender: Tejun Heo <htejun@gmail.com> Message-ID: <Y0Wfl88objrECjSo@slm.duckdns.org> On Mon, Oct 10, 2022 at 10:28:08PM -0700, Namhyung Kim 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. > Acked-by: Jiri Olsa <jolsa@kernel.org> > Acked-by: Andrii Nakryiko <andrii@kernel.org> > Signed-off-by: Namhyung Kim <namhyung@kernel.org> Applied to cgroup/for-6.1-fixes. Thanks. -- tejun -------------------------- So, I picked the message id, Y0Wfl88objrECjSo@slm.duckdns.org, and asked b4 to pick the patch: ⬢[acme@toolbox perf]$ b4 am --help | grep -A1 -- -c, -c, --check-newer-revisions Check if newer patch revisions exist ⬢[acme@toolbox perf]$ ⬢[acme@toolbox perf]$ b4 am -ctsl --cc-trailers Y0Wfl88objrECjSo@slm.duckdns.org Grabbing thread from lore.kernel.org/all/Y0Wfl88objrECjSo%40slm.duckdns.org/t.mbox.gz Checking for newer revisions on https://lore.kernel.org/all/ Analyzing 27 messages in the thread ('Acked-by', 'Andrii Nakryiko <andrii@kernel.org>', None) Will use the latest revision: v3 You can pick other revisions using the -vN flag Checking attestation on all messages, may take a moment... --- ✓ [PATCH v3] cgroup: Replace cgroup->ancestor_ids[] with ->ancestors[] --- ✓ Signed: DKIM/gmail.com (From: tj@kernel.org) --- Total patches: 1 --- Link: https://lore.kernel.org/r/YuRo2PLFH6wLgEkm@slm.duckdns.org Base: not specified git am ./v3_20220729_tj_cgroup_replace_cgroup_ancestor_ids_with_ancestors.mbx ⬢[acme@toolbox perf]$ Which got me this: ⬢[acme@toolbox perf]$ diffstat ./v3_20220729_tj_cgroup_replace_cgroup_ancestor_ids_with_ancestors.mbx include/linux/cgroup-defs.h | 16 ++++++++++------ include/linux/cgroup.h | 8 +++----- kernel/cgroup/cgroup.c | 7 +++---- net/netfilter/nft_socket.c | 9 +++++---- tools/perf/util/bpf_skel/bperf_cgroup.bpf.c | 2 +- 5 files changed, 22 insertions(+), 20 deletions(-) ⬢[acme@toolbox perf]$ ⬢[acme@toolbox perf]$ grep From: ./v3_20220729_tj_cgroup_replace_cgroup_ancestor_ids_with_ancestors.mbx From: Tejun Heo <tj@kernel.org> ⬢[acme@toolbox perf]$ That mixes kernel and tools bits and touches tools/perf/util/bpf_skel/bperf_cgroup.bpf.c, hence my request to add me to the CC list for patches touching tools/perf/. My assumption that it was a new patch was because b4 somehow got to v3_20220729_tj_cgroup_replace_cgroup_ancestor_ids_with_ancestors, which has v3 and touches the tools cgroup bpf skel. So it seems b4 is confused somehow. Hope this clarifies. - Arnaldo
diff --git a/tools/perf/util/bpf_skel/bperf_cgroup.bpf.c b/tools/perf/util/bpf_skel/bperf_cgroup.bpf.c index 435a87556688..6a438e0102c5 100644 --- a/tools/perf/util/bpf_skel/bperf_cgroup.bpf.c +++ b/tools/perf/util/bpf_skel/bperf_cgroup.bpf.c @@ -43,6 +43,18 @@ 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; @@ -50,6 +62,21 @@ int enabled = 0; int use_cgroup_v2 = 0; int perf_subsys_id = -1; +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); + } else { + /* recast pointer to capture old type for compiler */ + struct cgroup___old *cgrp_old = (void *)cgrp; + + return BPF_CORE_READ(cgrp_old, ancestor_ids[level]); + } +} + static inline int get_cgroup_v1_idx(__u32 *cgrps, int size) { struct task_struct *p = (void *)bpf_get_current_task(); @@ -77,7 +104,7 @@ static inline int get_cgroup_v1_idx(__u32 *cgrps, int size) break; // convert cgroup-id to a map index - cgrp_id = BPF_CORE_READ(cgrp, ancestors[i], kn, id); + cgrp_id = get_cgroup_v1_ancestor_id(cgrp, i); elem = bpf_map_lookup_elem(&cgrp_idx, &cgrp_id); if (!elem) continue;