diff mbox series

perf stat: Support old kernels for bperf cgroup counting

Message ID 20220922041435.709119-1-namhyung@kernel.org (mailing list archive)
State Not Applicable
Delegated to: BPF
Headers show
Series perf stat: Support old kernels for bperf cgroup counting | expand

Checks

Context Check Description
netdev/tree_selection success Not a local patch

Commit Message

Namhyung Kim Sept. 22, 2022, 4:14 a.m. UTC
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(-)

Comments

Tejun Heo Sept. 24, 2022, 3:22 a.m. UTC | #1
On Wed, Sep 21, 2022 at 09:14:35PM -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.
> 
> 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!

FWIW, looks fine to me and I'd be happy to route this through the cgroup
tree once it gets acked.

Thanks.
Namhyung Kim Sept. 30, 2022, 8:30 p.m. UTC | #2
On Fri, Sep 23, 2022 at 8:22 PM Tejun Heo <tj@kernel.org> wrote:
>
> On Wed, Sep 21, 2022 at 09:14:35PM -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.
> >
> > 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!
>
> FWIW, looks fine to me and I'd be happy to route this through the cgroup
> tree once it gets acked.

Thanks Tejun!

Can any perf + bpf folks take a look?
Jiri Olsa Sept. 30, 2022, 9:43 p.m. UTC | #3
On Wed, Sep 21, 2022 at 09:14:35PM -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.
> 
> 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!

could you please paste the cgroup tree link?

thanks,
jirka

> 
>  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);
> +	} 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();
> @@ -70,7 +97,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.37.3.968.ga6b4b080e4-goog
>
Namhyung Kim Sept. 30, 2022, 9:56 p.m. UTC | #4
Hi Jiri,

On Fri, Sep 30, 2022 at 2:44 PM Jiri Olsa <olsajiri@gmail.com> wrote:
>
> On Wed, Sep 21, 2022 at 09:14:35PM -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.
> >
> > 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!
>
> could you please paste the cgroup tree link?

Do you mean this?

  https://git.kernel.org/pub/scm/linux/kernel/git/tj/cgroup.git

Thanks,.
Namhyung
Arnaldo Carvalho de Melo Sept. 30, 2022, 10 p.m. UTC | #5
On September 30, 2022 6:56:40 PM GMT-03:00, Namhyung Kim <namhyung@kernel.org> wrote:
>Hi Jiri,
>
>On Fri, Sep 30, 2022 at 2:44 PM Jiri Olsa <olsajiri@gmail.com> wrote:
>>
>> On Wed, Sep 21, 2022 at 09:14:35PM -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.
>> >
>> > 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!
>>
>> could you please paste the cgroup tree link?
>
>Do you mean this?
>
>  https://git.kernel.org/pub/scm/linux/kernel/git/tj/cgroup.git
>


Which branch and cset in that tree does you perf skel depends on?

- Arnaldo 
>Thanks,.
>Namhyung
Namhyung Kim Sept. 30, 2022, 10:11 p.m. UTC | #6
On Fri, Sep 30, 2022 at 3:00 PM Arnaldo Carvalho de Melo
<arnaldo.melo@gmail.com> wrote:
>
>
>
> On September 30, 2022 6:56:40 PM GMT-03:00, Namhyung Kim <namhyung@kernel.org> wrote:
> >Hi Jiri,
> >
> >On Fri, Sep 30, 2022 at 2:44 PM Jiri Olsa <olsajiri@gmail.com> wrote:
> >>
> >> On Wed, Sep 21, 2022 at 09:14:35PM -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.
> >> >
> >> > 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!
> >>
> >> could you please paste the cgroup tree link?
> >
> >Do you mean this?
> >
> >  https://git.kernel.org/pub/scm/linux/kernel/git/tj/cgroup.git
> >
>
>
> Which branch and cset in that tree does you perf skel depends on?

I believe it's for-6.1 and the cset is in

  https://git.kernel.org/pub/scm/linux/kernel/git/tj/cgroup.git/commit/?h=for-6.1&id=7f203bc89eb66d6afde7eae91347fc0352090cc3

Thanks,
Namhyung
Andrii Nakryiko Sept. 30, 2022, 10:48 p.m. UTC | #7
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>


> +       } 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();
> @@ -70,7 +97,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.37.3.968.ga6b4b080e4-goog
>
Namhyung Kim Oct. 1, 2022, 2:31 a.m. UTC | #8
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?

Thanks,
Namhyung
Jiri Olsa Oct. 1, 2022, 1:57 p.m. UTC | #9
On Fri, Sep 30, 2022 at 02:56:40PM -0700, Namhyung Kim wrote:
> Hi Jiri,
> 
> On Fri, Sep 30, 2022 at 2:44 PM Jiri Olsa <olsajiri@gmail.com> wrote:
> >
> > On Wed, Sep 21, 2022 at 09:14:35PM -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.
> > >
> > > 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!
> >
> > could you please paste the cgroup tree link?
> 
> Do you mean this?
> 
>   https://git.kernel.org/pub/scm/linux/kernel/git/tj/cgroup.git

yea, wanted to try that.. I cherry-picked the 7f203bc89eb6 ;-)

thanks,
jirka

> 
> Thanks,.
> Namhyung
Jiri Olsa Oct. 1, 2022, 1:58 p.m. UTC | #10
On Wed, Sep 21, 2022 at 09:14:35PM -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.
> 
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>

lgtm

Acked-by: Jiri Olsa <jolsa@kernel.org>

jirka

> ---
> 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);
> +	} 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();
> @@ -70,7 +97,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.37.3.968.ga6b4b080e4-goog
>
Andrii Nakryiko Oct. 5, 2022, 10:36 p.m. UTC | #11
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
Tejun Heo Oct. 10, 2022, 11:59 p.m. UTC | #12
On Wed, Sep 21, 2022 at 09:14:35PM -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.
> 
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>

Looks like it's acked enough but the patch doesn't apply anymore. Namhyung,
can you please refresh the patch? I'll route this through
cgroup/for-6.1-fixes unless somebody objects.

Thanks.
Namhyung Kim Oct. 11, 2022, 5:24 a.m. UTC | #13
Hi Tejun,

On Mon, Oct 10, 2022 at 4:59 PM Tejun Heo <tj@kernel.org> wrote:
>
> On Wed, Sep 21, 2022 at 09:14:35PM -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.
> >
> > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
>
> Looks like it's acked enough but the patch doesn't apply anymore. Namhyung,
> can you please refresh the patch? I'll route this through
> cgroup/for-6.1-fixes unless somebody objects.

Sorry about the conflict.  There was another change to get the perf
cgroup subsys id properly.  Will send v2 soon.

Thanks,
Namhyung
diff mbox series

Patch

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);
+	} 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();
@@ -70,7 +97,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;