| 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: <d133fc97-e4d7-4970-933f-2762c25ae50e@intel.com>
Date: Fri, 15 Nov 2024 10:38:10 +0200
From: Adrian Hunter <adrian.hunter@...el.com>
To: Leo Yan <leo.yan@....com>, Arnaldo Carvalho de Melo <acme@...nel.org>,
Ian Rogers <irogers@...gle.com>, Namhyung Kim <namhyung@...nel.org>,
Mark Rutland <mark.rutland@....com>,
Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
Jiri Olsa <jolsa@...nel.org>, "Liang, Kan" <kan.liang@...ux.intel.com>,
James Clark <james.clark@...aro.org>, linux-perf-users@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/3] libperf cpumap: Refactor perf_cpu_map__merge()
On 7/11/24 14:53, Leo Yan wrote:
> The perf_cpu_map__merge() function has two arguments, 'orig' and
> 'other'. The function definition might cause confusion as it could give
> the impression that the CPU maps in the two arguments are copied into a
> new allocated structure, which is then returned as the result.
>
> The purpose of the function is to merge the CPU map 'other' into the CPU
> map 'orig'. This commit changes the 'orig' argument to a pointer to
> pointer, so the new result will be updated into 'orig'.
>
> The return value is changed to an int type, as an error number or 0 for
> success.
>
> Update callers and tests for the new function definition.
>
> Signed-off-by: Leo Yan <leo.yan@....com>
Reviewed-by: Adrian Hunter <adrian.hunter@...el.com>
> ---
> tools/lib/perf/cpumap.c | 49 +++++++++++++++-------------
> tools/lib/perf/evlist.c | 2 +-
> tools/lib/perf/include/perf/cpumap.h | 4 +--
> tools/perf/tests/cpumap.c | 13 ++++----
> tools/perf/util/mem-events.c | 5 ++-
> 5 files changed, 40 insertions(+), 33 deletions(-)
>
> diff --git a/tools/lib/perf/cpumap.c b/tools/lib/perf/cpumap.c
> index cae799ad44e1..a36e90d38142 100644
> --- a/tools/lib/perf/cpumap.c
> +++ b/tools/lib/perf/cpumap.c
> @@ -1,4 +1,5 @@
> // SPDX-License-Identifier: GPL-2.0-only
> +#include <errno.h>
> #include <perf/cpumap.h>
> #include <stdlib.h>
> #include <linux/refcount.h>
> @@ -436,46 +437,49 @@ bool perf_cpu_map__is_subset(const struct perf_cpu_map *a, const struct perf_cpu
> }
>
> /*
> - * Merge two cpumaps
> + * Merge two cpumaps.
> *
> - * orig either gets freed and replaced with a new map, or reused
> - * with no reference count change (similar to "realloc")
> - * other has its reference count increased.
> + * If 'other' is subset of '*orig', '*orig' keeps itself with no reference count
> + * change (similar to "realloc").
> + *
> + * If '*orig' is subset of 'other', '*orig' reuses 'other' with its reference
> + * count increased.
> + *
> + * Otherwise, '*orig' gets freed and replaced with a new map.
> */
> -
> -struct perf_cpu_map *perf_cpu_map__merge(struct perf_cpu_map *orig,
> - struct perf_cpu_map *other)
> +int perf_cpu_map__merge(struct perf_cpu_map **orig, struct perf_cpu_map *other)
> {
> struct perf_cpu *tmp_cpus;
> int tmp_len;
> int i, j, k;
> struct perf_cpu_map *merged;
>
> - if (perf_cpu_map__is_subset(orig, other))
> - return orig;
> - if (perf_cpu_map__is_subset(other, orig)) {
> - perf_cpu_map__put(orig);
> - return perf_cpu_map__get(other);
> + if (perf_cpu_map__is_subset(*orig, other))
> + return 0;
> + if (perf_cpu_map__is_subset(other, *orig)) {
> + perf_cpu_map__put(*orig);
> + *orig = perf_cpu_map__get(other);
> + return 0;
> }
>
> - tmp_len = __perf_cpu_map__nr(orig) + __perf_cpu_map__nr(other);
> + tmp_len = __perf_cpu_map__nr(*orig) + __perf_cpu_map__nr(other);
> tmp_cpus = malloc(tmp_len * sizeof(struct perf_cpu));
> if (!tmp_cpus)
> - return NULL;
> + return -ENOMEM;
>
> /* Standard merge algorithm from wikipedia */
> i = j = k = 0;
> - while (i < __perf_cpu_map__nr(orig) && j < __perf_cpu_map__nr(other)) {
> - if (__perf_cpu_map__cpu(orig, i).cpu <= __perf_cpu_map__cpu(other, j).cpu) {
> - if (__perf_cpu_map__cpu(orig, i).cpu == __perf_cpu_map__cpu(other, j).cpu)
> + while (i < __perf_cpu_map__nr(*orig) && j < __perf_cpu_map__nr(other)) {
> + if (__perf_cpu_map__cpu(*orig, i).cpu <= __perf_cpu_map__cpu(other, j).cpu) {
> + if (__perf_cpu_map__cpu(*orig, i).cpu == __perf_cpu_map__cpu(other, j).cpu)
> j++;
> - tmp_cpus[k++] = __perf_cpu_map__cpu(orig, i++);
> + tmp_cpus[k++] = __perf_cpu_map__cpu(*orig, i++);
> } else
> tmp_cpus[k++] = __perf_cpu_map__cpu(other, j++);
> }
>
> - while (i < __perf_cpu_map__nr(orig))
> - tmp_cpus[k++] = __perf_cpu_map__cpu(orig, i++);
> + while (i < __perf_cpu_map__nr(*orig))
> + tmp_cpus[k++] = __perf_cpu_map__cpu(*orig, i++);
>
> while (j < __perf_cpu_map__nr(other))
> tmp_cpus[k++] = __perf_cpu_map__cpu(other, j++);
> @@ -483,8 +487,9 @@ struct perf_cpu_map *perf_cpu_map__merge(struct perf_cpu_map *orig,
>
> merged = cpu_map__trim_new(k, tmp_cpus);
> free(tmp_cpus);
> - perf_cpu_map__put(orig);
> - return merged;
> + perf_cpu_map__put(*orig);
> + *orig = merged;
> + return 0;
> }
>
> struct perf_cpu_map *perf_cpu_map__intersect(struct perf_cpu_map *orig,
> diff --git a/tools/lib/perf/evlist.c b/tools/lib/perf/evlist.c
> index c6d67fc9e57e..94b7369f3efe 100644
> --- a/tools/lib/perf/evlist.c
> +++ b/tools/lib/perf/evlist.c
> @@ -75,7 +75,7 @@ static void __perf_evlist__propagate_maps(struct perf_evlist *evlist,
> evsel->threads = perf_thread_map__get(evlist->threads);
> }
>
> - evlist->all_cpus = perf_cpu_map__merge(evlist->all_cpus, evsel->cpus);
> + perf_cpu_map__merge(&evlist->all_cpus, evsel->cpus);
> }
>
> static void perf_evlist__propagate_maps(struct perf_evlist *evlist)
> diff --git a/tools/lib/perf/include/perf/cpumap.h b/tools/lib/perf/include/perf/cpumap.h
> index 90457d17fb2f..c83bfb2c36ff 100644
> --- a/tools/lib/perf/include/perf/cpumap.h
> +++ b/tools/lib/perf/include/perf/cpumap.h
> @@ -39,8 +39,8 @@ LIBPERF_API struct perf_cpu_map *perf_cpu_map__new_online_cpus(void);
> LIBPERF_API struct perf_cpu_map *perf_cpu_map__new(const char *cpu_list);
> LIBPERF_API struct perf_cpu_map *perf_cpu_map__read(FILE *file);
> LIBPERF_API struct perf_cpu_map *perf_cpu_map__get(struct perf_cpu_map *map);
> -LIBPERF_API struct perf_cpu_map *perf_cpu_map__merge(struct perf_cpu_map *orig,
> - struct perf_cpu_map *other);
> +LIBPERF_API int perf_cpu_map__merge(struct perf_cpu_map **orig,
> + struct perf_cpu_map *other);
> LIBPERF_API struct perf_cpu_map *perf_cpu_map__intersect(struct perf_cpu_map *orig,
> struct perf_cpu_map *other);
> LIBPERF_API void perf_cpu_map__put(struct perf_cpu_map *map);
> diff --git a/tools/perf/tests/cpumap.c b/tools/perf/tests/cpumap.c
> index 2f0168b2a5a9..7f189d57232f 100644
> --- a/tools/perf/tests/cpumap.c
> +++ b/tools/perf/tests/cpumap.c
> @@ -160,14 +160,14 @@ static int test__cpu_map_merge(struct test_suite *test __maybe_unused, int subte
> {
> struct perf_cpu_map *a = perf_cpu_map__new("4,2,1");
> struct perf_cpu_map *b = perf_cpu_map__new("4,5,7");
> - struct perf_cpu_map *c = perf_cpu_map__merge(a, b);
> char buf[100];
>
> - TEST_ASSERT_VAL("failed to merge map: bad nr", perf_cpu_map__nr(c) == 5);
> - cpu_map__snprint(c, buf, sizeof(buf));
> + perf_cpu_map__merge(&a, b);
> + TEST_ASSERT_VAL("failed to merge map: bad nr", perf_cpu_map__nr(a) == 5);
> + cpu_map__snprint(a, buf, sizeof(buf));
> TEST_ASSERT_VAL("failed to merge map: bad result", !strcmp(buf, "1-2,4-5,7"));
> perf_cpu_map__put(b);
> - perf_cpu_map__put(c);
> + perf_cpu_map__put(a);
> return 0;
> }
>
> @@ -233,9 +233,8 @@ static int test__cpu_map_equal(struct test_suite *test __maybe_unused, int subte
> }
>
> /* Maps equal made maps. */
> - tmp = perf_cpu_map__merge(perf_cpu_map__get(one), two);
> - TEST_ASSERT_VAL("pair", perf_cpu_map__equal(pair, tmp));
> - perf_cpu_map__put(tmp);
> + perf_cpu_map__merge(&two, one);
> + TEST_ASSERT_VAL("pair", perf_cpu_map__equal(pair, two));
>
> tmp = perf_cpu_map__intersect(pair, one);
> TEST_ASSERT_VAL("one", perf_cpu_map__equal(one, tmp));
> diff --git a/tools/perf/util/mem-events.c b/tools/perf/util/mem-events.c
> index bf5090f5220b..3692e988c86e 100644
> --- a/tools/perf/util/mem-events.c
> +++ b/tools/perf/util/mem-events.c
> @@ -258,6 +258,7 @@ int perf_mem_events__record_args(const char **rec_argv, int *argv_nr)
> const char *s;
> char *copy;
> struct perf_cpu_map *cpu_map = NULL;
> + int ret;
>
> while ((pmu = perf_pmus__scan_mem(pmu)) != NULL) {
> for (int j = 0; j < PERF_MEM_EVENTS__MAX; j++) {
> @@ -283,7 +284,9 @@ int perf_mem_events__record_args(const char **rec_argv, int *argv_nr)
> rec_argv[i++] = "-e";
> rec_argv[i++] = copy;
>
> - cpu_map = perf_cpu_map__merge(cpu_map, pmu->cpus);
> + ret = perf_cpu_map__merge(&cpu_map, pmu->cpus);
> + if (ret < 0)
> + return ret;
> }
> }
>
Powered by blists - more mailing lists