Re: [PATCH] perf tools: Move probing cgroup sampling support

From: Arnaldo Carvalho de Melo
Date: Tue Jun 01 2021 - 09:30:36 EST


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 <namhyung@xxxxxxxxxx>
> ---
> 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