I found that checking cgroup sampling support using the missing
features doesn't work on old kernels. Because it added both
attr.cgroup bit and PERF_SAMPLE_CGROUP bit, it needs to check
whichever comes first (usually the actual event, not dummy).
But it only checks the attr.cgroup bit which is set only in the dummy
event so cannot detect failtures due the sample bits. Also we don't
ignore the missing feature and retry, it'd be better checking it with
the API probing logic.
Signed-off-by: Namhyung Kim <[email protected]>
---
tools/perf/builtin-record.c | 6 ++++++
tools/perf/util/evsel.c | 6 +-----
tools/perf/util/evsel.h | 1 -
tools/perf/util/perf_api_probe.c | 10 ++++++++++
tools/perf/util/perf_api_probe.h | 1 +
5 files changed, 18 insertions(+), 6 deletions(-)
diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index bc3dd379eb67..71efe6573ee7 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -2733,6 +2733,12 @@ int cmd_record(int argc, const char **argv)
rec->no_buildid = true;
}
+ if (rec->opts.record_cgroup && !perf_can_record_cgroup()) {
+ pr_err("Kernel has no cgroup sampling support.\n");
+ err = -EINVAL;
+ goto out_opts;
+ }
+
if (rec->opts.kcore)
rec->data.is_dir = true;
diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index 4a3cd1b5bb33..2462584d0ee5 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -1217,7 +1217,7 @@ void evsel__config(struct evsel *evsel, struct record_opts *opts,
attr->namespaces = track;
if (opts->record_cgroup) {
- attr->cgroup = track && !perf_missing_features.cgroup;
+ attr->cgroup = track;
evsel__set_sample_bit(evsel, CGROUP);
}
@@ -1933,10 +1933,6 @@ static int evsel__open_cpu(struct evsel *evsel, struct perf_cpu_map *cpus,
perf_missing_features.data_page_size = true;
pr_debug2_peo("Kernel has no PERF_SAMPLE_DATA_PAGE_SIZE support, bailing out\n");
goto out_close;
- } else if (!perf_missing_features.cgroup && evsel->core.attr.cgroup) {
- perf_missing_features.cgroup = true;
- pr_debug2_peo("Kernel has no cgroup sampling support, bailing out\n");
- goto out_close;
} else if (!perf_missing_features.branch_hw_idx &&
(evsel->core.attr.branch_sample_type & PERF_SAMPLE_BRANCH_HW_INDEX)) {
perf_missing_features.branch_hw_idx = true;
diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
index 75cf5dbfe208..fecf13c0e4da 100644
--- a/tools/perf/util/evsel.h
+++ b/tools/perf/util/evsel.h
@@ -168,7 +168,6 @@ struct perf_missing_features {
bool bpf;
bool aux_output;
bool branch_hw_idx;
- bool cgroup;
bool data_page_size;
bool code_page_size;
bool weight_struct;
diff --git a/tools/perf/util/perf_api_probe.c b/tools/perf/util/perf_api_probe.c
index 829af17a0867..020411682a3c 100644
--- a/tools/perf/util/perf_api_probe.c
+++ b/tools/perf/util/perf_api_probe.c
@@ -103,6 +103,11 @@ static void perf_probe_build_id(struct evsel *evsel)
evsel->core.attr.build_id = 1;
}
+static void perf_probe_cgroup(struct evsel *evsel)
+{
+ evsel->core.attr.cgroup = 1;
+}
+
bool perf_can_sample_identifier(void)
{
return perf_probe_api(perf_probe_sample_identifier);
@@ -182,3 +187,8 @@ bool perf_can_record_build_id(void)
{
return perf_probe_api(perf_probe_build_id);
}
+
+bool perf_can_record_cgroup(void)
+{
+ return perf_probe_api(perf_probe_cgroup);
+}
diff --git a/tools/perf/util/perf_api_probe.h b/tools/perf/util/perf_api_probe.h
index f12ca55f509a..b104168efb15 100644
--- a/tools/perf/util/perf_api_probe.h
+++ b/tools/perf/util/perf_api_probe.h
@@ -12,5 +12,6 @@ bool perf_can_record_switch_events(void);
bool perf_can_record_text_poke_events(void);
bool perf_can_sample_identifier(void);
bool perf_can_record_build_id(void);
+bool perf_can_record_cgroup(void);
#endif // __PERF_API_PROBE_H
--
2.32.0.rc0.204.g9fa02ecfa5-goog
On Thu, May 27, 2021 at 11:28:35AM -0700, Namhyung Kim wrote:
> I found that checking cgroup sampling support using the missing
> features doesn't work on old kernels. Because it added both
> attr.cgroup bit and PERF_SAMPLE_CGROUP bit, it needs to check
> whichever comes first (usually the actual event, not dummy).
>
> But it only checks the attr.cgroup bit which is set only in the dummy
> event so cannot detect failtures due the sample bits. Also we don't
> ignore the missing feature and retry, it'd be better checking it with
> the API probing logic.
>
> Signed-off-by: Namhyung Kim <[email protected]>
Acked-by: Jiri Olsa <[email protected]>
thanks,
jirka
> ---
> tools/perf/builtin-record.c | 6 ++++++
> tools/perf/util/evsel.c | 6 +-----
> tools/perf/util/evsel.h | 1 -
> tools/perf/util/perf_api_probe.c | 10 ++++++++++
> tools/perf/util/perf_api_probe.h | 1 +
> 5 files changed, 18 insertions(+), 6 deletions(-)
>
> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> index bc3dd379eb67..71efe6573ee7 100644
> --- a/tools/perf/builtin-record.c
> +++ b/tools/perf/builtin-record.c
> @@ -2733,6 +2733,12 @@ int cmd_record(int argc, const char **argv)
> rec->no_buildid = true;
> }
>
> + if (rec->opts.record_cgroup && !perf_can_record_cgroup()) {
> + pr_err("Kernel has no cgroup sampling support.\n");
> + err = -EINVAL;
> + goto out_opts;
> + }
> +
> if (rec->opts.kcore)
> rec->data.is_dir = true;
>
> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> index 4a3cd1b5bb33..2462584d0ee5 100644
> --- a/tools/perf/util/evsel.c
> +++ b/tools/perf/util/evsel.c
> @@ -1217,7 +1217,7 @@ void evsel__config(struct evsel *evsel, struct record_opts *opts,
> attr->namespaces = track;
>
> if (opts->record_cgroup) {
> - attr->cgroup = track && !perf_missing_features.cgroup;
> + attr->cgroup = track;
> evsel__set_sample_bit(evsel, CGROUP);
> }
>
> @@ -1933,10 +1933,6 @@ static int evsel__open_cpu(struct evsel *evsel, struct perf_cpu_map *cpus,
> perf_missing_features.data_page_size = true;
> pr_debug2_peo("Kernel has no PERF_SAMPLE_DATA_PAGE_SIZE support, bailing out\n");
> goto out_close;
> - } else if (!perf_missing_features.cgroup && evsel->core.attr.cgroup) {
> - perf_missing_features.cgroup = true;
> - pr_debug2_peo("Kernel has no cgroup sampling support, bailing out\n");
> - goto out_close;
> } else if (!perf_missing_features.branch_hw_idx &&
> (evsel->core.attr.branch_sample_type & PERF_SAMPLE_BRANCH_HW_INDEX)) {
> perf_missing_features.branch_hw_idx = true;
> diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
> index 75cf5dbfe208..fecf13c0e4da 100644
> --- a/tools/perf/util/evsel.h
> +++ b/tools/perf/util/evsel.h
> @@ -168,7 +168,6 @@ struct perf_missing_features {
> bool bpf;
> bool aux_output;
> bool branch_hw_idx;
> - bool cgroup;
> bool data_page_size;
> bool code_page_size;
> bool weight_struct;
> diff --git a/tools/perf/util/perf_api_probe.c b/tools/perf/util/perf_api_probe.c
> index 829af17a0867..020411682a3c 100644
> --- a/tools/perf/util/perf_api_probe.c
> +++ b/tools/perf/util/perf_api_probe.c
> @@ -103,6 +103,11 @@ static void perf_probe_build_id(struct evsel *evsel)
> evsel->core.attr.build_id = 1;
> }
>
> +static void perf_probe_cgroup(struct evsel *evsel)
> +{
> + evsel->core.attr.cgroup = 1;
> +}
> +
> bool perf_can_sample_identifier(void)
> {
> return perf_probe_api(perf_probe_sample_identifier);
> @@ -182,3 +187,8 @@ bool perf_can_record_build_id(void)
> {
> return perf_probe_api(perf_probe_build_id);
> }
> +
> +bool perf_can_record_cgroup(void)
> +{
> + return perf_probe_api(perf_probe_cgroup);
> +}
> diff --git a/tools/perf/util/perf_api_probe.h b/tools/perf/util/perf_api_probe.h
> index f12ca55f509a..b104168efb15 100644
> --- a/tools/perf/util/perf_api_probe.h
> +++ b/tools/perf/util/perf_api_probe.h
> @@ -12,5 +12,6 @@ bool perf_can_record_switch_events(void);
> bool perf_can_record_text_poke_events(void);
> bool perf_can_sample_identifier(void);
> bool perf_can_record_build_id(void);
> +bool perf_can_record_cgroup(void);
>
> #endif // __PERF_API_PROBE_H
> --
> 2.32.0.rc0.204.g9fa02ecfa5-goog
>
Em Thu, May 27, 2021 at 11:28:35AM -0700, Namhyung Kim escreveu:
> I found that checking cgroup sampling support using the missing
> features doesn't work on old kernels. Because it added both
> attr.cgroup bit and PERF_SAMPLE_CGROUP bit, it needs to check
> whichever comes first (usually the actual event, not dummy).
>
> But it only checks the attr.cgroup bit which is set only in the dummy
> event so cannot detect failtures due the sample bits. Also we don't
> ignore the missing feature and retry, it'd be better checking it with
> the API probing logic.
>
> Signed-off-by: Namhyung Kim <[email protected]>
> ---
> tools/perf/builtin-record.c | 6 ++++++
> tools/perf/util/evsel.c | 6 +-----
> tools/perf/util/evsel.h | 1 -
> tools/perf/util/perf_api_probe.c | 10 ++++++++++
> tools/perf/util/perf_api_probe.h | 1 +
> 5 files changed, 18 insertions(+), 6 deletions(-)
>
> diff --git a/tools/perf/util/perf_api_probe.c b/tools/perf/util/perf_api_probe.c
> index 829af17a0867..020411682a3c 100644
> --- a/tools/perf/util/perf_api_probe.c
> +++ b/tools/perf/util/perf_api_probe.c
> @@ -103,6 +103,11 @@ static void perf_probe_build_id(struct evsel *evsel)
> evsel->core.attr.build_id = 1;
> }
>
> +static void perf_probe_cgroup(struct evsel *evsel)
> +{
> + evsel->core.attr.cgroup = 1;
> +}
> +
> bool perf_can_sample_identifier(void)
> {
> return perf_probe_api(perf_probe_sample_identifier);
> @@ -182,3 +187,8 @@ bool perf_can_record_build_id(void)
> {
> return perf_probe_api(perf_probe_build_id);
> }
> +
> +bool perf_can_record_cgroup(void)
> +{
> + return perf_probe_api(perf_probe_cgroup);
> +}
> diff --git a/tools/perf/util/perf_api_probe.h b/tools/perf/util/perf_api_probe.h
> index f12ca55f509a..b104168efb15 100644
> --- a/tools/perf/util/perf_api_probe.h
> +++ b/tools/perf/util/perf_api_probe.h
> @@ -12,5 +12,6 @@ bool perf_can_record_switch_events(void);
> bool perf_can_record_text_poke_events(void);
> bool perf_can_sample_identifier(void);
> bool perf_can_record_build_id(void);
> +bool perf_can_record_cgroup(void);
>
> #endif // __PERF_API_PROBE_H
> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> index bc3dd379eb67..71efe6573ee7 100644
> --- a/tools/perf/builtin-record.c
> +++ b/tools/perf/builtin-record.c
> @@ -2733,6 +2733,12 @@ int cmd_record(int argc, const char **argv)
> rec->no_buildid = true;
> }
>
> + if (rec->opts.record_cgroup && !perf_can_record_cgroup()) {
> + pr_err("Kernel has no cgroup sampling support.\n");
> + err = -EINVAL;
> + goto out_opts;
> + }
> +
> if (rec->opts.kcore)
> rec->data.is_dir = true;
>
The above is perf/urgent material and should fix your issue, right?
The part below is a separate patch and can be left for later, or maybe
remain in the codebase, as simple tools that use just one evsel and
request a cgroup will continue probing the kernel, etc. I.e. it
shouldn't get in the way for cases with dummies, etc.
Simple tools then won't have to get that !perf_can_record_cgroup() call.
- Arnaldo
> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> index 4a3cd1b5bb33..2462584d0ee5 100644
> --- a/tools/perf/util/evsel.c
> +++ b/tools/perf/util/evsel.c
> @@ -1217,7 +1217,7 @@ void evsel__config(struct evsel *evsel, struct record_opts *opts,
> attr->namespaces = track;
>
> if (opts->record_cgroup) {
> - attr->cgroup = track && !perf_missing_features.cgroup;
> + attr->cgroup = track;
> evsel__set_sample_bit(evsel, CGROUP);
> }
>
> @@ -1933,10 +1933,6 @@ static int evsel__open_cpu(struct evsel *evsel, struct perf_cpu_map *cpus,
> perf_missing_features.data_page_size = true;
> pr_debug2_peo("Kernel has no PERF_SAMPLE_DATA_PAGE_SIZE support, bailing out\n");
> goto out_close;
> - } else if (!perf_missing_features.cgroup && evsel->core.attr.cgroup) {
> - perf_missing_features.cgroup = true;
> - pr_debug2_peo("Kernel has no cgroup sampling support, bailing out\n");
> - goto out_close;
> } else if (!perf_missing_features.branch_hw_idx &&
> (evsel->core.attr.branch_sample_type & PERF_SAMPLE_BRANCH_HW_INDEX)) {
> perf_missing_features.branch_hw_idx = true;
> diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
> index 75cf5dbfe208..fecf13c0e4da 100644
> --- a/tools/perf/util/evsel.h
> +++ b/tools/perf/util/evsel.h
> @@ -168,7 +168,6 @@ struct perf_missing_features {
> bool bpf;
> bool aux_output;
> bool branch_hw_idx;
> - bool cgroup;
> bool data_page_size;
> bool code_page_size;
> bool weight_struct;
> --
> 2.32.0.rc0.204.g9fa02ecfa5-goog
>
--
- Arnaldo
Em Tue, Jun 01, 2021 at 10:30:25AM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Thu, May 27, 2021 at 11:28:35AM -0700, Namhyung Kim escreveu:
> > I found that checking cgroup sampling support using the missing
> > features doesn't work on old kernels. Because it added both
> > attr.cgroup bit and PERF_SAMPLE_CGROUP bit, it needs to check
> > whichever comes first (usually the actual event, not dummy).
> >
> > But it only checks the attr.cgroup bit which is set only in the dummy
> > event so cannot detect failtures due the sample bits. Also we don't
> > ignore the missing feature and retry, it'd be better checking it with
> > the API probing logic.
> >
> > Signed-off-by: Namhyung Kim <[email protected]>
> > ---
> > tools/perf/builtin-record.c | 6 ++++++
> > tools/perf/util/evsel.c | 6 +-----
> > tools/perf/util/evsel.h | 1 -
> > tools/perf/util/perf_api_probe.c | 10 ++++++++++
> > tools/perf/util/perf_api_probe.h | 1 +
> > 5 files changed, 18 insertions(+), 6 deletions(-)
> >
> > diff --git a/tools/perf/util/perf_api_probe.c b/tools/perf/util/perf_api_probe.c
> > index 829af17a0867..020411682a3c 100644
> > --- a/tools/perf/util/perf_api_probe.c
> > +++ b/tools/perf/util/perf_api_probe.c
> > @@ -103,6 +103,11 @@ static void perf_probe_build_id(struct evsel *evsel)
> > evsel->core.attr.build_id = 1;
> > }
> >
> > +static void perf_probe_cgroup(struct evsel *evsel)
> > +{
> > + evsel->core.attr.cgroup = 1;
> > +}
> > +
> > bool perf_can_sample_identifier(void)
> > {
> > return perf_probe_api(perf_probe_sample_identifier);
> > @@ -182,3 +187,8 @@ bool perf_can_record_build_id(void)
> > {
> > return perf_probe_api(perf_probe_build_id);
> > }
> > +
> > +bool perf_can_record_cgroup(void)
> > +{
> > + return perf_probe_api(perf_probe_cgroup);
> > +}
> > diff --git a/tools/perf/util/perf_api_probe.h b/tools/perf/util/perf_api_probe.h
> > index f12ca55f509a..b104168efb15 100644
> > --- a/tools/perf/util/perf_api_probe.h
> > +++ b/tools/perf/util/perf_api_probe.h
> > @@ -12,5 +12,6 @@ bool perf_can_record_switch_events(void);
> > bool perf_can_record_text_poke_events(void);
> > bool perf_can_sample_identifier(void);
> > bool perf_can_record_build_id(void);
> > +bool perf_can_record_cgroup(void);
> >
> > #endif // __PERF_API_PROBE_H
> > diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> > index bc3dd379eb67..71efe6573ee7 100644
> > --- a/tools/perf/builtin-record.c
> > +++ b/tools/perf/builtin-record.c
> > @@ -2733,6 +2733,12 @@ int cmd_record(int argc, const char **argv)
> > rec->no_buildid = true;
> > }
> >
> > + if (rec->opts.record_cgroup && !perf_can_record_cgroup()) {
> > + pr_err("Kernel has no cgroup sampling support.\n");
> > + err = -EINVAL;
> > + goto out_opts;
> > + }
> > +
> > if (rec->opts.kcore)
> > rec->data.is_dir = true;
> >
>
> The above is perf/urgent material and should fix your issue, right?
>
> The part below is a separate patch and can be left for later, or maybe
> remain in the codebase, as simple tools that use just one evsel and
> request a cgroup will continue probing the kernel, etc. I.e. it
> shouldn't get in the way for cases with dummies, etc.
>
> Simple tools then won't have to get that !perf_can_record_cgroup() call.
I did it tentatively in my local branch, i.e. removed the removal of the
fallback part, thus is just for it to be tested by the containers setup,
etc, we can change this before it hits acme/perf/core externally.
- Arnaldo
Hi Arnaldo,
On Tue, Jun 1, 2021 at 6:34 AM Arnaldo Carvalho de Melo <[email protected]> wrote:
>
> Em Tue, Jun 01, 2021 at 10:30:25AM -0300, Arnaldo Carvalho de Melo escreveu:
> > Em Thu, May 27, 2021 at 11:28:35AM -0700, Namhyung Kim escreveu:
> > > I found that checking cgroup sampling support using the missing
> > > features doesn't work on old kernels. Because it added both
> > > attr.cgroup bit and PERF_SAMPLE_CGROUP bit, it needs to check
> > > whichever comes first (usually the actual event, not dummy).
> > >
> > > But it only checks the attr.cgroup bit which is set only in the dummy
> > > event so cannot detect failtures due the sample bits. Also we don't
> > > ignore the missing feature and retry, it'd be better checking it with
> > > the API probing logic.
> > >
> > > Signed-off-by: Namhyung Kim <[email protected]>
> > > ---
> > > tools/perf/builtin-record.c | 6 ++++++
> > > tools/perf/util/evsel.c | 6 +-----
> > > tools/perf/util/evsel.h | 1 -
> > > tools/perf/util/perf_api_probe.c | 10 ++++++++++
> > > tools/perf/util/perf_api_probe.h | 1 +
> > > 5 files changed, 18 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/tools/perf/util/perf_api_probe.c b/tools/perf/util/perf_api_probe.c
> > > index 829af17a0867..020411682a3c 100644
> > > --- a/tools/perf/util/perf_api_probe.c
> > > +++ b/tools/perf/util/perf_api_probe.c
> > > @@ -103,6 +103,11 @@ static void perf_probe_build_id(struct evsel *evsel)
> > > evsel->core.attr.build_id = 1;
> > > }
> > >
> > > +static void perf_probe_cgroup(struct evsel *evsel)
> > > +{
> > > + evsel->core.attr.cgroup = 1;
> > > +}
> > > +
> > > bool perf_can_sample_identifier(void)
> > > {
> > > return perf_probe_api(perf_probe_sample_identifier);
> > > @@ -182,3 +187,8 @@ bool perf_can_record_build_id(void)
> > > {
> > > return perf_probe_api(perf_probe_build_id);
> > > }
> > > +
> > > +bool perf_can_record_cgroup(void)
> > > +{
> > > + return perf_probe_api(perf_probe_cgroup);
> > > +}
> > > diff --git a/tools/perf/util/perf_api_probe.h b/tools/perf/util/perf_api_probe.h
> > > index f12ca55f509a..b104168efb15 100644
> > > --- a/tools/perf/util/perf_api_probe.h
> > > +++ b/tools/perf/util/perf_api_probe.h
> > > @@ -12,5 +12,6 @@ bool perf_can_record_switch_events(void);
> > > bool perf_can_record_text_poke_events(void);
> > > bool perf_can_sample_identifier(void);
> > > bool perf_can_record_build_id(void);
> > > +bool perf_can_record_cgroup(void);
> > >
> > > #endif // __PERF_API_PROBE_H
> > > diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> > > index bc3dd379eb67..71efe6573ee7 100644
> > > --- a/tools/perf/builtin-record.c
> > > +++ b/tools/perf/builtin-record.c
> > > @@ -2733,6 +2733,12 @@ int cmd_record(int argc, const char **argv)
> > > rec->no_buildid = true;
> > > }
> > >
> > > + if (rec->opts.record_cgroup && !perf_can_record_cgroup()) {
> > > + pr_err("Kernel has no cgroup sampling support.\n");
> > > + err = -EINVAL;
> > > + goto out_opts;
> > > + }
> > > +
> > > if (rec->opts.kcore)
> > > rec->data.is_dir = true;
> > >
> >
> > The above is perf/urgent material and should fix your issue, right?
> >
> > The part below is a separate patch and can be left for later, or maybe
> > remain in the codebase, as simple tools that use just one evsel and
> > request a cgroup will continue probing the kernel, etc. I.e. it
> > shouldn't get in the way for cases with dummies, etc.
> >
> > Simple tools then won't have to get that !perf_can_record_cgroup() call.
>
> I did it tentatively in my local branch, i.e. removed the removal of the
> fallback part, thus is just for it to be tested by the containers setup,
> etc, we can change this before it hits acme/perf/core externally.
Sure, thanks for doing this!
Thanks,
Namhyung