[v2] perf stat: Support old kernels for bperf cgroup counting

[PATCH v2] perf stat: Support old kernels for bperf cgroup counting

Posted by Namhyung Kim 3 years, 5 months ago

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>
---
 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 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;
-- 
2.38.0.rc1.362.ged0d419d3c-goog

Re: [PATCH v2] perf stat: Support old kernels for bperf cgroup counting

Posted by Tejun Heo 3 years, 5 months ago

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

Re: [PATCH v2] perf stat: Support old kernels for bperf cgroup counting

Posted by Arnaldo Carvalho de Melo 3 years, 5 months ago

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

Re: [PATCH v2] perf stat: Support old kernels for bperf cgroup counting

Posted by Tejun Heo 3 years, 5 months ago

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?

-- 
tejun

Re: [PATCH v2] perf stat: Support old kernels for bperf cgroup counting

Posted by Arnaldo Carvalho de Melo 3 years, 5 months ago

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

Re: [PATCH v2] perf stat: Support old kernels for bperf cgroup counting

Posted by Arnaldo Carvalho de Melo 3 years, 5 months ago

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