[PATCH v2 0/3] perf cpumap: Refactor perf_cpu_map__merge()

[PATCH v2 0/3] perf cpumap: Refactor perf_cpu_map__merge()

[Leo Yan]

perf_cpu_map__merge() 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.

This patch series refactors perf_cpu_map__merge(), makes that the first
argument 'orig' as a pointer to pointer, the merged result will be
updated into 'orig' rather than returning a pointer.  This can be clear
for the semantics that it merges 'other' into 'orig'.

The perf test has been updated for covering more cases for CPU map
merging.  Tested result is:

  # ./perf test 41
   41: CPU map                            :
   41.1: Synthesize cpu map               : Ok
   41.2: Print cpu map                    : Ok
   41.3: Merge cpu map                    : Ok
   41.4: Intersect cpu map                : Ok
   41.5: Equal cpu map                    : Ok


Leo Yan (3):
  libperf cpumap: Refactor perf_cpu_map__merge()
  perf cpumap: Add more tests for CPU map merging
  perf cpumap: Add checking for reference counter

 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            | 54 ++++++++++++++++++++++------
 tools/perf/util/mem-events.c         |  5 ++-
 5 files changed, 77 insertions(+), 37 deletions(-)

-- 
2.34.1

[PATCH v2 1/3] libperf cpumap: Refactor perf_cpu_map__merge()

[Leo Yan]

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@arm.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;
 		}
 	}
 
-- 
2.34.1

[PATCH v2 2/3] perf cpumap: Add more tests for CPU map merging

[Leo Yan]

Add additional tests for CPU map merging to cover more cases.

These tests include different types of arguments, such as when one CPU
map is a subset of another, as well as cases with or without overlap
between the two maps.

Signed-off-by: Leo Yan <leo.yan@arm.com>
---
 tools/perf/tests/cpumap.c | 34 +++++++++++++++++++++++++++++-----
 1 file changed, 29 insertions(+), 5 deletions(-)

diff --git a/tools/perf/tests/cpumap.c b/tools/perf/tests/cpumap.c
index 7f189d57232f..f8187a801b8e 100644
--- a/tools/perf/tests/cpumap.c
+++ b/tools/perf/tests/cpumap.c
@@ -156,21 +156,45 @@ static int test__cpu_map_print(struct test_suite *test __maybe_unused, int subte
 	return 0;
 }
 
-static int test__cpu_map_merge(struct test_suite *test __maybe_unused, int subtest __maybe_unused)
+static int __test__cpu_map_merge(const char *lhs, const char *rhs, int nr, const char *expected)
 {
-	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 *a = perf_cpu_map__new(lhs);
+	struct perf_cpu_map *b = perf_cpu_map__new(rhs);
 	char buf[100];
 
 	perf_cpu_map__merge(&a, b);
-	TEST_ASSERT_VAL("failed to merge map: bad nr", perf_cpu_map__nr(a) == 5);
+	TEST_ASSERT_VAL("failed to merge map: bad nr", perf_cpu_map__nr(a) == nr);
 	cpu_map__snprint(a, buf, sizeof(buf));
-	TEST_ASSERT_VAL("failed to merge map: bad result", !strcmp(buf, "1-2,4-5,7"));
+	TEST_ASSERT_VAL("failed to merge map: bad result", !strcmp(buf, expected));
 	perf_cpu_map__put(b);
 	perf_cpu_map__put(a);
 	return 0;
 }
 
+static int test__cpu_map_merge(struct test_suite *test __maybe_unused,
+			       int subtest __maybe_unused)
+{
+	int ret;
+
+	ret = __test__cpu_map_merge("4,2,1", "4,5,7", 5, "1-2,4-5,7");
+	if (ret)
+		return ret;
+	ret = __test__cpu_map_merge("1-8", "6-9", 9, "1-9");
+	if (ret)
+		return ret;
+	ret = __test__cpu_map_merge("1-8,12-20", "6-9,15", 18, "1-9,12-20");
+	if (ret)
+		return ret;
+	ret = __test__cpu_map_merge("4,2,1", "1", 3, "1-2,4");
+	if (ret)
+		return ret;
+	ret = __test__cpu_map_merge("1", "4,2,1", 3, "1-2,4");
+	if (ret)
+		return ret;
+	ret = __test__cpu_map_merge("1", "1", 1, "1");
+	return ret;
+}
+
 static int __test__cpu_map_intersect(const char *lhs, const char *rhs, int nr, const char *expected)
 {
 	struct perf_cpu_map *a = perf_cpu_map__new(lhs);
-- 
2.34.1

[PATCH v2 3/3] perf cpumap: Add checking for reference counter

[Leo Yan]

For the CPU map merging test, add an extra check for the reference
counter before releasing the last CPU map.

Signed-off-by: Leo Yan <leo.yan@arm.com>
---
 tools/perf/tests/cpumap.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/tools/perf/tests/cpumap.c b/tools/perf/tests/cpumap.c
index f8187a801b8e..5ed7ff072ea3 100644
--- a/tools/perf/tests/cpumap.c
+++ b/tools/perf/tests/cpumap.c
@@ -167,6 +167,15 @@ static int __test__cpu_map_merge(const char *lhs, const char *rhs, int nr, const
 	cpu_map__snprint(a, buf, sizeof(buf));
 	TEST_ASSERT_VAL("failed to merge map: bad result", !strcmp(buf, expected));
 	perf_cpu_map__put(b);
+
+	/*
+	 * If 'b' is a superset of 'a', 'a' points to the same map with the
+	 * map 'b'. In this case, the owner 'b' has released the resource above
+	 * but 'a' still keeps the ownership, the reference counter should be 1.
+	 */
+	TEST_ASSERT_VAL("unexpected refcnt: bad result",
+			refcount_read(perf_cpu_map__refcnt(a)) == 1);
+
 	perf_cpu_map__put(a);
 	return 0;
 }
-- 
2.34.1

Re: [PATCH v2 1/3] libperf cpumap: Refactor perf_cpu_map__merge()

[Adrian Hunter]

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@arm.com>

Reviewed-by: Adrian Hunter <adrian.hunter@intel.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;
>  		}
>  	}
>  

Re: [PATCH v2 2/3] perf cpumap: Add more tests for CPU map merging

[Adrian Hunter]

On 7/11/24 14:53, Leo Yan wrote:
> Add additional tests for CPU map merging to cover more cases.
> 
> These tests include different types of arguments, such as when one CPU
> map is a subset of another, as well as cases with or without overlap
> between the two maps.
> 
> Signed-off-by: Leo Yan <leo.yan@arm.com>

Reviewed-by: Adrian Hunter <adrian.hunter@intel.com>

> ---
>  tools/perf/tests/cpumap.c | 34 +++++++++++++++++++++++++++++-----
>  1 file changed, 29 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/perf/tests/cpumap.c b/tools/perf/tests/cpumap.c
> index 7f189d57232f..f8187a801b8e 100644
> --- a/tools/perf/tests/cpumap.c
> +++ b/tools/perf/tests/cpumap.c
> @@ -156,21 +156,45 @@ static int test__cpu_map_print(struct test_suite *test __maybe_unused, int subte
>  	return 0;
>  }
>  
> -static int test__cpu_map_merge(struct test_suite *test __maybe_unused, int subtest __maybe_unused)
> +static int __test__cpu_map_merge(const char *lhs, const char *rhs, int nr, const char *expected)
>  {
> -	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 *a = perf_cpu_map__new(lhs);
> +	struct perf_cpu_map *b = perf_cpu_map__new(rhs);
>  	char buf[100];
>  
>  	perf_cpu_map__merge(&a, b);
> -	TEST_ASSERT_VAL("failed to merge map: bad nr", perf_cpu_map__nr(a) == 5);
> +	TEST_ASSERT_VAL("failed to merge map: bad nr", perf_cpu_map__nr(a) == nr);
>  	cpu_map__snprint(a, buf, sizeof(buf));
> -	TEST_ASSERT_VAL("failed to merge map: bad result", !strcmp(buf, "1-2,4-5,7"));
> +	TEST_ASSERT_VAL("failed to merge map: bad result", !strcmp(buf, expected));
>  	perf_cpu_map__put(b);
>  	perf_cpu_map__put(a);
>  	return 0;
>  }
>  
> +static int test__cpu_map_merge(struct test_suite *test __maybe_unused,
> +			       int subtest __maybe_unused)
> +{
> +	int ret;
> +
> +	ret = __test__cpu_map_merge("4,2,1", "4,5,7", 5, "1-2,4-5,7");
> +	if (ret)
> +		return ret;
> +	ret = __test__cpu_map_merge("1-8", "6-9", 9, "1-9");
> +	if (ret)
> +		return ret;
> +	ret = __test__cpu_map_merge("1-8,12-20", "6-9,15", 18, "1-9,12-20");
> +	if (ret)
> +		return ret;
> +	ret = __test__cpu_map_merge("4,2,1", "1", 3, "1-2,4");
> +	if (ret)
> +		return ret;
> +	ret = __test__cpu_map_merge("1", "4,2,1", 3, "1-2,4");
> +	if (ret)
> +		return ret;
> +	ret = __test__cpu_map_merge("1", "1", 1, "1");
> +	return ret;
> +}
> +
>  static int __test__cpu_map_intersect(const char *lhs, const char *rhs, int nr, const char *expected)
>  {
>  	struct perf_cpu_map *a = perf_cpu_map__new(lhs);

Re: [PATCH v2 3/3] perf cpumap: Add checking for reference counter

[Adrian Hunter]

On 7/11/24 14:53, Leo Yan wrote:
> For the CPU map merging test, add an extra check for the reference
> counter before releasing the last CPU map.
> 
> Signed-off-by: Leo Yan <leo.yan@arm.com>

Reviewed-by: Adrian Hunter <adrian.hunter@intel.com>

> ---
>  tools/perf/tests/cpumap.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/tools/perf/tests/cpumap.c b/tools/perf/tests/cpumap.c
> index f8187a801b8e..5ed7ff072ea3 100644
> --- a/tools/perf/tests/cpumap.c
> +++ b/tools/perf/tests/cpumap.c
> @@ -167,6 +167,15 @@ static int __test__cpu_map_merge(const char *lhs, const char *rhs, int nr, const
>  	cpu_map__snprint(a, buf, sizeof(buf));
>  	TEST_ASSERT_VAL("failed to merge map: bad result", !strcmp(buf, expected));
>  	perf_cpu_map__put(b);
> +
> +	/*
> +	 * If 'b' is a superset of 'a', 'a' points to the same map with the
> +	 * map 'b'. In this case, the owner 'b' has released the resource above
> +	 * but 'a' still keeps the ownership, the reference counter should be 1.
> +	 */
> +	TEST_ASSERT_VAL("unexpected refcnt: bad result",
> +			refcount_read(perf_cpu_map__refcnt(a)) == 1);
> +
>  	perf_cpu_map__put(a);
>  	return 0;
>  }

Re: [PATCH v2 3/3] perf cpumap: Add checking for reference counter

[Arnaldo Carvalho de Melo]

On Fri, Nov 15, 2024 at 10:49:01AM +0200, Adrian Hunter wrote:
> On 7/11/24 14:53, Leo Yan wrote:
> > For the CPU map merging test, add an extra check for the reference
> > counter before releasing the last CPU map.
> > 
> > Signed-off-by: Leo Yan <leo.yan@arm.com>
> 
> Reviewed-by: Adrian Hunter <adrian.hunter@intel.com>

Thanks, applied the series.

- Arnaldo
 
> > ---
> >  tools/perf/tests/cpumap.c | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> > 
> > diff --git a/tools/perf/tests/cpumap.c b/tools/perf/tests/cpumap.c
> > index f8187a801b8e..5ed7ff072ea3 100644
> > --- a/tools/perf/tests/cpumap.c
> > +++ b/tools/perf/tests/cpumap.c
> > @@ -167,6 +167,15 @@ static int __test__cpu_map_merge(const char *lhs, const char *rhs, int nr, const
> >  	cpu_map__snprint(a, buf, sizeof(buf));
> >  	TEST_ASSERT_VAL("failed to merge map: bad result", !strcmp(buf, expected));
> >  	perf_cpu_map__put(b);
> > +
> > +	/*
> > +	 * If 'b' is a superset of 'a', 'a' points to the same map with the
> > +	 * map 'b'. In this case, the owner 'b' has released the resource above
> > +	 * but 'a' still keeps the ownership, the reference counter should be 1.
> > +	 */
> > +	TEST_ASSERT_VAL("unexpected refcnt: bad result",
> > +			refcount_read(perf_cpu_map__refcnt(a)) == 1);
> > +
> >  	perf_cpu_map__put(a);
> >  	return 0;
> >  }