| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <3f03541e-6dab-472f-bad9-4cdc0c0dc061@intel.com>
Date: Tue, 3 Sep 2024 18:26:54 +0300
From: Adrian Hunter <adrian.hunter@...el.com>
To: Leo Yan <leo.yan@....com>, Peter Zijlstra <peterz@...radead.org>,
Arnaldo Carvalho de Melo <acme@...nel.org>,
Namhyung Kim <namhyung@...nel.org>, Mark Rutland <mark.rutland@....com>,
Jiri Olsa <jolsa@...nel.org>, Ian Rogers <irogers@...gle.com>,
"Liang, Kan" <kan.liang@...ux.intel.com>,
Suzuki K Poulose <suzuki.poulose@....com>, Mike Leach
<mike.leach@...aro.org>, James Clark <james.clark@...aro.org>,
John Garry <john.g.garry@...cle.com>, Will Deacon <will@...nel.org>,
Yicong Yang <yangyicong@...ilicon.com>,
Jonathan Cameron <jonathan.cameron@...wei.com>,
linux-perf-users@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org, coresight@...ts.linaro.org
Subject: Re: [PATCH v6 4/8] perf auxtrace: Introduce
auxtrace_record__validate_events()
On 23/08/24 14:33, Leo Yan wrote:
> A prerequisite for multiple AUX events is that the AUX events cannot
> overlap CPU maps. The reason is that every CPU has only one AUX trace
> buffer and maps it to an unique buffer index for CPU and system tracing
> mode.
>
> To prevent the case of CPU maps overlapping occurring within multiple
> AUX events, the auxtrace_record__validate_events() function is
> introduced. It iterates through all AUX events and returns failure if
> it detects CPU maps overlapping.
>
> Signed-off-by: Leo Yan <leo.yan@....com>
> ---
> tools/perf/builtin-record.c | 4 +++
> tools/perf/util/auxtrace.c | 64 +++++++++++++++++++++++++++++++++++++
> tools/perf/util/auxtrace.h | 7 ++++
> 3 files changed, 75 insertions(+)
>
> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> index adbaf80b398c..2c618efba97d 100644
> --- a/tools/perf/builtin-record.c
> +++ b/tools/perf/builtin-record.c
> @@ -862,6 +862,10 @@ static int record__auxtrace_init(struct record *rec)
>
> auxtrace_regroup_aux_output(rec->evlist);
>
> + err = auxtrace_validate_events(rec->evlist);
> + if (err)
> + return err;
> +
> return auxtrace_parse_filters(rec->evlist);
> }
>
> diff --git a/tools/perf/util/auxtrace.c b/tools/perf/util/auxtrace.c
> index ca8682966fae..87e4f21b6edf 100644
> --- a/tools/perf/util/auxtrace.c
> +++ b/tools/perf/util/auxtrace.c
> @@ -2828,6 +2828,70 @@ int auxtrace_parse_filters(struct evlist *evlist)
> return 0;
> }
>
> +int auxtrace_validate_events(struct evlist *evlist)
'auxtrace_validate_aux_events' would better indicate that it is
looking only at AUX area events.
> +{
> + struct evsel *evsel;
> + struct perf_cpu_map *cpu_map = NULL;
> + struct perf_cpu_map *cpu_map_intersect = NULL;
> + struct perf_cpu_map *cpu_map_merged = NULL;
> + int ret = 0;
> +
> + if (!evlist)
> + return 0;
Elsewhere we assume it is not NULL, might as well here too.
> +
> + /*
> + * Currently the tool only supports multiple AUX events without
> + * overlapping CPU maps and every CPU has its unique AUX buffer
> + * for CPU or system mode tracing.
> + *
> + * Returns failure if detects CPU maps overlapping.
> + */
> + evlist__for_each_entry(evlist, evsel) {
> + if (!evsel__is_aux_event(evsel))
> + continue;
> +
> + if (perf_cpu_map__is_empty(evsel->pmu->cpus))
> + continue;
Unless perf_cpu_map__intersect() is broken, the empty check
should not be needed.
Shouldn't we be looking at evsel->cpus ?
Possibly need to consider the perf_cpu_map__has_any_cpu() case?
e.g.
if (cpu_map && (perf_cpu_map__has_any_cpu(evsel->cpus) ||
perf_cpu_map__has_any_cpu(cpu_map)) {
ret = -EINVAL;
break;
}
> +
> + cpu_map_intersect = perf_cpu_map__intersect(cpu_map, evsel->pmu->cpus);
> + if (cpu_map_intersect) {
> + perf_cpu_map__put(cpu_map_intersect);
> + pr_err("Doesn't support AUX events with overlapping CPU masks\n");
> + ret = -EINVAL;
> + break;
> + }
> + perf_cpu_map__put(cpu_map_intersect);
Maybe add a helper:
static bool perf_cpu_map__do_maps_intersect(struct perf_cpu_map *a, struct perf_cpu_map *b)
{
struct perf_cpu_map *intersection = perf_cpu_map__intersect(a, b);
bool ret = !perf_cpu_map__is_empty(intersection);
perf_cpu_map__put(intersection);
return ret;
}
> +
> + cpu_map_merged = perf_cpu_map__merge(cpu_map, evsel->pmu->cpus);
> + if (!cpu_map_merged) {
> + ret = -ENOMEM;
> + break;
> + }
> +
> + /* Update the CPU maps after merging */
> + perf_cpu_map__put(cpu_map);
> + cpu_map = cpu_map_merged;
perf_cpu_map__merge() is a bit tricky - see its comments. This
should probably all just be:
cpu_map = perf_cpu_map__merge(cpu_map, evsel->pmu->cpus);
> + }
> +
> + if (!ret)
> + goto out;
Could we put the error path last i.e.
perf_cpu_map__put(cpu_map);
if (ret)
goto out_err;
return 0;
out_err:
> +
> + /* If fails, dump CPU maps for debugging */
> + evlist__for_each_entry(evlist, evsel) {
> + char buf[200];
> +
> + if (!evsel__is_aux_event(evsel))
> + continue;
> +
> + cpu_map__snprint(evsel->pmu->cpus, buf, sizeof(buf));
> + pr_debug("AUX event [%s]'s cpu map is: %s\n", evsel->pmu->name, buf);
Could probably use cpu_map__fprintf(pmu->cpus, debug_file()) and
not need buf.
> + }
> +
> +out:
> + perf_cpu_map__put(cpu_map);
> + return ret;
> +}
> +
> int auxtrace__process_event(struct perf_session *session, union perf_event *event,
> struct perf_sample *sample, const struct perf_tool *tool)
> {
> diff --git a/tools/perf/util/auxtrace.h b/tools/perf/util/auxtrace.h
> index a1895a4f530b..67a74ad0c383 100644
> --- a/tools/perf/util/auxtrace.h
> +++ b/tools/perf/util/auxtrace.h
> @@ -636,6 +636,7 @@ void addr_filters__exit(struct addr_filters *filts);
> int addr_filters__parse_bare_filter(struct addr_filters *filts,
> const char *filter);
> int auxtrace_parse_filters(struct evlist *evlist);
> +int auxtrace_validate_events(struct evlist *evlist);
>
> int auxtrace__process_event(struct perf_session *session, union perf_event *event,
> struct perf_sample *sample, const struct perf_tool *tool);
> @@ -875,6 +876,12 @@ int auxtrace_parse_filters(struct evlist *evlist __maybe_unused)
> return 0;
> }
>
> +static inline
> +int auxtrace_validate_events(struct evlist *evlist __maybe_unused)
> +{
> + return 0;
> +}
> +
> int auxtrace_mmap__mmap(struct auxtrace_mmap *mm,
> struct auxtrace_mmap_params *mp,
> void *userpg, int fd);
Powered by blists - more mailing lists