[PATCH v2 0/6] Corrections to cpu map event encoding

[PATCH v2 0/6] Corrections to cpu map event encoding

[Ian Rogers]

A mask encoding of a cpu map is laid out as:
  u16 nr
  u16 long_size
  unsigned long mask[];
However, the mask may be 8-byte aligned meaning there is a 4-byte pad
after long_size. This means 32-bit and 64-bit builds see the mask as
being at different offsets. On top of this the structure is in the byte
data[] encoded as:
  u16 type
  char data[]
This means the mask's struct isn't the required 4 or 8 byte aligned, but
is offset by 2. Consequently the long reads and writes are causing
undefined behavior as the alignment is broken.

These changes do minor clean up with const, visibility of functions
and using the constant time max function. It then adds 32 and 64-bit
mask encoding variants, packed to match current alignment. Taking the
address of a packed struct leads to unaligned data, so function
arguments are altered to be passed the packed struct. To compact the
mask encoding further and drop the padding, the 4-byte variant is
preferred. Finally a new range encoding is added, that reduces the
size of the common case of a range of CPUs to a single u64.

On a 72 CPU (hyperthread) machine the original encoding of all CPUs is:
0x9a98 [0x28]: event: 74
.
. ... raw event: size 40 bytes
.  0000:  4a 00 00 00 00 00 28 00 01 00 02 00 08 00 00 00  J.....(.........
.  0010:  00 00 ff ff ff ff ff ff ff ff ff 00 00 00 00 00  ................
.  0020:  00 00 00 00 00 00 00 00                          ........        

0 0 0x9a98 [0x28]: PERF_RECORD_CPU_MAP

Using the 4-byte encoding it is:
0x9a98@pipe [0x20]: event: 74
.
. ... raw event: size 32 bytes
.  0000:  4a 00 00 00 00 00 20 00 01 00 03 00 04 00 ff ff  J..... .........
.  0010:  ff ff ff ff ff ff ff 00 00 00 00 00 00 00 00 00  ................

0 0 0x9a98 [0x20]: PERF_RECORD_CPU_MAP

Finally, with the range encoding it is:
0x9ab8@pipe [0x10]: event: 74
.
. ... raw event: size 16 bytes
.  0000:  4a 00 00 00 00 00 10 00 02 00 00 00 00 00 47 00  J.............G.

0 0 0x9ab8 [0x10]: PERF_RECORD_CPU_MAP

v2. Fixes a bug in the size computation of the update header
    introduced by the last patch (Add range data encoding) and caught
    by address sanitizer.

Ian Rogers (6):
  perf cpumap: Const map for max
  perf cpumap: Synthetic events and const/static
  perf cpumap: Compute mask size in constant time
  perf cpumap: Fix alignment for masks in event encoding
  perf events: Prefer union over variable length array
  perf cpumap: Add range data encoding

 tools/lib/perf/cpumap.c              |   2 +-
 tools/lib/perf/include/perf/cpumap.h |   2 +-
 tools/lib/perf/include/perf/event.h  |  61 ++++++++-
 tools/perf/tests/cpumap.c            |  71 ++++++++---
 tools/perf/tests/event_update.c      |  14 +--
 tools/perf/util/cpumap.c             | 111 +++++++++++++---
 tools/perf/util/cpumap.h             |   4 +-
 tools/perf/util/event.h              |   4 -
 tools/perf/util/header.c             |  24 ++--
 tools/perf/util/session.c            |  35 +++---
 tools/perf/util/synthetic-events.c   | 182 +++++++++++++--------------
 tools/perf/util/synthetic-events.h   |   2 +-
 12 files changed, 327 insertions(+), 185 deletions(-)

-- 
2.36.1.476.g0c4daa206d-goog

[PATCH v2 1/6] perf cpumap: Const map for max

[Ian Rogers]

Allows max to be used with const perf_cpu_maps.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/lib/perf/cpumap.c              | 2 +-
 tools/lib/perf/include/perf/cpumap.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/lib/perf/cpumap.c b/tools/lib/perf/cpumap.c
index 384d5e076ee4..6cd0be7c1bb4 100644
--- a/tools/lib/perf/cpumap.c
+++ b/tools/lib/perf/cpumap.c
@@ -309,7 +309,7 @@ bool perf_cpu_map__has(const struct perf_cpu_map *cpus, struct perf_cpu cpu)
 	return perf_cpu_map__idx(cpus, cpu) != -1;
 }
 
-struct perf_cpu perf_cpu_map__max(struct perf_cpu_map *map)
+struct perf_cpu perf_cpu_map__max(const struct perf_cpu_map *map)
 {
 	struct perf_cpu result = {
 		.cpu = -1
diff --git a/tools/lib/perf/include/perf/cpumap.h b/tools/lib/perf/include/perf/cpumap.h
index 24de795b09bb..03aceb72a783 100644
--- a/tools/lib/perf/include/perf/cpumap.h
+++ b/tools/lib/perf/include/perf/cpumap.h
@@ -23,7 +23,7 @@ LIBPERF_API void perf_cpu_map__put(struct perf_cpu_map *map);
 LIBPERF_API struct perf_cpu perf_cpu_map__cpu(const struct perf_cpu_map *cpus, int idx);
 LIBPERF_API int perf_cpu_map__nr(const struct perf_cpu_map *cpus);
 LIBPERF_API bool perf_cpu_map__empty(const struct perf_cpu_map *map);
-LIBPERF_API struct perf_cpu perf_cpu_map__max(struct perf_cpu_map *map);
+LIBPERF_API struct perf_cpu perf_cpu_map__max(const struct perf_cpu_map *map);
 LIBPERF_API bool perf_cpu_map__has(const struct perf_cpu_map *map, struct perf_cpu cpu);
 
 #define perf_cpu_map__for_each_cpu(cpu, idx, cpus)		\
-- 
2.36.1.476.g0c4daa206d-goog

[PATCH v2 2/6] perf cpumap: Synthetic events and const/static

[Ian Rogers]

Make the cpumap arguments const to make it clearer they are in rather
than out arguments. Make two functions static and remove external
declarations.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/event.h            |  4 ----
 tools/perf/util/synthetic-events.c | 20 +++++++++++---------
 tools/perf/util/synthetic-events.h |  2 +-
 3 files changed, 12 insertions(+), 14 deletions(-)

diff --git a/tools/perf/util/event.h b/tools/perf/util/event.h
index cdd72e05fd28..2a69e639f6b3 100644
--- a/tools/perf/util/event.h
+++ b/tools/perf/util/event.h
@@ -461,10 +461,6 @@ size_t perf_event__fprintf(union perf_event *event, struct machine *machine, FIL
 int kallsyms__get_function_start(const char *kallsyms_filename,
 				 const char *symbol_name, u64 *addr);
 
-void *cpu_map_data__alloc(struct perf_cpu_map *map, size_t *size, u16 *type, int *max);
-void  cpu_map_data__synthesize(struct perf_record_cpu_map_data *data, struct perf_cpu_map *map,
-			       u16 type, int max);
-
 void event_attr_init(struct perf_event_attr *attr);
 
 int perf_event_paranoid(void);
diff --git a/tools/perf/util/synthetic-events.c b/tools/perf/util/synthetic-events.c
index 27acdc5e5723..b8a42a096502 100644
--- a/tools/perf/util/synthetic-events.c
+++ b/tools/perf/util/synthetic-events.c
@@ -1184,7 +1184,7 @@ int perf_event__synthesize_thread_map2(struct perf_tool *tool,
 }
 
 static void synthesize_cpus(struct cpu_map_entries *cpus,
-			    struct perf_cpu_map *map)
+			    const struct perf_cpu_map *map)
 {
 	int i, map_nr = perf_cpu_map__nr(map);
 
@@ -1195,7 +1195,7 @@ static void synthesize_cpus(struct cpu_map_entries *cpus,
 }
 
 static void synthesize_mask(struct perf_record_record_cpu_map *mask,
-			    struct perf_cpu_map *map, int max)
+			    const struct perf_cpu_map *map, int max)
 {
 	int i;
 
@@ -1206,12 +1206,12 @@ static void synthesize_mask(struct perf_record_record_cpu_map *mask,
 		set_bit(perf_cpu_map__cpu(map, i).cpu, mask->mask);
 }
 
-static size_t cpus_size(struct perf_cpu_map *map)
+static size_t cpus_size(const struct perf_cpu_map *map)
 {
 	return sizeof(struct cpu_map_entries) + perf_cpu_map__nr(map) * sizeof(u16);
 }
 
-static size_t mask_size(struct perf_cpu_map *map, int *max)
+static size_t mask_size(const struct perf_cpu_map *map, int *max)
 {
 	int i;
 
@@ -1228,7 +1228,8 @@ static size_t mask_size(struct perf_cpu_map *map, int *max)
 	return sizeof(struct perf_record_record_cpu_map) + BITS_TO_LONGS(*max) * sizeof(long);
 }
 
-void *cpu_map_data__alloc(struct perf_cpu_map *map, size_t *size, u16 *type, int *max)
+static void *cpu_map_data__alloc(const struct perf_cpu_map *map, size_t *size,
+				 u16 *type, int *max)
 {
 	size_t size_cpus, size_mask;
 	bool is_dummy = perf_cpu_map__empty(map);
@@ -1262,8 +1263,9 @@ void *cpu_map_data__alloc(struct perf_cpu_map *map, size_t *size, u16 *type, int
 	return zalloc(*size);
 }
 
-void cpu_map_data__synthesize(struct perf_record_cpu_map_data *data, struct perf_cpu_map *map,
-			      u16 type, int max)
+static void cpu_map_data__synthesize(struct perf_record_cpu_map_data *data,
+				     const struct perf_cpu_map *map,
+				     u16 type, int max)
 {
 	data->type = type;
 
@@ -1278,7 +1280,7 @@ void cpu_map_data__synthesize(struct perf_record_cpu_map_data *data, struct perf
 	}
 }
 
-static struct perf_record_cpu_map *cpu_map_event__new(struct perf_cpu_map *map)
+static struct perf_record_cpu_map *cpu_map_event__new(const struct perf_cpu_map *map)
 {
 	size_t size = sizeof(struct perf_record_cpu_map);
 	struct perf_record_cpu_map *event;
@@ -1298,7 +1300,7 @@ static struct perf_record_cpu_map *cpu_map_event__new(struct perf_cpu_map *map)
 }
 
 int perf_event__synthesize_cpu_map(struct perf_tool *tool,
-				   struct perf_cpu_map *map,
+				   const struct perf_cpu_map *map,
 				   perf_event__handler_t process,
 				   struct machine *machine)
 {
diff --git a/tools/perf/util/synthetic-events.h b/tools/perf/util/synthetic-events.h
index 78a0450db164..44839190234a 100644
--- a/tools/perf/util/synthetic-events.h
+++ b/tools/perf/util/synthetic-events.h
@@ -46,7 +46,7 @@ typedef int (*perf_event__handler_t)(struct perf_tool *tool, union perf_event *e
 int perf_event__synthesize_attrs(struct perf_tool *tool, struct evlist *evlist, perf_event__handler_t process);
 int perf_event__synthesize_attr(struct perf_tool *tool, struct perf_event_attr *attr, u32 ids, u64 *id, perf_event__handler_t process);
 int perf_event__synthesize_build_id(struct perf_tool *tool, struct dso *pos, u16 misc, perf_event__handler_t process, struct machine *machine);
-int perf_event__synthesize_cpu_map(struct perf_tool *tool, struct perf_cpu_map *cpus, perf_event__handler_t process, struct machine *machine);
+int perf_event__synthesize_cpu_map(struct perf_tool *tool, const struct perf_cpu_map *cpus, perf_event__handler_t process, struct machine *machine);
 int perf_event__synthesize_event_update_cpus(struct perf_tool *tool, struct evsel *evsel, perf_event__handler_t process);
 int perf_event__synthesize_event_update_name(struct perf_tool *tool, struct evsel *evsel, perf_event__handler_t process);
 int perf_event__synthesize_event_update_scale(struct perf_tool *tool, struct evsel *evsel, perf_event__handler_t process);
-- 
2.36.1.476.g0c4daa206d-goog

[PATCH v2 3/6] perf cpumap: Compute mask size in constant time

[Ian Rogers]

perf_cpu_map__max computes the cpumap's maximum value, no need to
iterate over all values.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/synthetic-events.c | 13 +------------
 1 file changed, 1 insertion(+), 12 deletions(-)

diff --git a/tools/perf/util/synthetic-events.c b/tools/perf/util/synthetic-events.c
index b8a42a096502..0d87df20ec44 100644
--- a/tools/perf/util/synthetic-events.c
+++ b/tools/perf/util/synthetic-events.c
@@ -1213,18 +1213,7 @@ static size_t cpus_size(const struct perf_cpu_map *map)
 
 static size_t mask_size(const struct perf_cpu_map *map, int *max)
 {
-	int i;
-
-	*max = 0;
-
-	for (i = 0; i < perf_cpu_map__nr(map); i++) {
-		/* bit position of the cpu is + 1 */
-		int bit = perf_cpu_map__cpu(map, i).cpu + 1;
-
-		if (bit > *max)
-			*max = bit;
-	}
-
+	*max = perf_cpu_map__max(map).cpu;
 	return sizeof(struct perf_record_record_cpu_map) + BITS_TO_LONGS(*max) * sizeof(long);
 }
 
-- 
2.36.1.476.g0c4daa206d-goog

[PATCH v2 4/6] perf cpumap: Fix alignment for masks in event encoding

[Ian Rogers]

A mask encoding of a cpu map is laid out as:
  u16 nr
  u16 long_size
  unsigned long mask[];
However, the mask may be 8-byte aligned meaning there is a 4-byte pad
after long_size. This means 32-bit and 64-bit builds see the mask as
being at different offsets. On top of this the structure is in the byte
data[] encoded as:
  u16 type
  char data[]
This means the mask's struct isn't the required 4 or 8 byte aligned, but
is offset by 2. Consequently the long reads and writes are causing
undefined behavior as the alignment is broken.

Fix the mask struct by creating explicit 32 and 64-bit variants, use a
union to avoid data[] and casts; the struct must be packed so the
layout matches the existing perf.data layout. Taking an address of a
member of a packed struct breaks alignment so pass the packed
perf_record_cpu_map_data to functions, so they can access variables with
the right alignment.

As the 64-bit version has 4 bytes of padding, optimizing writing to only
write the 32-bit version.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/lib/perf/include/perf/event.h | 36 +++++++++++--
 tools/perf/tests/cpumap.c           | 19 ++++---
 tools/perf/util/cpumap.c            | 80 +++++++++++++++++++++++------
 tools/perf/util/cpumap.h            |  4 +-
 tools/perf/util/session.c           | 30 +++++------
 tools/perf/util/synthetic-events.c  | 34 +++++++-----
 6 files changed, 143 insertions(+), 60 deletions(-)

diff --git a/tools/lib/perf/include/perf/event.h b/tools/lib/perf/include/perf/event.h
index e7758707cadd..d2d32589758a 100644
--- a/tools/lib/perf/include/perf/event.h
+++ b/tools/lib/perf/include/perf/event.h
@@ -6,6 +6,7 @@
 #include <linux/types.h>
 #include <linux/limits.h>
 #include <linux/bpf.h>
+#include <linux/compiler.h>
 #include <sys/types.h> /* pid_t */
 
 #define event_contains(obj, mem) ((obj).header.size > offsetof(typeof(obj), mem))
@@ -153,20 +154,47 @@ enum {
 	PERF_CPU_MAP__MASK = 1,
 };
 
+/*
+ * Array encoding of a perf_cpu_map where nr is the number of entries in cpu[]
+ * and each entry is a value for a CPU in the map.
+ */
 struct cpu_map_entries {
 	__u16			 nr;
 	__u16			 cpu[];
 };
 
-struct perf_record_record_cpu_map {
+/* Bitmap encoding of a perf_cpu_map where bitmap entries are 32-bit. */
+struct perf_record_mask_cpu_map32 {
+	/* Number of mask values. */
 	__u16			 nr;
+	/* Constant 4. */
 	__u16			 long_size;
-	unsigned long		 mask[];
+	/* Bitmap data. */
+	__u32			 mask[];
 };
 
-struct perf_record_cpu_map_data {
+/* Bitmap encoding of a perf_cpu_map where bitmap entries are 64-bit. */
+struct perf_record_mask_cpu_map64 {
+	/* Number of mask values. */
+	__u16			 nr;
+	/* Constant 8. */
+	__u16			 long_size;
+	/* Legacy padding. */
+	char                     __pad[4];
+	/* Bitmap data. */
+	__u64			 mask[];
+};
+
+struct __packed perf_record_cpu_map_data {
 	__u16			 type;
-	char			 data[];
+	union {
+		/* Used when type == PERF_CPU_MAP__CPUS. */
+		struct cpu_map_entries cpus_data;
+		/* Used when type == PERF_CPU_MAP__MASK and long_size == 4. */
+		struct perf_record_mask_cpu_map32 mask32_data;
+		/* Used when type == PERF_CPU_MAP__MASK and long_size == 8. */
+		struct perf_record_mask_cpu_map64 mask64_data;
+	};
 };
 
 struct perf_record_cpu_map {
diff --git a/tools/perf/tests/cpumap.c b/tools/perf/tests/cpumap.c
index f94929ebb54b..7ea150cdc137 100644
--- a/tools/perf/tests/cpumap.c
+++ b/tools/perf/tests/cpumap.c
@@ -17,21 +17,23 @@ static int process_event_mask(struct perf_tool *tool __maybe_unused,
 			 struct machine *machine __maybe_unused)
 {
 	struct perf_record_cpu_map *map_event = &event->cpu_map;
-	struct perf_record_record_cpu_map *mask;
 	struct perf_record_cpu_map_data *data;
 	struct perf_cpu_map *map;
 	int i;
+	unsigned int long_size;
 
 	data = &map_event->data;
 
 	TEST_ASSERT_VAL("wrong type", data->type == PERF_CPU_MAP__MASK);
 
-	mask = (struct perf_record_record_cpu_map *)data->data;
+	long_size = data->mask32_data.long_size;
 
-	TEST_ASSERT_VAL("wrong nr",   mask->nr == 1);
+	TEST_ASSERT_VAL("wrong long_size", long_size == 4 || long_size == 8);
+
+	TEST_ASSERT_VAL("wrong nr",   data->mask32_data.nr == 1);
 
 	for (i = 0; i < 20; i++) {
-		TEST_ASSERT_VAL("wrong cpu", test_bit(i, mask->mask));
+		TEST_ASSERT_VAL("wrong cpu", perf_record_cpu_map_data__test_bit(i, data));
 	}
 
 	map = cpu_map__new_data(data);
@@ -51,7 +53,6 @@ static int process_event_cpus(struct perf_tool *tool __maybe_unused,
 			 struct machine *machine __maybe_unused)
 {
 	struct perf_record_cpu_map *map_event = &event->cpu_map;
-	struct cpu_map_entries *cpus;
 	struct perf_record_cpu_map_data *data;
 	struct perf_cpu_map *map;
 
@@ -59,11 +60,9 @@ static int process_event_cpus(struct perf_tool *tool __maybe_unused,
 
 	TEST_ASSERT_VAL("wrong type", data->type == PERF_CPU_MAP__CPUS);
 
-	cpus = (struct cpu_map_entries *)data->data;
-
-	TEST_ASSERT_VAL("wrong nr",   cpus->nr == 2);
-	TEST_ASSERT_VAL("wrong cpu",  cpus->cpu[0] == 1);
-	TEST_ASSERT_VAL("wrong cpu",  cpus->cpu[1] == 256);
+	TEST_ASSERT_VAL("wrong nr",   data->cpus_data.nr == 2);
+	TEST_ASSERT_VAL("wrong cpu",  data->cpus_data.cpu[0] == 1);
+	TEST_ASSERT_VAL("wrong cpu",  data->cpus_data.cpu[1] == 256);
 
 	map = cpu_map__new_data(data);
 	TEST_ASSERT_VAL("wrong nr",  perf_cpu_map__nr(map) == 2);
diff --git a/tools/perf/util/cpumap.c b/tools/perf/util/cpumap.c
index 12b2243222b0..ae43fb88f444 100644
--- a/tools/perf/util/cpumap.c
+++ b/tools/perf/util/cpumap.c
@@ -22,54 +22,102 @@ static int max_node_num;
  */
 static int *cpunode_map;
 
-static struct perf_cpu_map *cpu_map__from_entries(struct cpu_map_entries *cpus)
+bool perf_record_cpu_map_data__test_bit(int i,
+					const struct perf_record_cpu_map_data *data)
+{
+	int bit_word32 = i / 32;
+	__u32 bit_mask32 = 1U << (i & 31);
+	int bit_word64 = i / 64;
+	__u64 bit_mask64 = ((__u64)1) << (i & 63);
+
+	return (data->mask32_data.long_size == 4)
+		? (bit_word32 < data->mask32_data.nr) &&
+		(data->mask32_data.mask[bit_word32] & bit_mask32) != 0
+		: (bit_word64 < data->mask64_data.nr) &&
+		(data->mask64_data.mask[bit_word64] & bit_mask64) != 0;
+}
+
+/* Read ith mask value from data into the given 64-bit sized bitmap */
+static void perf_record_cpu_map_data__read_one_mask(const struct perf_record_cpu_map_data *data,
+						    int i, unsigned long *bitmap)
+{
+#if __SIZEOF_LONG__ == 8
+	if (data->mask32_data.long_size == 4)
+		bitmap[0] = data->mask32_data.mask[i];
+	else
+		bitmap[0] = data->mask64_data.mask[i];
+#else
+	if (data->mask32_data.long_size == 4) {
+		bitmap[0] = data->mask32_data.mask[i];
+		bitmap[1] = 0;
+	} else {
+#if __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
+		bitmap[0] = (unsigned long)(data->mask64_data.mask[i] >> 32);
+		bitmap[1] = (unsigned long)data->mask64_data.mask[i];
+#else
+		bitmap[0] = (unsigned long)data->mask64_data.mask[i];
+		bitmap[1] = (unsigned long)(data->mask64_data.mask[i] >> 32);
+#endif
+	}
+#endif
+}
+static struct perf_cpu_map *cpu_map__from_entries(const struct perf_record_cpu_map_data *data)
 {
 	struct perf_cpu_map *map;
 
-	map = perf_cpu_map__empty_new(cpus->nr);
+	map = perf_cpu_map__empty_new(data->cpus_data.nr);
 	if (map) {
 		unsigned i;
 
-		for (i = 0; i < cpus->nr; i++) {
+		for (i = 0; i < data->cpus_data.nr; i++) {
 			/*
 			 * Special treatment for -1, which is not real cpu number,
 			 * and we need to use (int) -1 to initialize map[i],
 			 * otherwise it would become 65535.
 			 */
-			if (cpus->cpu[i] == (u16) -1)
+			if (data->cpus_data.cpu[i] == (u16) -1)
 				map->map[i].cpu = -1;
 			else
-				map->map[i].cpu = (int) cpus->cpu[i];
+				map->map[i].cpu = (int) data->cpus_data.cpu[i];
 		}
 	}
 
 	return map;
 }
 
-static struct perf_cpu_map *cpu_map__from_mask(struct perf_record_record_cpu_map *mask)
+static struct perf_cpu_map *cpu_map__from_mask(const struct perf_record_cpu_map_data *data)
 {
+	DECLARE_BITMAP(local_copy, 64);
+	int weight = 0, mask_nr = data->mask32_data.nr;
 	struct perf_cpu_map *map;
-	int nr, nbits = mask->nr * mask->long_size * BITS_PER_BYTE;
 
-	nr = bitmap_weight(mask->mask, nbits);
+	for (int i = 0; i < mask_nr; i++) {
+		perf_record_cpu_map_data__read_one_mask(data, i, local_copy);
+		weight += bitmap_weight(local_copy, 64);
+	}
+
+	map = perf_cpu_map__empty_new(weight);
+	if (!map)
+		return NULL;
 
-	map = perf_cpu_map__empty_new(nr);
-	if (map) {
-		int cpu, i = 0;
+	for (int i = 0, j = 0; i < mask_nr; i++) {
+		int cpus_per_i = (i * data->mask32_data.long_size  * BITS_PER_BYTE);
+		int cpu;
 
-		for_each_set_bit(cpu, mask->mask, nbits)
-			map->map[i++].cpu = cpu;
+		perf_record_cpu_map_data__read_one_mask(data, i, local_copy);
+		for_each_set_bit(cpu, local_copy, 64)
+			map->map[j++].cpu = cpu + cpus_per_i;
 	}
 	return map;
 
 }
 
-struct perf_cpu_map *cpu_map__new_data(struct perf_record_cpu_map_data *data)
+struct perf_cpu_map *cpu_map__new_data(const struct perf_record_cpu_map_data *data)
 {
 	if (data->type == PERF_CPU_MAP__CPUS)
-		return cpu_map__from_entries((struct cpu_map_entries *)data->data);
+		return cpu_map__from_entries(data);
 	else
-		return cpu_map__from_mask((struct perf_record_record_cpu_map *)data->data);
+		return cpu_map__from_mask(data);
 }
 
 size_t cpu_map__fprintf(struct perf_cpu_map *map, FILE *fp)
diff --git a/tools/perf/util/cpumap.h b/tools/perf/util/cpumap.h
index 703ae6d3386e..fa8a5acdcae1 100644
--- a/tools/perf/util/cpumap.h
+++ b/tools/perf/util/cpumap.h
@@ -37,9 +37,11 @@ struct cpu_aggr_map {
 
 struct perf_record_cpu_map_data;
 
+bool perf_record_cpu_map_data__test_bit(int i, const struct perf_record_cpu_map_data *data);
+
 struct perf_cpu_map *perf_cpu_map__empty_new(int nr);
 
-struct perf_cpu_map *cpu_map__new_data(struct perf_record_cpu_map_data *data);
+struct perf_cpu_map *cpu_map__new_data(const struct perf_record_cpu_map_data *data);
 size_t cpu_map__snprint(struct perf_cpu_map *map, char *buf, size_t size);
 size_t cpu_map__snprint_mask(struct perf_cpu_map *map, char *buf, size_t size);
 size_t cpu_map__fprintf(struct perf_cpu_map *map, FILE *fp);
diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
index 0aa818977d2b..d52a39ba48e3 100644
--- a/tools/perf/util/session.c
+++ b/tools/perf/util/session.c
@@ -914,30 +914,30 @@ static void perf_event__cpu_map_swap(union perf_event *event,
 				     bool sample_id_all __maybe_unused)
 {
 	struct perf_record_cpu_map_data *data = &event->cpu_map.data;
-	struct cpu_map_entries *cpus;
-	struct perf_record_record_cpu_map *mask;
-	unsigned i;
 
 	data->type = bswap_16(data->type);
 
 	switch (data->type) {
 	case PERF_CPU_MAP__CPUS:
-		cpus = (struct cpu_map_entries *)data->data;
-
-		cpus->nr = bswap_16(cpus->nr);
+		data->cpus_data.nr = bswap_16(data->cpus_data.nr);
 
-		for (i = 0; i < cpus->nr; i++)
-			cpus->cpu[i] = bswap_16(cpus->cpu[i]);
+		for (unsigned i = 0; i < data->cpus_data.nr; i++)
+			data->cpus_data.cpu[i] = bswap_16(data->cpus_data.cpu[i]);
 		break;
 	case PERF_CPU_MAP__MASK:
-		mask = (struct perf_record_record_cpu_map *)data->data;
-
-		mask->nr = bswap_16(mask->nr);
-		mask->long_size = bswap_16(mask->long_size);
+		data->mask32_data.long_size = bswap_16(data->mask32_data.long_size);
 
-		switch (mask->long_size) {
-		case 4: mem_bswap_32(&mask->mask, mask->nr); break;
-		case 8: mem_bswap_64(&mask->mask, mask->nr); break;
+		switch (data->mask32_data.long_size) {
+		case 4:
+			data->mask32_data.nr = bswap_16(data->mask32_data.nr);
+			for (unsigned i = 0; i < data->mask32_data.nr; i++)
+				data->mask32_data.mask[i] = bswap_32(data->mask32_data.mask[i]);
+			break;
+		case 8:
+			data->mask64_data.nr = bswap_16(data->mask64_data.nr);
+			for (unsigned i = 0; i < data->mask64_data.nr; i++)
+				data->mask64_data.mask[i] = bswap_64(data->mask64_data.mask[i]);
+			break;
 		default:
 			pr_err("cpu_map swap: unsupported long size\n");
 		}
diff --git a/tools/perf/util/synthetic-events.c b/tools/perf/util/synthetic-events.c
index 0d87df20ec44..4fa7d0d7dbcf 100644
--- a/tools/perf/util/synthetic-events.c
+++ b/tools/perf/util/synthetic-events.c
@@ -1183,27 +1183,33 @@ int perf_event__synthesize_thread_map2(struct perf_tool *tool,
 	return err;
 }
 
-static void synthesize_cpus(struct cpu_map_entries *cpus,
+static void synthesize_cpus(struct perf_record_cpu_map_data *data,
 			    const struct perf_cpu_map *map)
 {
 	int i, map_nr = perf_cpu_map__nr(map);
 
-	cpus->nr = map_nr;
+	data->cpus_data.nr = map_nr;
 
 	for (i = 0; i < map_nr; i++)
-		cpus->cpu[i] = perf_cpu_map__cpu(map, i).cpu;
+		data->cpus_data.cpu[i] = perf_cpu_map__cpu(map, i).cpu;
 }
 
-static void synthesize_mask(struct perf_record_record_cpu_map *mask,
+static void synthesize_mask(struct perf_record_cpu_map_data *data,
 			    const struct perf_cpu_map *map, int max)
 {
-	int i;
+	int idx;
+	struct perf_cpu cpu;
+
+	/* Due to padding, the 4bytes per entry mask variant is always smaller. */
+	data->mask32_data.nr = BITS_TO_U32(max);
+	data->mask32_data.long_size = 4;
 
-	mask->nr = BITS_TO_LONGS(max);
-	mask->long_size = sizeof(long);
+	perf_cpu_map__for_each_cpu(cpu, idx, map) {
+		int bit_word = cpu.cpu / 32;
+		__u32 bit_mask = 1U << (cpu.cpu & 31);
 
-	for (i = 0; i < perf_cpu_map__nr(map); i++)
-		set_bit(perf_cpu_map__cpu(map, i).cpu, mask->mask);
+		data->mask32_data.mask[bit_word] |= bit_mask;
+	}
 }
 
 static size_t cpus_size(const struct perf_cpu_map *map)
@@ -1214,7 +1220,7 @@ static size_t cpus_size(const struct perf_cpu_map *map)
 static size_t mask_size(const struct perf_cpu_map *map, int *max)
 {
 	*max = perf_cpu_map__max(map).cpu;
-	return sizeof(struct perf_record_record_cpu_map) + BITS_TO_LONGS(*max) * sizeof(long);
+	return sizeof(struct perf_record_mask_cpu_map32) + BITS_TO_U32(*max) * sizeof(__u32);
 }
 
 static void *cpu_map_data__alloc(const struct perf_cpu_map *map, size_t *size,
@@ -1247,7 +1253,7 @@ static void *cpu_map_data__alloc(const struct perf_cpu_map *map, size_t *size,
 		*type  = PERF_CPU_MAP__MASK;
 	}
 
-	*size += sizeof(struct perf_record_cpu_map_data);
+	*size += sizeof(__u16); /* For perf_record_cpu_map_data.type. */
 	*size = PERF_ALIGN(*size, sizeof(u64));
 	return zalloc(*size);
 }
@@ -1260,10 +1266,10 @@ static void cpu_map_data__synthesize(struct perf_record_cpu_map_data *data,
 
 	switch (type) {
 	case PERF_CPU_MAP__CPUS:
-		synthesize_cpus((struct cpu_map_entries *) data->data, map);
+		synthesize_cpus(data, map);
 		break;
 	case PERF_CPU_MAP__MASK:
-		synthesize_mask((struct perf_record_record_cpu_map *)data->data, map, max);
+		synthesize_mask(data, map, max);
 	default:
 		break;
 	}
@@ -1271,7 +1277,7 @@ static void cpu_map_data__synthesize(struct perf_record_cpu_map_data *data,
 
 static struct perf_record_cpu_map *cpu_map_event__new(const struct perf_cpu_map *map)
 {
-	size_t size = sizeof(struct perf_record_cpu_map);
+	size_t size = sizeof(struct perf_event_header);
 	struct perf_record_cpu_map *event;
 	int max;
 	u16 type;
-- 
2.36.1.476.g0c4daa206d-goog

Re: [PATCH v2 4/6] perf cpumap: Fix alignment for masks in event encoding

[Namhyung Kim]

Hi Ian,

On Tue, Jun 14, 2022 at 7:34 AM Ian Rogers <irogers@google.com> wrote:
>
> A mask encoding of a cpu map is laid out as:
>   u16 nr
>   u16 long_size
>   unsigned long mask[];
> However, the mask may be 8-byte aligned meaning there is a 4-byte pad
> after long_size. This means 32-bit and 64-bit builds see the mask as
> being at different offsets. On top of this the structure is in the byte
> data[] encoded as:
>   u16 type
>   char data[]
> This means the mask's struct isn't the required 4 or 8 byte aligned, but
> is offset by 2. Consequently the long reads and writes are causing
> undefined behavior as the alignment is broken.
>
> Fix the mask struct by creating explicit 32 and 64-bit variants, use a
> union to avoid data[] and casts; the struct must be packed so the
> layout matches the existing perf.data layout. Taking an address of a
> member of a packed struct breaks alignment so pass the packed
> perf_record_cpu_map_data to functions, so they can access variables with
> the right alignment.
>
> As the 64-bit version has 4 bytes of padding, optimizing writing to only
> write the 32-bit version.
>
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/lib/perf/include/perf/event.h | 36 +++++++++++--
>  tools/perf/tests/cpumap.c           | 19 ++++---
>  tools/perf/util/cpumap.c            | 80 +++++++++++++++++++++++------
>  tools/perf/util/cpumap.h            |  4 +-
>  tools/perf/util/session.c           | 30 +++++------
>  tools/perf/util/synthetic-events.c  | 34 +++++++-----
>  6 files changed, 143 insertions(+), 60 deletions(-)
>
> diff --git a/tools/lib/perf/include/perf/event.h b/tools/lib/perf/include/perf/event.h
> index e7758707cadd..d2d32589758a 100644
> --- a/tools/lib/perf/include/perf/event.h
> +++ b/tools/lib/perf/include/perf/event.h
> @@ -6,6 +6,7 @@
>  #include <linux/types.h>
>  #include <linux/limits.h>
>  #include <linux/bpf.h>
> +#include <linux/compiler.h>
>  #include <sys/types.h> /* pid_t */
>
>  #define event_contains(obj, mem) ((obj).header.size > offsetof(typeof(obj), mem))
> @@ -153,20 +154,47 @@ enum {
>         PERF_CPU_MAP__MASK = 1,
>  };
>
> +/*
> + * Array encoding of a perf_cpu_map where nr is the number of entries in cpu[]
> + * and each entry is a value for a CPU in the map.
> + */
>  struct cpu_map_entries {
>         __u16                    nr;
>         __u16                    cpu[];
>  };
>
> -struct perf_record_record_cpu_map {
> +/* Bitmap encoding of a perf_cpu_map where bitmap entries are 32-bit. */
> +struct perf_record_mask_cpu_map32 {
> +       /* Number of mask values. */
>         __u16                    nr;
> +       /* Constant 4. */
>         __u16                    long_size;
> -       unsigned long            mask[];
> +       /* Bitmap data. */
> +       __u32                    mask[];
>  };
>
> -struct perf_record_cpu_map_data {
> +/* Bitmap encoding of a perf_cpu_map where bitmap entries are 64-bit. */
> +struct perf_record_mask_cpu_map64 {
> +       /* Number of mask values. */
> +       __u16                    nr;
> +       /* Constant 8. */
> +       __u16                    long_size;
> +       /* Legacy padding. */
> +       char                     __pad[4];
> +       /* Bitmap data. */
> +       __u64                    mask[];
> +};
> +
> +struct __packed perf_record_cpu_map_data {
>         __u16                    type;
> -       char                     data[];
> +       union {
> +               /* Used when type == PERF_CPU_MAP__CPUS. */
> +               struct cpu_map_entries cpus_data;
> +               /* Used when type == PERF_CPU_MAP__MASK and long_size == 4. */
> +               struct perf_record_mask_cpu_map32 mask32_data;
> +               /* Used when type == PERF_CPU_MAP__MASK and long_size == 8. */
> +               struct perf_record_mask_cpu_map64 mask64_data;
> +       };
>  };

How about moving the 'type' to the union as well?
This way we don't need to pack the entire struct
and can have a common struct for 32 and 64 bit..

struct cpu_map_entries {
    __u16 type;
    __u16 nr;
    __u16 cpu[];
};

struct perf_record_mask_cpu_map {
    __u16 type;
    __u16 nr;
    __u16 long_size;  // still needed?
    __u16 pad;
    unsigned long mask[];
};

// changed it to union
union perf_record_cpu_map_data {
    __u16  type;
    struct cpu_map_entries cpus_data;
    struct perf_record_mask_cpu_map mask_data;
};

Thanks,
Namhyung

Re: [PATCH v2 4/6] perf cpumap: Fix alignment for masks in event encoding

[Ian Rogers]

On Tue, Jun 14, 2022 at 3:44 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> Hi Ian,
>
> On Tue, Jun 14, 2022 at 7:34 AM Ian Rogers <irogers@google.com> wrote:
> >
> > A mask encoding of a cpu map is laid out as:
> >   u16 nr
> >   u16 long_size
> >   unsigned long mask[];
> > However, the mask may be 8-byte aligned meaning there is a 4-byte pad
> > after long_size. This means 32-bit and 64-bit builds see the mask as
> > being at different offsets. On top of this the structure is in the byte
> > data[] encoded as:
> >   u16 type
> >   char data[]
> > This means the mask's struct isn't the required 4 or 8 byte aligned, but
> > is offset by 2. Consequently the long reads and writes are causing
> > undefined behavior as the alignment is broken.
> >
> > Fix the mask struct by creating explicit 32 and 64-bit variants, use a
> > union to avoid data[] and casts; the struct must be packed so the
> > layout matches the existing perf.data layout. Taking an address of a
> > member of a packed struct breaks alignment so pass the packed
> > perf_record_cpu_map_data to functions, so they can access variables with
> > the right alignment.
> >
> > As the 64-bit version has 4 bytes of padding, optimizing writing to only
> > write the 32-bit version.
> >
> > Signed-off-by: Ian Rogers <irogers@google.com>
> > ---
> >  tools/lib/perf/include/perf/event.h | 36 +++++++++++--
> >  tools/perf/tests/cpumap.c           | 19 ++++---
> >  tools/perf/util/cpumap.c            | 80 +++++++++++++++++++++++------
> >  tools/perf/util/cpumap.h            |  4 +-
> >  tools/perf/util/session.c           | 30 +++++------
> >  tools/perf/util/synthetic-events.c  | 34 +++++++-----
> >  6 files changed, 143 insertions(+), 60 deletions(-)
> >
> > diff --git a/tools/lib/perf/include/perf/event.h b/tools/lib/perf/include/perf/event.h
> > index e7758707cadd..d2d32589758a 100644
> > --- a/tools/lib/perf/include/perf/event.h
> > +++ b/tools/lib/perf/include/perf/event.h
> > @@ -6,6 +6,7 @@
> >  #include <linux/types.h>
> >  #include <linux/limits.h>
> >  #include <linux/bpf.h>
> > +#include <linux/compiler.h>
> >  #include <sys/types.h> /* pid_t */
> >
> >  #define event_contains(obj, mem) ((obj).header.size > offsetof(typeof(obj), mem))
> > @@ -153,20 +154,47 @@ enum {
> >         PERF_CPU_MAP__MASK = 1,
> >  };
> >
> > +/*
> > + * Array encoding of a perf_cpu_map where nr is the number of entries in cpu[]
> > + * and each entry is a value for a CPU in the map.
> > + */
> >  struct cpu_map_entries {
> >         __u16                    nr;
> >         __u16                    cpu[];
> >  };
> >
> > -struct perf_record_record_cpu_map {
> > +/* Bitmap encoding of a perf_cpu_map where bitmap entries are 32-bit. */
> > +struct perf_record_mask_cpu_map32 {
> > +       /* Number of mask values. */
> >         __u16                    nr;
> > +       /* Constant 4. */
> >         __u16                    long_size;
> > -       unsigned long            mask[];
> > +       /* Bitmap data. */
> > +       __u32                    mask[];
> >  };
> >
> > -struct perf_record_cpu_map_data {
> > +/* Bitmap encoding of a perf_cpu_map where bitmap entries are 64-bit. */
> > +struct perf_record_mask_cpu_map64 {
> > +       /* Number of mask values. */
> > +       __u16                    nr;
> > +       /* Constant 8. */
> > +       __u16                    long_size;
> > +       /* Legacy padding. */
> > +       char                     __pad[4];
> > +       /* Bitmap data. */
> > +       __u64                    mask[];
> > +};
> > +
> > +struct __packed perf_record_cpu_map_data {
> >         __u16                    type;
> > -       char                     data[];
> > +       union {
> > +               /* Used when type == PERF_CPU_MAP__CPUS. */
> > +               struct cpu_map_entries cpus_data;
> > +               /* Used when type == PERF_CPU_MAP__MASK and long_size == 4. */
> > +               struct perf_record_mask_cpu_map32 mask32_data;
> > +               /* Used when type == PERF_CPU_MAP__MASK and long_size == 8. */
> > +               struct perf_record_mask_cpu_map64 mask64_data;
> > +       };
> >  };
>
> How about moving the 'type' to the union as well?
> This way we don't need to pack the entire struct
> and can have a common struct for 32 and 64 bit..
>
> struct cpu_map_entries {
>     __u16 type;
>     __u16 nr;
>     __u16 cpu[];
> };
>
> struct perf_record_mask_cpu_map {
>     __u16 type;
>     __u16 nr;
>     __u16 long_size;  // still needed?
>     __u16 pad;
>     unsigned long mask[];
> };
>
> // changed it to union
> union perf_record_cpu_map_data {
>     __u16  type;
>     struct cpu_map_entries cpus_data;
>     struct perf_record_mask_cpu_map mask_data;
> };



Thanks Namhyung,

Unfortunately this doesn't quite work as I want to make it so that the
existing cpu map encodings work with this change - ie, an old
perf.data should be readable in a newer perf with this change (the
range encoding will require that new perf.data files are read by
versions of perf with these changes). For this to work I need the
layout to match the existing unaligned code, I either need to make
mask bytes and memcpy or use an attribute like packed. Fwiw, this is a
little more efficient than the layout above as with long_size == 4 the
pad isn't necessary saving 2 bytes. I think with the packed approach
we can also add new unpacked variants like above, although I'd be keen
not to use a type that varies in size like long. I guess at some
future date we could remove the legacy supporting packed versions so
that packed or byte copying is unnecessary.

I could use a union as you show above, unfortunately that will need
the 'struct perf_record_mask_cpu_map32' and 'struct
perf_record_mask_cpu_map64' to be packed or to use bytes. We'd lose
one use of packed just to introduce two others. Potentially it is more
of a breaking change for users of this code via libperf.

These changes are something of a bug report along with fixes. If there
is a consensus that the right way to fix the bug is to break legacy
perf.data files then I'm happy to update the code accordingly (as you
show above).

Thanks,
Ian

> Thanks,
> Namhyung

Re: [PATCH v2 4/6] perf cpumap: Fix alignment for masks in event encoding

[Jiri Olsa]

On Tue, Jun 14, 2022 at 07:33:51AM -0700, Ian Rogers wrote:
> A mask encoding of a cpu map is laid out as:
>   u16 nr
>   u16 long_size
>   unsigned long mask[];
> However, the mask may be 8-byte aligned meaning there is a 4-byte pad
> after long_size. This means 32-bit and 64-bit builds see the mask as
> being at different offsets. On top of this the structure is in the byte
> data[] encoded as:
>   u16 type
>   char data[]
> This means the mask's struct isn't the required 4 or 8 byte aligned, but
> is offset by 2. Consequently the long reads and writes are causing
> undefined behavior as the alignment is broken.
> 
> Fix the mask struct by creating explicit 32 and 64-bit variants, use a
> union to avoid data[] and casts; the struct must be packed so the
> layout matches the existing perf.data layout. Taking an address of a
> member of a packed struct breaks alignment so pass the packed
> perf_record_cpu_map_data to functions, so they can access variables with
> the right alignment.
> 
> As the 64-bit version has 4 bytes of padding, optimizing writing to only
> write the 32-bit version.
> 
> Signed-off-by: Ian Rogers <irogers@google.com>

SNIP

>  struct perf_record_cpu_map {
> diff --git a/tools/perf/tests/cpumap.c b/tools/perf/tests/cpumap.c
> index f94929ebb54b..7ea150cdc137 100644
> --- a/tools/perf/tests/cpumap.c
> +++ b/tools/perf/tests/cpumap.c
> @@ -17,21 +17,23 @@ static int process_event_mask(struct perf_tool *tool __maybe_unused,
>  			 struct machine *machine __maybe_unused)
>  {
>  	struct perf_record_cpu_map *map_event = &event->cpu_map;
> -	struct perf_record_record_cpu_map *mask;
>  	struct perf_record_cpu_map_data *data;
>  	struct perf_cpu_map *map;
>  	int i;
> +	unsigned int long_size;
>  
>  	data = &map_event->data;
>  
>  	TEST_ASSERT_VAL("wrong type", data->type == PERF_CPU_MAP__MASK);
>  
> -	mask = (struct perf_record_record_cpu_map *)data->data;
> +	long_size = data->mask32_data.long_size;
>  
> -	TEST_ASSERT_VAL("wrong nr",   mask->nr == 1);
> +	TEST_ASSERT_VAL("wrong long_size", long_size == 4 || long_size == 8);

should we check here just for long_size == 4 ?

SNIP

> diff --git a/tools/perf/util/cpumap.c b/tools/perf/util/cpumap.c
> index 12b2243222b0..ae43fb88f444 100644
> --- a/tools/perf/util/cpumap.c
> +++ b/tools/perf/util/cpumap.c
> @@ -22,54 +22,102 @@ static int max_node_num;
>   */
>  static int *cpunode_map;
>  
> -static struct perf_cpu_map *cpu_map__from_entries(struct cpu_map_entries *cpus)
> +bool perf_record_cpu_map_data__test_bit(int i,
> +					const struct perf_record_cpu_map_data *data)
> +{
> +	int bit_word32 = i / 32;
> +	__u32 bit_mask32 = 1U << (i & 31);
> +	int bit_word64 = i / 64;
> +	__u64 bit_mask64 = ((__u64)1) << (i & 63);
> +
> +	return (data->mask32_data.long_size == 4)
> +		? (bit_word32 < data->mask32_data.nr) &&
> +		(data->mask32_data.mask[bit_word32] & bit_mask32) != 0
> +		: (bit_word64 < data->mask64_data.nr) &&
> +		(data->mask64_data.mask[bit_word64] & bit_mask64) != 0;
> +}
> +
> +/* Read ith mask value from data into the given 64-bit sized bitmap */
> +static void perf_record_cpu_map_data__read_one_mask(const struct perf_record_cpu_map_data *data,
> +						    int i, unsigned long *bitmap)
> +{
> +#if __SIZEOF_LONG__ == 8
> +	if (data->mask32_data.long_size == 4)
> +		bitmap[0] = data->mask32_data.mask[i];
> +	else
> +		bitmap[0] = data->mask64_data.mask[i];
> +#else
> +	if (data->mask32_data.long_size == 4) {
> +		bitmap[0] = data->mask32_data.mask[i];
> +		bitmap[1] = 0;
> +	} else {
> +#if __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
> +		bitmap[0] = (unsigned long)(data->mask64_data.mask[i] >> 32);
> +		bitmap[1] = (unsigned long)data->mask64_data.mask[i];
> +#else
> +		bitmap[0] = (unsigned long)data->mask64_data.mask[i];
> +		bitmap[1] = (unsigned long)(data->mask64_data.mask[i] >> 32);
> +#endif

should this be taken care of earlier by perf_event__cpu_map_swap ?


> +	}
> +#endif
> +}
> +static struct perf_cpu_map *cpu_map__from_entries(const struct perf_record_cpu_map_data *data)
>  {
>  	struct perf_cpu_map *map;
>  
> -	map = perf_cpu_map__empty_new(cpus->nr);
> +	map = perf_cpu_map__empty_new(data->cpus_data.nr);
>  	if (map) {
>  		unsigned i;
>  
> -		for (i = 0; i < cpus->nr; i++) {
> +		for (i = 0; i < data->cpus_data.nr; i++) {
>  			/*
>  			 * Special treatment for -1, which is not real cpu number,
>  			 * and we need to use (int) -1 to initialize map[i],
>  			 * otherwise it would become 65535.
>  			 */
> -			if (cpus->cpu[i] == (u16) -1)
> +			if (data->cpus_data.cpu[i] == (u16) -1)
>  				map->map[i].cpu = -1;
>  			else
> -				map->map[i].cpu = (int) cpus->cpu[i];
> +				map->map[i].cpu = (int) data->cpus_data.cpu[i];
>  		}
>  	}
>  
>  	return map;
>  }
>  
> -static struct perf_cpu_map *cpu_map__from_mask(struct perf_record_record_cpu_map *mask)
> +static struct perf_cpu_map *cpu_map__from_mask(const struct perf_record_cpu_map_data *data)
>  {
> +	DECLARE_BITMAP(local_copy, 64);
> +	int weight = 0, mask_nr = data->mask32_data.nr;
>  	struct perf_cpu_map *map;
> -	int nr, nbits = mask->nr * mask->long_size * BITS_PER_BYTE;
>  
> -	nr = bitmap_weight(mask->mask, nbits);
> +	for (int i = 0; i < mask_nr; i++) {
> +		perf_record_cpu_map_data__read_one_mask(data, i, local_copy);
> +		weight += bitmap_weight(local_copy, 64);
> +	}
> +
> +	map = perf_cpu_map__empty_new(weight);
> +	if (!map)
> +		return NULL;
>  
> -	map = perf_cpu_map__empty_new(nr);
> -	if (map) {
> -		int cpu, i = 0;
> +	for (int i = 0, j = 0; i < mask_nr; i++) {
> +		int cpus_per_i = (i * data->mask32_data.long_size  * BITS_PER_BYTE);
> +		int cpu;
>  
> -		for_each_set_bit(cpu, mask->mask, nbits)
> -			map->map[i++].cpu = cpu;
> +		perf_record_cpu_map_data__read_one_mask(data, i, local_copy);
> +		for_each_set_bit(cpu, local_copy, 64)
> +			map->map[j++].cpu = cpu + cpus_per_i;
>  	}
>  	return map;
>  
>  }
>  
> -struct perf_cpu_map *cpu_map__new_data(struct perf_record_cpu_map_data *data)
> +struct perf_cpu_map *cpu_map__new_data(const struct perf_record_cpu_map_data *data)
>  {
>  	if (data->type == PERF_CPU_MAP__CPUS)
> -		return cpu_map__from_entries((struct cpu_map_entries *)data->data);
> +		return cpu_map__from_entries(data);
>  	else
> -		return cpu_map__from_mask((struct perf_record_record_cpu_map *)data->data);
> +		return cpu_map__from_mask(data);
>  }
>  
>  size_t cpu_map__fprintf(struct perf_cpu_map *map, FILE *fp)
> diff --git a/tools/perf/util/cpumap.h b/tools/perf/util/cpumap.h
> index 703ae6d3386e..fa8a5acdcae1 100644
> --- a/tools/perf/util/cpumap.h
> +++ b/tools/perf/util/cpumap.h
> @@ -37,9 +37,11 @@ struct cpu_aggr_map {
>  
>  struct perf_record_cpu_map_data;
>  
> +bool perf_record_cpu_map_data__test_bit(int i, const struct perf_record_cpu_map_data *data);
> +
>  struct perf_cpu_map *perf_cpu_map__empty_new(int nr);
>  
> -struct perf_cpu_map *cpu_map__new_data(struct perf_record_cpu_map_data *data);
> +struct perf_cpu_map *cpu_map__new_data(const struct perf_record_cpu_map_data *data);
>  size_t cpu_map__snprint(struct perf_cpu_map *map, char *buf, size_t size);
>  size_t cpu_map__snprint_mask(struct perf_cpu_map *map, char *buf, size_t size);
>  size_t cpu_map__fprintf(struct perf_cpu_map *map, FILE *fp);
> diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
> index 0aa818977d2b..d52a39ba48e3 100644
> --- a/tools/perf/util/session.c
> +++ b/tools/perf/util/session.c
> @@ -914,30 +914,30 @@ static void perf_event__cpu_map_swap(union perf_event *event,
>  				     bool sample_id_all __maybe_unused)
>  {
>  	struct perf_record_cpu_map_data *data = &event->cpu_map.data;
> -	struct cpu_map_entries *cpus;
> -	struct perf_record_record_cpu_map *mask;
> -	unsigned i;
>  
>  	data->type = bswap_16(data->type);
>  
>  	switch (data->type) {
>  	case PERF_CPU_MAP__CPUS:
> -		cpus = (struct cpu_map_entries *)data->data;
> -
> -		cpus->nr = bswap_16(cpus->nr);
> +		data->cpus_data.nr = bswap_16(data->cpus_data.nr);
>  
> -		for (i = 0; i < cpus->nr; i++)
> -			cpus->cpu[i] = bswap_16(cpus->cpu[i]);
> +		for (unsigned i = 0; i < data->cpus_data.nr; i++)
> +			data->cpus_data.cpu[i] = bswap_16(data->cpus_data.cpu[i]);
>  		break;
>  	case PERF_CPU_MAP__MASK:
> -		mask = (struct perf_record_record_cpu_map *)data->data;
> -
> -		mask->nr = bswap_16(mask->nr);
> -		mask->long_size = bswap_16(mask->long_size);
> +		data->mask32_data.long_size = bswap_16(data->mask32_data.long_size);
>  
> -		switch (mask->long_size) {
> -		case 4: mem_bswap_32(&mask->mask, mask->nr); break;
> -		case 8: mem_bswap_64(&mask->mask, mask->nr); break;
> +		switch (data->mask32_data.long_size) {
> +		case 4:
> +			data->mask32_data.nr = bswap_16(data->mask32_data.nr);
> +			for (unsigned i = 0; i < data->mask32_data.nr; i++)
> +				data->mask32_data.mask[i] = bswap_32(data->mask32_data.mask[i]);
> +			break;

why not use the mem_bswap_* functions?

looks like we never swap it completely, because we passed
mask->nr where should be the size

> +		case 8:
> +			data->mask64_data.nr = bswap_16(data->mask64_data.nr);
> +			for (unsigned i = 0; i < data->mask64_data.nr; i++)
> +				data->mask64_data.mask[i] = bswap_64(data->mask64_data.mask[i]);
> +			break;
>  		default:
>  			pr_err("cpu_map swap: unsupported long size\n");
>  		}
> diff --git a/tools/perf/util/synthetic-events.c b/tools/perf/util/synthetic-events.c
> index 0d87df20ec44..4fa7d0d7dbcf 100644
> --- a/tools/perf/util/synthetic-events.c
> +++ b/tools/perf/util/synthetic-events.c
> @@ -1183,27 +1183,33 @@ int perf_event__synthesize_thread_map2(struct perf_tool *tool,
>  	return err;
>  }
>  
> -static void synthesize_cpus(struct cpu_map_entries *cpus,
> +static void synthesize_cpus(struct perf_record_cpu_map_data *data,
>  			    const struct perf_cpu_map *map)
>  {
>  	int i, map_nr = perf_cpu_map__nr(map);
>  
> -	cpus->nr = map_nr;
> +	data->cpus_data.nr = map_nr;
>  
>  	for (i = 0; i < map_nr; i++)
> -		cpus->cpu[i] = perf_cpu_map__cpu(map, i).cpu;
> +		data->cpus_data.cpu[i] = perf_cpu_map__cpu(map, i).cpu;
>  }
>  
> -static void synthesize_mask(struct perf_record_record_cpu_map *mask,
> +static void synthesize_mask(struct perf_record_cpu_map_data *data,
>  			    const struct perf_cpu_map *map, int max)
>  {
> -	int i;
> +	int idx;
> +	struct perf_cpu cpu;
> +
> +	/* Due to padding, the 4bytes per entry mask variant is always smaller. */
> +	data->mask32_data.nr = BITS_TO_U32(max);
> +	data->mask32_data.long_size = 4;

ok, so we go always with 32 bit version

>  
> -	mask->nr = BITS_TO_LONGS(max);
> -	mask->long_size = sizeof(long);
> +	perf_cpu_map__for_each_cpu(cpu, idx, map) {
> +		int bit_word = cpu.cpu / 32;
> +		__u32 bit_mask = 1U << (cpu.cpu & 31);

set_bit uses (nr % 32), but I guess it does not matter


jirka

>  
> -	for (i = 0; i < perf_cpu_map__nr(map); i++)
> -		set_bit(perf_cpu_map__cpu(map, i).cpu, mask->mask);
> +		data->mask32_data.mask[bit_word] |= bit_mask;
> +	}
>  }
>  
>  static size_t cpus_size(const struct perf_cpu_map *map)
> @@ -1214,7 +1220,7 @@ static size_t cpus_size(const struct perf_cpu_map *map)
>  static size_t mask_size(const struct perf_cpu_map *map, int *max)
>  {
>  	*max = perf_cpu_map__max(map).cpu;
> -	return sizeof(struct perf_record_record_cpu_map) + BITS_TO_LONGS(*max) * sizeof(long);
> +	return sizeof(struct perf_record_mask_cpu_map32) + BITS_TO_U32(*max) * sizeof(__u32);
>  }
>  
>  static void *cpu_map_data__alloc(const struct perf_cpu_map *map, size_t *size,
> @@ -1247,7 +1253,7 @@ static void *cpu_map_data__alloc(const struct perf_cpu_map *map, size_t *size,
>  		*type  = PERF_CPU_MAP__MASK;
>  	}
>  
> -	*size += sizeof(struct perf_record_cpu_map_data);
> +	*size += sizeof(__u16); /* For perf_record_cpu_map_data.type. */
>  	*size = PERF_ALIGN(*size, sizeof(u64));
>  	return zalloc(*size);
>  }
> @@ -1260,10 +1266,10 @@ static void cpu_map_data__synthesize(struct perf_record_cpu_map_data *data,
>  
>  	switch (type) {
>  	case PERF_CPU_MAP__CPUS:
> -		synthesize_cpus((struct cpu_map_entries *) data->data, map);
> +		synthesize_cpus(data, map);
>  		break;
>  	case PERF_CPU_MAP__MASK:
> -		synthesize_mask((struct perf_record_record_cpu_map *)data->data, map, max);
> +		synthesize_mask(data, map, max);
>  	default:
>  		break;
>  	}
> @@ -1271,7 +1277,7 @@ static void cpu_map_data__synthesize(struct perf_record_cpu_map_data *data,
>  
>  static struct perf_record_cpu_map *cpu_map_event__new(const struct perf_cpu_map *map)
>  {
> -	size_t size = sizeof(struct perf_record_cpu_map);
> +	size_t size = sizeof(struct perf_event_header);
>  	struct perf_record_cpu_map *event;
>  	int max;
>  	u16 type;
> -- 
> 2.36.1.476.g0c4daa206d-goog
> 

Re: [PATCH v2 4/6] perf cpumap: Fix alignment for masks in event encoding

[Ian Rogers]

 asOn Wed, Jun 29, 2022 at 2:18 AM Jiri Olsa <olsajiri@gmail.com> wrote:
>
> On Tue, Jun 14, 2022 at 07:33:51AM -0700, Ian Rogers wrote:
> > A mask encoding of a cpu map is laid out as:
> >   u16 nr
> >   u16 long_size
> >   unsigned long mask[];
> > However, the mask may be 8-byte aligned meaning there is a 4-byte pad
> > after long_size. This means 32-bit and 64-bit builds see the mask as
> > being at different offsets. On top of this the structure is in the byte
> > data[] encoded as:
> >   u16 type
> >   char data[]
> > This means the mask's struct isn't the required 4 or 8 byte aligned, but
> > is offset by 2. Consequently the long reads and writes are causing
> > undefined behavior as the alignment is broken.
> >
> > Fix the mask struct by creating explicit 32 and 64-bit variants, use a
> > union to avoid data[] and casts; the struct must be packed so the
> > layout matches the existing perf.data layout. Taking an address of a
> > member of a packed struct breaks alignment so pass the packed
> > perf_record_cpu_map_data to functions, so they can access variables with
> > the right alignment.
> >
> > As the 64-bit version has 4 bytes of padding, optimizing writing to only
> > write the 32-bit version.
> >
> > Signed-off-by: Ian Rogers <irogers@google.com>
>
> SNIP
>
> >  struct perf_record_cpu_map {
> > diff --git a/tools/perf/tests/cpumap.c b/tools/perf/tests/cpumap.c
> > index f94929ebb54b..7ea150cdc137 100644
> > --- a/tools/perf/tests/cpumap.c
> > +++ b/tools/perf/tests/cpumap.c
> > @@ -17,21 +17,23 @@ static int process_event_mask(struct perf_tool *tool __maybe_unused,
> >                        struct machine *machine __maybe_unused)
> >  {
> >       struct perf_record_cpu_map *map_event = &event->cpu_map;
> > -     struct perf_record_record_cpu_map *mask;
> >       struct perf_record_cpu_map_data *data;
> >       struct perf_cpu_map *map;
> >       int i;
> > +     unsigned int long_size;
> >
> >       data = &map_event->data;
> >
> >       TEST_ASSERT_VAL("wrong type", data->type == PERF_CPU_MAP__MASK);
> >
> > -     mask = (struct perf_record_record_cpu_map *)data->data;
> > +     long_size = data->mask32_data.long_size;
> >
> > -     TEST_ASSERT_VAL("wrong nr",   mask->nr == 1);
> > +     TEST_ASSERT_VAL("wrong long_size", long_size == 4 || long_size == 8);
>
> should we check here just for long_size == 4 ?

We could given that we expect 4 byte versions only to be generated
after this change. Unit tests act like documentation, so I am hesitant
to remove the == 8 given it is a valid option.

> SNIP
>
> > diff --git a/tools/perf/util/cpumap.c b/tools/perf/util/cpumap.c
> > index 12b2243222b0..ae43fb88f444 100644
> > --- a/tools/perf/util/cpumap.c
> > +++ b/tools/perf/util/cpumap.c
> > @@ -22,54 +22,102 @@ static int max_node_num;
> >   */
> >  static int *cpunode_map;
> >
> > -static struct perf_cpu_map *cpu_map__from_entries(struct cpu_map_entries *cpus)
> > +bool perf_record_cpu_map_data__test_bit(int i,
> > +                                     const struct perf_record_cpu_map_data *data)
> > +{
> > +     int bit_word32 = i / 32;
> > +     __u32 bit_mask32 = 1U << (i & 31);
> > +     int bit_word64 = i / 64;
> > +     __u64 bit_mask64 = ((__u64)1) << (i & 63);
> > +
> > +     return (data->mask32_data.long_size == 4)
> > +             ? (bit_word32 < data->mask32_data.nr) &&
> > +             (data->mask32_data.mask[bit_word32] & bit_mask32) != 0
> > +             : (bit_word64 < data->mask64_data.nr) &&
> > +             (data->mask64_data.mask[bit_word64] & bit_mask64) != 0;
> > +}
> > +
> > +/* Read ith mask value from data into the given 64-bit sized bitmap */
> > +static void perf_record_cpu_map_data__read_one_mask(const struct perf_record_cpu_map_data *data,
> > +                                                 int i, unsigned long *bitmap)
> > +{
> > +#if __SIZEOF_LONG__ == 8
> > +     if (data->mask32_data.long_size == 4)
> > +             bitmap[0] = data->mask32_data.mask[i];
> > +     else
> > +             bitmap[0] = data->mask64_data.mask[i];
> > +#else
> > +     if (data->mask32_data.long_size == 4) {
> > +             bitmap[0] = data->mask32_data.mask[i];
> > +             bitmap[1] = 0;
> > +     } else {
> > +#if __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
> > +             bitmap[0] = (unsigned long)(data->mask64_data.mask[i] >> 32);
> > +             bitmap[1] = (unsigned long)data->mask64_data.mask[i];
> > +#else
> > +             bitmap[0] = (unsigned long)data->mask64_data.mask[i];
> > +             bitmap[1] = (unsigned long)(data->mask64_data.mask[i] >> 32);
> > +#endif
>
> should this be taken care of earlier by perf_event__cpu_map_swap ?

By the #ifdefs we're either in big or little endian 32-bit code here,
so long is 32-bit but the bitmap is 64-bit. For little endian we
pretty much just copy the bytes over, but for big endian the values
are swapped so the littler value occurs in the low-bits. The bytes are
still swapped from perf_event__cpu_map_swap but I was trying to make
sure that the order of the words was correct.

> > +     }
> > +#endif
> > +}
> > +static struct perf_cpu_map *cpu_map__from_entries(const struct perf_record_cpu_map_data *data)
> >  {
> >       struct perf_cpu_map *map;
> >
> > -     map = perf_cpu_map__empty_new(cpus->nr);
> > +     map = perf_cpu_map__empty_new(data->cpus_data.nr);
> >       if (map) {
> >               unsigned i;
> >
> > -             for (i = 0; i < cpus->nr; i++) {
> > +             for (i = 0; i < data->cpus_data.nr; i++) {
> >                       /*
> >                        * Special treatment for -1, which is not real cpu number,
> >                        * and we need to use (int) -1 to initialize map[i],
> >                        * otherwise it would become 65535.
> >                        */
> > -                     if (cpus->cpu[i] == (u16) -1)
> > +                     if (data->cpus_data.cpu[i] == (u16) -1)
> >                               map->map[i].cpu = -1;
> >                       else
> > -                             map->map[i].cpu = (int) cpus->cpu[i];
> > +                             map->map[i].cpu = (int) data->cpus_data.cpu[i];
> >               }
> >       }
> >
> >       return map;
> >  }
> >
> > -static struct perf_cpu_map *cpu_map__from_mask(struct perf_record_record_cpu_map *mask)
> > +static struct perf_cpu_map *cpu_map__from_mask(const struct perf_record_cpu_map_data *data)
> >  {
> > +     DECLARE_BITMAP(local_copy, 64);
> > +     int weight = 0, mask_nr = data->mask32_data.nr;
> >       struct perf_cpu_map *map;
> > -     int nr, nbits = mask->nr * mask->long_size * BITS_PER_BYTE;
> >
> > -     nr = bitmap_weight(mask->mask, nbits);
> > +     for (int i = 0; i < mask_nr; i++) {
> > +             perf_record_cpu_map_data__read_one_mask(data, i, local_copy);
> > +             weight += bitmap_weight(local_copy, 64);
> > +     }
> > +
> > +     map = perf_cpu_map__empty_new(weight);
> > +     if (!map)
> > +             return NULL;
> >
> > -     map = perf_cpu_map__empty_new(nr);
> > -     if (map) {
> > -             int cpu, i = 0;
> > +     for (int i = 0, j = 0; i < mask_nr; i++) {
> > +             int cpus_per_i = (i * data->mask32_data.long_size  * BITS_PER_BYTE);
> > +             int cpu;
> >
> > -             for_each_set_bit(cpu, mask->mask, nbits)
> > -                     map->map[i++].cpu = cpu;
> > +             perf_record_cpu_map_data__read_one_mask(data, i, local_copy);
> > +             for_each_set_bit(cpu, local_copy, 64)
> > +                     map->map[j++].cpu = cpu + cpus_per_i;
> >       }
> >       return map;
> >
> >  }
> >
> > -struct perf_cpu_map *cpu_map__new_data(struct perf_record_cpu_map_data *data)
> > +struct perf_cpu_map *cpu_map__new_data(const struct perf_record_cpu_map_data *data)
> >  {
> >       if (data->type == PERF_CPU_MAP__CPUS)
> > -             return cpu_map__from_entries((struct cpu_map_entries *)data->data);
> > +             return cpu_map__from_entries(data);
> >       else
> > -             return cpu_map__from_mask((struct perf_record_record_cpu_map *)data->data);
> > +             return cpu_map__from_mask(data);
> >  }
> >
> >  size_t cpu_map__fprintf(struct perf_cpu_map *map, FILE *fp)
> > diff --git a/tools/perf/util/cpumap.h b/tools/perf/util/cpumap.h
> > index 703ae6d3386e..fa8a5acdcae1 100644
> > --- a/tools/perf/util/cpumap.h
> > +++ b/tools/perf/util/cpumap.h
> > @@ -37,9 +37,11 @@ struct cpu_aggr_map {
> >
> >  struct perf_record_cpu_map_data;
> >
> > +bool perf_record_cpu_map_data__test_bit(int i, const struct perf_record_cpu_map_data *data);
> > +
> >  struct perf_cpu_map *perf_cpu_map__empty_new(int nr);
> >
> > -struct perf_cpu_map *cpu_map__new_data(struct perf_record_cpu_map_data *data);
> > +struct perf_cpu_map *cpu_map__new_data(const struct perf_record_cpu_map_data *data);
> >  size_t cpu_map__snprint(struct perf_cpu_map *map, char *buf, size_t size);
> >  size_t cpu_map__snprint_mask(struct perf_cpu_map *map, char *buf, size_t size);
> >  size_t cpu_map__fprintf(struct perf_cpu_map *map, FILE *fp);
> > diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
> > index 0aa818977d2b..d52a39ba48e3 100644
> > --- a/tools/perf/util/session.c
> > +++ b/tools/perf/util/session.c
> > @@ -914,30 +914,30 @@ static void perf_event__cpu_map_swap(union perf_event *event,
> >                                    bool sample_id_all __maybe_unused)
> >  {
> >       struct perf_record_cpu_map_data *data = &event->cpu_map.data;
> > -     struct cpu_map_entries *cpus;
> > -     struct perf_record_record_cpu_map *mask;
> > -     unsigned i;
> >
> >       data->type = bswap_16(data->type);
> >
> >       switch (data->type) {
> >       case PERF_CPU_MAP__CPUS:
> > -             cpus = (struct cpu_map_entries *)data->data;
> > -
> > -             cpus->nr = bswap_16(cpus->nr);
> > +             data->cpus_data.nr = bswap_16(data->cpus_data.nr);
> >
> > -             for (i = 0; i < cpus->nr; i++)
> > -                     cpus->cpu[i] = bswap_16(cpus->cpu[i]);
> > +             for (unsigned i = 0; i < data->cpus_data.nr; i++)
> > +                     data->cpus_data.cpu[i] = bswap_16(data->cpus_data.cpu[i]);
> >               break;
> >       case PERF_CPU_MAP__MASK:
> > -             mask = (struct perf_record_record_cpu_map *)data->data;
> > -
> > -             mask->nr = bswap_16(mask->nr);
> > -             mask->long_size = bswap_16(mask->long_size);
> > +             data->mask32_data.long_size = bswap_16(data->mask32_data.long_size);
> >
> > -             switch (mask->long_size) {
> > -             case 4: mem_bswap_32(&mask->mask, mask->nr); break;
> > -             case 8: mem_bswap_64(&mask->mask, mask->nr); break;
> > +             switch (data->mask32_data.long_size) {
> > +             case 4:
> > +                     data->mask32_data.nr = bswap_16(data->mask32_data.nr);
> > +                     for (unsigned i = 0; i < data->mask32_data.nr; i++)
> > +                             data->mask32_data.mask[i] = bswap_32(data->mask32_data.mask[i]);
> > +                     break;
>
> why not use the mem_bswap_* functions?

The mem_bswap_* assume the arrays of 32 or 64-bit values are aligned,
which isn't true here because of the packing. Passing the values
results in undefined behavior.

> looks like we never swap it completely, because we passed
> mask->nr where should be the size
>
> > +             case 8:
> > +                     data->mask64_data.nr = bswap_16(data->mask64_data.nr);
> > +                     for (unsigned i = 0; i < data->mask64_data.nr; i++)
> > +                             data->mask64_data.mask[i] = bswap_64(data->mask64_data.mask[i]);
> > +                     break;
> >               default:
> >                       pr_err("cpu_map swap: unsupported long size\n");
> >               }
> > diff --git a/tools/perf/util/synthetic-events.c b/tools/perf/util/synthetic-events.c
> > index 0d87df20ec44..4fa7d0d7dbcf 100644
> > --- a/tools/perf/util/synthetic-events.c
> > +++ b/tools/perf/util/synthetic-events.c
> > @@ -1183,27 +1183,33 @@ int perf_event__synthesize_thread_map2(struct perf_tool *tool,
> >       return err;
> >  }
> >
> > -static void synthesize_cpus(struct cpu_map_entries *cpus,
> > +static void synthesize_cpus(struct perf_record_cpu_map_data *data,
> >                           const struct perf_cpu_map *map)
> >  {
> >       int i, map_nr = perf_cpu_map__nr(map);
> >
> > -     cpus->nr = map_nr;
> > +     data->cpus_data.nr = map_nr;
> >
> >       for (i = 0; i < map_nr; i++)
> > -             cpus->cpu[i] = perf_cpu_map__cpu(map, i).cpu;
> > +             data->cpus_data.cpu[i] = perf_cpu_map__cpu(map, i).cpu;
> >  }
> >
> > -static void synthesize_mask(struct perf_record_record_cpu_map *mask,
> > +static void synthesize_mask(struct perf_record_cpu_map_data *data,
> >                           const struct perf_cpu_map *map, int max)
> >  {
> > -     int i;
> > +     int idx;
> > +     struct perf_cpu cpu;
> > +
> > +     /* Due to padding, the 4bytes per entry mask variant is always smaller. */
> > +     data->mask32_data.nr = BITS_TO_U32(max);
> > +     data->mask32_data.long_size = 4;
>
> ok, so we go always with 32 bit version

I couldn't come up with a condition that made the 64-bit version sensible :-)

> >
> > -     mask->nr = BITS_TO_LONGS(max);
> > -     mask->long_size = sizeof(long);
> > +     perf_cpu_map__for_each_cpu(cpu, idx, map) {
> > +             int bit_word = cpu.cpu / 32;
> > +             __u32 bit_mask = 1U << (cpu.cpu & 31);
>
> set_bit uses (nr % 32), but I guess it does not matter

Perhaps if nr were negative, better not thought about :-)

Thanks,
Ian

>
> jirka
>
> >
> > -     for (i = 0; i < perf_cpu_map__nr(map); i++)
> > -             set_bit(perf_cpu_map__cpu(map, i).cpu, mask->mask);
> > +             data->mask32_data.mask[bit_word] |= bit_mask;
> > +     }
> >  }
> >
> >  static size_t cpus_size(const struct perf_cpu_map *map)
> > @@ -1214,7 +1220,7 @@ static size_t cpus_size(const struct perf_cpu_map *map)
> >  static size_t mask_size(const struct perf_cpu_map *map, int *max)
> >  {
> >       *max = perf_cpu_map__max(map).cpu;
> > -     return sizeof(struct perf_record_record_cpu_map) + BITS_TO_LONGS(*max) * sizeof(long);
> > +     return sizeof(struct perf_record_mask_cpu_map32) + BITS_TO_U32(*max) * sizeof(__u32);
> >  }
> >
> >  static void *cpu_map_data__alloc(const struct perf_cpu_map *map, size_t *size,
> > @@ -1247,7 +1253,7 @@ static void *cpu_map_data__alloc(const struct perf_cpu_map *map, size_t *size,
> >               *type  = PERF_CPU_MAP__MASK;
> >       }
> >
> > -     *size += sizeof(struct perf_record_cpu_map_data);
> > +     *size += sizeof(__u16); /* For perf_record_cpu_map_data.type. */
> >       *size = PERF_ALIGN(*size, sizeof(u64));
> >       return zalloc(*size);
> >  }
> > @@ -1260,10 +1266,10 @@ static void cpu_map_data__synthesize(struct perf_record_cpu_map_data *data,
> >
> >       switch (type) {
> >       case PERF_CPU_MAP__CPUS:
> > -             synthesize_cpus((struct cpu_map_entries *) data->data, map);
> > +             synthesize_cpus(data, map);
> >               break;
> >       case PERF_CPU_MAP__MASK:
> > -             synthesize_mask((struct perf_record_record_cpu_map *)data->data, map, max);
> > +             synthesize_mask(data, map, max);
> >       default:
> >               break;
> >       }
> > @@ -1271,7 +1277,7 @@ static void cpu_map_data__synthesize(struct perf_record_cpu_map_data *data,
> >
> >  static struct perf_record_cpu_map *cpu_map_event__new(const struct perf_cpu_map *map)
> >  {
> > -     size_t size = sizeof(struct perf_record_cpu_map);
> > +     size_t size = sizeof(struct perf_event_header);
> >       struct perf_record_cpu_map *event;
> >       int max;
> >       u16 type;
> > --
> > 2.36.1.476.g0c4daa206d-goog
> >

Re: [PATCH v2 4/6] perf cpumap: Fix alignment for masks in event encoding

[Arnaldo Carvalho de Melo]

Em Tue, Jun 14, 2022 at 07:33:51AM -0700, Ian Rogers escreveu:
> A mask encoding of a cpu map is laid out as:
>   u16 nr
>   u16 long_size
>   unsigned long mask[];
> However, the mask may be 8-byte aligned meaning there is a 4-byte pad
> after long_size. This means 32-bit and 64-bit builds see the mask as
> being at different offsets. On top of this the structure is in the byte
> data[] encoded as:
>   u16 type
>   char data[]
> This means the mask's struct isn't the required 4 or 8 byte aligned, but
> is offset by 2. Consequently the long reads and writes are causing
> undefined behavior as the alignment is broken.
> 
> Fix the mask struct by creating explicit 32 and 64-bit variants, use a
> union to avoid data[] and casts; the struct must be packed so the
> layout matches the existing perf.data layout. Taking an address of a
> member of a packed struct breaks alignment so pass the packed
> perf_record_cpu_map_data to functions, so they can access variables with
> the right alignment.
> 
> As the 64-bit version has 4 bytes of padding, optimizing writing to only
> write the 32-bit version.
> 
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/lib/perf/include/perf/event.h | 36 +++++++++++--
>  tools/perf/tests/cpumap.c           | 19 ++++---
>  tools/perf/util/cpumap.c            | 80 +++++++++++++++++++++++------
>  tools/perf/util/cpumap.h            |  4 +-
>  tools/perf/util/session.c           | 30 +++++------
>  tools/perf/util/synthetic-events.c  | 34 +++++++-----
>  6 files changed, 143 insertions(+), 60 deletions(-)
> 
> diff --git a/tools/lib/perf/include/perf/event.h b/tools/lib/perf/include/perf/event.h
> index e7758707cadd..d2d32589758a 100644
> --- a/tools/lib/perf/include/perf/event.h
> +++ b/tools/lib/perf/include/perf/event.h
> @@ -6,6 +6,7 @@
>  #include <linux/types.h>
>  #include <linux/limits.h>
>  #include <linux/bpf.h>
> +#include <linux/compiler.h>
>  #include <sys/types.h> /* pid_t */
>  
>  #define event_contains(obj, mem) ((obj).header.size > offsetof(typeof(obj), mem))
> @@ -153,20 +154,47 @@ enum {
>  	PERF_CPU_MAP__MASK = 1,
>  };
>  
> +/*
> + * Array encoding of a perf_cpu_map where nr is the number of entries in cpu[]
> + * and each entry is a value for a CPU in the map.
> + */
>  struct cpu_map_entries {
>  	__u16			 nr;
>  	__u16			 cpu[];
>  };
>  
> -struct perf_record_record_cpu_map {
> +/* Bitmap encoding of a perf_cpu_map where bitmap entries are 32-bit. */
> +struct perf_record_mask_cpu_map32 {
> +	/* Number of mask values. */
>  	__u16			 nr;
> +	/* Constant 4. */
>  	__u16			 long_size;
> -	unsigned long		 mask[];
> +	/* Bitmap data. */
> +	__u32			 mask[];
>  };
>  
> -struct perf_record_cpu_map_data {
> +/* Bitmap encoding of a perf_cpu_map where bitmap entries are 64-bit. */
> +struct perf_record_mask_cpu_map64 {
> +	/* Number of mask values. */
> +	__u16			 nr;
> +	/* Constant 8. */
> +	__u16			 long_size;
> +	/* Legacy padding. */
> +	char                     __pad[4];
> +	/* Bitmap data. */
> +	__u64			 mask[];
> +};
> +
> +struct __packed perf_record_cpu_map_data {

In various places I'm getting this:

[perfbuilder@five x-riscv]$ export BUILD_TARBALL=http://192.168.86.14/perf/perf-6.0.0-rc1.tar.xz
[perfbuilder@five x-riscv]$ time dm .
   1     5.47 ubuntu:22.04-x-riscv64        : FAIL gcc version 11.2.0 (Ubuntu 11.2.0-16ubuntu1)
    In file included from mmap.c:10:
    /git/perf-6.0.0-rc1/tools/lib/perf/include/perf/event.h:190:34: error: packed attribute causes inefficient alignment for 'type' [-Werror=attributes]
      190 |         __u16                    type;
          |                                  ^~~~
    cc1: all warnings being treated as errors
    In file included from util/event.h:12,
                     from builtin-diff.c:12:
    /git/perf-6.0.0-rc1/tools/lib/perf/include/perf/event.h:190:34: error: packed attribute causes inefficient alignment for 'type' [-Werror=attributes]
      190 |         __u16                    type;
          |                                  ^~~~
    In file included from util/events_stats.h:6,
                     from util/evlist.h:12,
                     from builtin-evlist.c:11:
    /git/perf-6.0.0-rc1/tools/lib/perf/include/perf/event.h:190:34: error: packed attribute causes inefficient alignment for 'type' [-Werror=attributes]
      190 |         __u16                    type;
          |                                  ^~~~

So probably we need to disable this -Werror=attributes in some
architectures?

- Arnaldo

>  	__u16			 type;
> -	char			 data[];
> +	union {
> +		/* Used when type == PERF_CPU_MAP__CPUS. */
> +		struct cpu_map_entries cpus_data;
> +		/* Used when type == PERF_CPU_MAP__MASK and long_size == 4. */
> +		struct perf_record_mask_cpu_map32 mask32_data;
> +		/* Used when type == PERF_CPU_MAP__MASK and long_size == 8. */
> +		struct perf_record_mask_cpu_map64 mask64_data;
> +	};
>  };
>  
>  struct perf_record_cpu_map {
> diff --git a/tools/perf/tests/cpumap.c b/tools/perf/tests/cpumap.c
> index f94929ebb54b..7ea150cdc137 100644
> --- a/tools/perf/tests/cpumap.c
> +++ b/tools/perf/tests/cpumap.c
> @@ -17,21 +17,23 @@ static int process_event_mask(struct perf_tool *tool __maybe_unused,
>  			 struct machine *machine __maybe_unused)
>  {
>  	struct perf_record_cpu_map *map_event = &event->cpu_map;
> -	struct perf_record_record_cpu_map *mask;
>  	struct perf_record_cpu_map_data *data;
>  	struct perf_cpu_map *map;
>  	int i;
> +	unsigned int long_size;
>  
>  	data = &map_event->data;
>  
>  	TEST_ASSERT_VAL("wrong type", data->type == PERF_CPU_MAP__MASK);
>  
> -	mask = (struct perf_record_record_cpu_map *)data->data;
> +	long_size = data->mask32_data.long_size;
>  
> -	TEST_ASSERT_VAL("wrong nr",   mask->nr == 1);
> +	TEST_ASSERT_VAL("wrong long_size", long_size == 4 || long_size == 8);
> +
> +	TEST_ASSERT_VAL("wrong nr",   data->mask32_data.nr == 1);
>  
>  	for (i = 0; i < 20; i++) {
> -		TEST_ASSERT_VAL("wrong cpu", test_bit(i, mask->mask));
> +		TEST_ASSERT_VAL("wrong cpu", perf_record_cpu_map_data__test_bit(i, data));
>  	}
>  
>  	map = cpu_map__new_data(data);
> @@ -51,7 +53,6 @@ static int process_event_cpus(struct perf_tool *tool __maybe_unused,
>  			 struct machine *machine __maybe_unused)
>  {
>  	struct perf_record_cpu_map *map_event = &event->cpu_map;
> -	struct cpu_map_entries *cpus;
>  	struct perf_record_cpu_map_data *data;
>  	struct perf_cpu_map *map;
>  
> @@ -59,11 +60,9 @@ static int process_event_cpus(struct perf_tool *tool __maybe_unused,
>  
>  	TEST_ASSERT_VAL("wrong type", data->type == PERF_CPU_MAP__CPUS);
>  
> -	cpus = (struct cpu_map_entries *)data->data;
> -
> -	TEST_ASSERT_VAL("wrong nr",   cpus->nr == 2);
> -	TEST_ASSERT_VAL("wrong cpu",  cpus->cpu[0] == 1);
> -	TEST_ASSERT_VAL("wrong cpu",  cpus->cpu[1] == 256);
> +	TEST_ASSERT_VAL("wrong nr",   data->cpus_data.nr == 2);
> +	TEST_ASSERT_VAL("wrong cpu",  data->cpus_data.cpu[0] == 1);
> +	TEST_ASSERT_VAL("wrong cpu",  data->cpus_data.cpu[1] == 256);
>  
>  	map = cpu_map__new_data(data);
>  	TEST_ASSERT_VAL("wrong nr",  perf_cpu_map__nr(map) == 2);
> diff --git a/tools/perf/util/cpumap.c b/tools/perf/util/cpumap.c
> index 12b2243222b0..ae43fb88f444 100644
> --- a/tools/perf/util/cpumap.c
> +++ b/tools/perf/util/cpumap.c
> @@ -22,54 +22,102 @@ static int max_node_num;
>   */
>  static int *cpunode_map;
>  
> -static struct perf_cpu_map *cpu_map__from_entries(struct cpu_map_entries *cpus)
> +bool perf_record_cpu_map_data__test_bit(int i,
> +					const struct perf_record_cpu_map_data *data)
> +{
> +	int bit_word32 = i / 32;
> +	__u32 bit_mask32 = 1U << (i & 31);
> +	int bit_word64 = i / 64;
> +	__u64 bit_mask64 = ((__u64)1) << (i & 63);
> +
> +	return (data->mask32_data.long_size == 4)
> +		? (bit_word32 < data->mask32_data.nr) &&
> +		(data->mask32_data.mask[bit_word32] & bit_mask32) != 0
> +		: (bit_word64 < data->mask64_data.nr) &&
> +		(data->mask64_data.mask[bit_word64] & bit_mask64) != 0;
> +}
> +
> +/* Read ith mask value from data into the given 64-bit sized bitmap */
> +static void perf_record_cpu_map_data__read_one_mask(const struct perf_record_cpu_map_data *data,
> +						    int i, unsigned long *bitmap)
> +{
> +#if __SIZEOF_LONG__ == 8
> +	if (data->mask32_data.long_size == 4)
> +		bitmap[0] = data->mask32_data.mask[i];
> +	else
> +		bitmap[0] = data->mask64_data.mask[i];
> +#else
> +	if (data->mask32_data.long_size == 4) {
> +		bitmap[0] = data->mask32_data.mask[i];
> +		bitmap[1] = 0;
> +	} else {
> +#if __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
> +		bitmap[0] = (unsigned long)(data->mask64_data.mask[i] >> 32);
> +		bitmap[1] = (unsigned long)data->mask64_data.mask[i];
> +#else
> +		bitmap[0] = (unsigned long)data->mask64_data.mask[i];
> +		bitmap[1] = (unsigned long)(data->mask64_data.mask[i] >> 32);
> +#endif
> +	}
> +#endif
> +}
> +static struct perf_cpu_map *cpu_map__from_entries(const struct perf_record_cpu_map_data *data)
>  {
>  	struct perf_cpu_map *map;
>  
> -	map = perf_cpu_map__empty_new(cpus->nr);
> +	map = perf_cpu_map__empty_new(data->cpus_data.nr);
>  	if (map) {
>  		unsigned i;
>  
> -		for (i = 0; i < cpus->nr; i++) {
> +		for (i = 0; i < data->cpus_data.nr; i++) {
>  			/*
>  			 * Special treatment for -1, which is not real cpu number,
>  			 * and we need to use (int) -1 to initialize map[i],
>  			 * otherwise it would become 65535.
>  			 */
> -			if (cpus->cpu[i] == (u16) -1)
> +			if (data->cpus_data.cpu[i] == (u16) -1)
>  				map->map[i].cpu = -1;
>  			else
> -				map->map[i].cpu = (int) cpus->cpu[i];
> +				map->map[i].cpu = (int) data->cpus_data.cpu[i];
>  		}
>  	}
>  
>  	return map;
>  }
>  
> -static struct perf_cpu_map *cpu_map__from_mask(struct perf_record_record_cpu_map *mask)
> +static struct perf_cpu_map *cpu_map__from_mask(const struct perf_record_cpu_map_data *data)
>  {
> +	DECLARE_BITMAP(local_copy, 64);
> +	int weight = 0, mask_nr = data->mask32_data.nr;
>  	struct perf_cpu_map *map;
> -	int nr, nbits = mask->nr * mask->long_size * BITS_PER_BYTE;
>  
> -	nr = bitmap_weight(mask->mask, nbits);
> +	for (int i = 0; i < mask_nr; i++) {
> +		perf_record_cpu_map_data__read_one_mask(data, i, local_copy);
> +		weight += bitmap_weight(local_copy, 64);
> +	}
> +
> +	map = perf_cpu_map__empty_new(weight);
> +	if (!map)
> +		return NULL;
>  
> -	map = perf_cpu_map__empty_new(nr);
> -	if (map) {
> -		int cpu, i = 0;
> +	for (int i = 0, j = 0; i < mask_nr; i++) {
> +		int cpus_per_i = (i * data->mask32_data.long_size  * BITS_PER_BYTE);
> +		int cpu;
>  
> -		for_each_set_bit(cpu, mask->mask, nbits)
> -			map->map[i++].cpu = cpu;
> +		perf_record_cpu_map_data__read_one_mask(data, i, local_copy);
> +		for_each_set_bit(cpu, local_copy, 64)
> +			map->map[j++].cpu = cpu + cpus_per_i;
>  	}
>  	return map;
>  
>  }
>  
> -struct perf_cpu_map *cpu_map__new_data(struct perf_record_cpu_map_data *data)
> +struct perf_cpu_map *cpu_map__new_data(const struct perf_record_cpu_map_data *data)
>  {
>  	if (data->type == PERF_CPU_MAP__CPUS)
> -		return cpu_map__from_entries((struct cpu_map_entries *)data->data);
> +		return cpu_map__from_entries(data);
>  	else
> -		return cpu_map__from_mask((struct perf_record_record_cpu_map *)data->data);
> +		return cpu_map__from_mask(data);
>  }
>  
>  size_t cpu_map__fprintf(struct perf_cpu_map *map, FILE *fp)
> diff --git a/tools/perf/util/cpumap.h b/tools/perf/util/cpumap.h
> index 703ae6d3386e..fa8a5acdcae1 100644
> --- a/tools/perf/util/cpumap.h
> +++ b/tools/perf/util/cpumap.h
> @@ -37,9 +37,11 @@ struct cpu_aggr_map {
>  
>  struct perf_record_cpu_map_data;
>  
> +bool perf_record_cpu_map_data__test_bit(int i, const struct perf_record_cpu_map_data *data);
> +
>  struct perf_cpu_map *perf_cpu_map__empty_new(int nr);
>  
> -struct perf_cpu_map *cpu_map__new_data(struct perf_record_cpu_map_data *data);
> +struct perf_cpu_map *cpu_map__new_data(const struct perf_record_cpu_map_data *data);
>  size_t cpu_map__snprint(struct perf_cpu_map *map, char *buf, size_t size);
>  size_t cpu_map__snprint_mask(struct perf_cpu_map *map, char *buf, size_t size);
>  size_t cpu_map__fprintf(struct perf_cpu_map *map, FILE *fp);
> diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
> index 0aa818977d2b..d52a39ba48e3 100644
> --- a/tools/perf/util/session.c
> +++ b/tools/perf/util/session.c
> @@ -914,30 +914,30 @@ static void perf_event__cpu_map_swap(union perf_event *event,
>  				     bool sample_id_all __maybe_unused)
>  {
>  	struct perf_record_cpu_map_data *data = &event->cpu_map.data;
> -	struct cpu_map_entries *cpus;
> -	struct perf_record_record_cpu_map *mask;
> -	unsigned i;
>  
>  	data->type = bswap_16(data->type);
>  
>  	switch (data->type) {
>  	case PERF_CPU_MAP__CPUS:
> -		cpus = (struct cpu_map_entries *)data->data;
> -
> -		cpus->nr = bswap_16(cpus->nr);
> +		data->cpus_data.nr = bswap_16(data->cpus_data.nr);
>  
> -		for (i = 0; i < cpus->nr; i++)
> -			cpus->cpu[i] = bswap_16(cpus->cpu[i]);
> +		for (unsigned i = 0; i < data->cpus_data.nr; i++)
> +			data->cpus_data.cpu[i] = bswap_16(data->cpus_data.cpu[i]);
>  		break;
>  	case PERF_CPU_MAP__MASK:
> -		mask = (struct perf_record_record_cpu_map *)data->data;
> -
> -		mask->nr = bswap_16(mask->nr);
> -		mask->long_size = bswap_16(mask->long_size);
> +		data->mask32_data.long_size = bswap_16(data->mask32_data.long_size);
>  
> -		switch (mask->long_size) {
> -		case 4: mem_bswap_32(&mask->mask, mask->nr); break;
> -		case 8: mem_bswap_64(&mask->mask, mask->nr); break;
> +		switch (data->mask32_data.long_size) {
> +		case 4:
> +			data->mask32_data.nr = bswap_16(data->mask32_data.nr);
> +			for (unsigned i = 0; i < data->mask32_data.nr; i++)
> +				data->mask32_data.mask[i] = bswap_32(data->mask32_data.mask[i]);
> +			break;
> +		case 8:
> +			data->mask64_data.nr = bswap_16(data->mask64_data.nr);
> +			for (unsigned i = 0; i < data->mask64_data.nr; i++)
> +				data->mask64_data.mask[i] = bswap_64(data->mask64_data.mask[i]);
> +			break;
>  		default:
>  			pr_err("cpu_map swap: unsupported long size\n");
>  		}
> diff --git a/tools/perf/util/synthetic-events.c b/tools/perf/util/synthetic-events.c
> index 0d87df20ec44..4fa7d0d7dbcf 100644
> --- a/tools/perf/util/synthetic-events.c
> +++ b/tools/perf/util/synthetic-events.c
> @@ -1183,27 +1183,33 @@ int perf_event__synthesize_thread_map2(struct perf_tool *tool,
>  	return err;
>  }
>  
> -static void synthesize_cpus(struct cpu_map_entries *cpus,
> +static void synthesize_cpus(struct perf_record_cpu_map_data *data,
>  			    const struct perf_cpu_map *map)
>  {
>  	int i, map_nr = perf_cpu_map__nr(map);
>  
> -	cpus->nr = map_nr;
> +	data->cpus_data.nr = map_nr;
>  
>  	for (i = 0; i < map_nr; i++)
> -		cpus->cpu[i] = perf_cpu_map__cpu(map, i).cpu;
> +		data->cpus_data.cpu[i] = perf_cpu_map__cpu(map, i).cpu;
>  }
>  
> -static void synthesize_mask(struct perf_record_record_cpu_map *mask,
> +static void synthesize_mask(struct perf_record_cpu_map_data *data,
>  			    const struct perf_cpu_map *map, int max)
>  {
> -	int i;
> +	int idx;
> +	struct perf_cpu cpu;
> +
> +	/* Due to padding, the 4bytes per entry mask variant is always smaller. */
> +	data->mask32_data.nr = BITS_TO_U32(max);
> +	data->mask32_data.long_size = 4;
>  
> -	mask->nr = BITS_TO_LONGS(max);
> -	mask->long_size = sizeof(long);
> +	perf_cpu_map__for_each_cpu(cpu, idx, map) {
> +		int bit_word = cpu.cpu / 32;
> +		__u32 bit_mask = 1U << (cpu.cpu & 31);
>  
> -	for (i = 0; i < perf_cpu_map__nr(map); i++)
> -		set_bit(perf_cpu_map__cpu(map, i).cpu, mask->mask);
> +		data->mask32_data.mask[bit_word] |= bit_mask;
> +	}
>  }
>  
>  static size_t cpus_size(const struct perf_cpu_map *map)
> @@ -1214,7 +1220,7 @@ static size_t cpus_size(const struct perf_cpu_map *map)
>  static size_t mask_size(const struct perf_cpu_map *map, int *max)
>  {
>  	*max = perf_cpu_map__max(map).cpu;
> -	return sizeof(struct perf_record_record_cpu_map) + BITS_TO_LONGS(*max) * sizeof(long);
> +	return sizeof(struct perf_record_mask_cpu_map32) + BITS_TO_U32(*max) * sizeof(__u32);
>  }
>  
>  static void *cpu_map_data__alloc(const struct perf_cpu_map *map, size_t *size,
> @@ -1247,7 +1253,7 @@ static void *cpu_map_data__alloc(const struct perf_cpu_map *map, size_t *size,
>  		*type  = PERF_CPU_MAP__MASK;
>  	}
>  
> -	*size += sizeof(struct perf_record_cpu_map_data);
> +	*size += sizeof(__u16); /* For perf_record_cpu_map_data.type. */
>  	*size = PERF_ALIGN(*size, sizeof(u64));
>  	return zalloc(*size);
>  }
> @@ -1260,10 +1266,10 @@ static void cpu_map_data__synthesize(struct perf_record_cpu_map_data *data,
>  
>  	switch (type) {
>  	case PERF_CPU_MAP__CPUS:
> -		synthesize_cpus((struct cpu_map_entries *) data->data, map);
> +		synthesize_cpus(data, map);
>  		break;
>  	case PERF_CPU_MAP__MASK:
> -		synthesize_mask((struct perf_record_record_cpu_map *)data->data, map, max);
> +		synthesize_mask(data, map, max);
>  	default:
>  		break;
>  	}
> @@ -1271,7 +1277,7 @@ static void cpu_map_data__synthesize(struct perf_record_cpu_map_data *data,
>  
>  static struct perf_record_cpu_map *cpu_map_event__new(const struct perf_cpu_map *map)
>  {
> -	size_t size = sizeof(struct perf_record_cpu_map);
> +	size_t size = sizeof(struct perf_event_header);
>  	struct perf_record_cpu_map *event;
>  	int max;
>  	u16 type;
> -- 
> 2.36.1.476.g0c4daa206d-goog

-- 

- Arnaldo

Re: [PATCH v2 4/6] perf cpumap: Fix alignment for masks in event encoding

[Ian Rogers]

On Thu, Aug 18, 2022 at 2:50 PM Arnaldo Carvalho de Melo
<acme@kernel.org> wrote:
>
> Em Tue, Jun 14, 2022 at 07:33:51AM -0700, Ian Rogers escreveu:
> > A mask encoding of a cpu map is laid out as:
> >   u16 nr
> >   u16 long_size
> >   unsigned long mask[];
> > However, the mask may be 8-byte aligned meaning there is a 4-byte pad
> > after long_size. This means 32-bit and 64-bit builds see the mask as
> > being at different offsets. On top of this the structure is in the byte
> > data[] encoded as:
> >   u16 type
> >   char data[]
> > This means the mask's struct isn't the required 4 or 8 byte aligned, but
> > is offset by 2. Consequently the long reads and writes are causing
> > undefined behavior as the alignment is broken.
> >
> > Fix the mask struct by creating explicit 32 and 64-bit variants, use a
> > union to avoid data[] and casts; the struct must be packed so the
> > layout matches the existing perf.data layout. Taking an address of a
> > member of a packed struct breaks alignment so pass the packed
> > perf_record_cpu_map_data to functions, so they can access variables with
> > the right alignment.
> >
> > As the 64-bit version has 4 bytes of padding, optimizing writing to only
> > write the 32-bit version.
> >
> > Signed-off-by: Ian Rogers <irogers@google.com>
> > ---
> >  tools/lib/perf/include/perf/event.h | 36 +++++++++++--
> >  tools/perf/tests/cpumap.c           | 19 ++++---
> >  tools/perf/util/cpumap.c            | 80 +++++++++++++++++++++++------
> >  tools/perf/util/cpumap.h            |  4 +-
> >  tools/perf/util/session.c           | 30 +++++------
> >  tools/perf/util/synthetic-events.c  | 34 +++++++-----
> >  6 files changed, 143 insertions(+), 60 deletions(-)
> >
> > diff --git a/tools/lib/perf/include/perf/event.h b/tools/lib/perf/include/perf/event.h
> > index e7758707cadd..d2d32589758a 100644
> > --- a/tools/lib/perf/include/perf/event.h
> > +++ b/tools/lib/perf/include/perf/event.h
> > @@ -6,6 +6,7 @@
> >  #include <linux/types.h>
> >  #include <linux/limits.h>
> >  #include <linux/bpf.h>
> > +#include <linux/compiler.h>
> >  #include <sys/types.h> /* pid_t */
> >
> >  #define event_contains(obj, mem) ((obj).header.size > offsetof(typeof(obj), mem))
> > @@ -153,20 +154,47 @@ enum {
> >       PERF_CPU_MAP__MASK = 1,
> >  };
> >
> > +/*
> > + * Array encoding of a perf_cpu_map where nr is the number of entries in cpu[]
> > + * and each entry is a value for a CPU in the map.
> > + */
> >  struct cpu_map_entries {
> >       __u16                    nr;
> >       __u16                    cpu[];
> >  };
> >
> > -struct perf_record_record_cpu_map {
> > +/* Bitmap encoding of a perf_cpu_map where bitmap entries are 32-bit. */
> > +struct perf_record_mask_cpu_map32 {
> > +     /* Number of mask values. */
> >       __u16                    nr;
> > +     /* Constant 4. */
> >       __u16                    long_size;
> > -     unsigned long            mask[];
> > +     /* Bitmap data. */
> > +     __u32                    mask[];
> >  };
> >
> > -struct perf_record_cpu_map_data {
> > +/* Bitmap encoding of a perf_cpu_map where bitmap entries are 64-bit. */
> > +struct perf_record_mask_cpu_map64 {
> > +     /* Number of mask values. */
> > +     __u16                    nr;
> > +     /* Constant 8. */
> > +     __u16                    long_size;
> > +     /* Legacy padding. */
> > +     char                     __pad[4];
> > +     /* Bitmap data. */
> > +     __u64                    mask[];
> > +};
> > +
> > +struct __packed perf_record_cpu_map_data {
>
> In various places I'm getting this:
>
> [perfbuilder@five x-riscv]$ export BUILD_TARBALL=http://192.168.86.14/perf/perf-6.0.0-rc1.tar.xz
> [perfbuilder@five x-riscv]$ time dm .
>    1     5.47 ubuntu:22.04-x-riscv64        : FAIL gcc version 11.2.0 (Ubuntu 11.2.0-16ubuntu1)
>     In file included from mmap.c:10:
>     /git/perf-6.0.0-rc1/tools/lib/perf/include/perf/event.h:190:34: error: packed attribute causes inefficient alignment for 'type' [-Werror=attributes]
>       190 |         __u16                    type;
>           |                                  ^~~~
>     cc1: all warnings being treated as errors
>     In file included from util/event.h:12,
>                      from builtin-diff.c:12:
>     /git/perf-6.0.0-rc1/tools/lib/perf/include/perf/event.h:190:34: error: packed attribute causes inefficient alignment for 'type' [-Werror=attributes]
>       190 |         __u16                    type;
>           |                                  ^~~~
>     In file included from util/events_stats.h:6,
>                      from util/evlist.h:12,
>                      from builtin-evlist.c:11:
>     /git/perf-6.0.0-rc1/tools/lib/perf/include/perf/event.h:190:34: error: packed attribute causes inefficient alignment for 'type' [-Werror=attributes]
>       190 |         __u16                    type;
>           |                                  ^~~~
>
> So probably we need to disable this -Werror=attributes in some
> architectures?

Looks like it. An inefficient load will be better than a SIGBUS.

Thanks,
Ian

> - Arnaldo
>
> >       __u16                    type;
> > -     char                     data[];
> > +     union {
> > +             /* Used when type == PERF_CPU_MAP__CPUS. */
> > +             struct cpu_map_entries cpus_data;
> > +             /* Used when type == PERF_CPU_MAP__MASK and long_size == 4. */
> > +             struct perf_record_mask_cpu_map32 mask32_data;
> > +             /* Used when type == PERF_CPU_MAP__MASK and long_size == 8. */
> > +             struct perf_record_mask_cpu_map64 mask64_data;
> > +     };
> >  };
> >
> >  struct perf_record_cpu_map {
> > diff --git a/tools/perf/tests/cpumap.c b/tools/perf/tests/cpumap.c
> > index f94929ebb54b..7ea150cdc137 100644
> > --- a/tools/perf/tests/cpumap.c
> > +++ b/tools/perf/tests/cpumap.c
> > @@ -17,21 +17,23 @@ static int process_event_mask(struct perf_tool *tool __maybe_unused,
> >                        struct machine *machine __maybe_unused)
> >  {
> >       struct perf_record_cpu_map *map_event = &event->cpu_map;
> > -     struct perf_record_record_cpu_map *mask;
> >       struct perf_record_cpu_map_data *data;
> >       struct perf_cpu_map *map;
> >       int i;
> > +     unsigned int long_size;
> >
> >       data = &map_event->data;
> >
> >       TEST_ASSERT_VAL("wrong type", data->type == PERF_CPU_MAP__MASK);
> >
> > -     mask = (struct perf_record_record_cpu_map *)data->data;
> > +     long_size = data->mask32_data.long_size;
> >
> > -     TEST_ASSERT_VAL("wrong nr",   mask->nr == 1);
> > +     TEST_ASSERT_VAL("wrong long_size", long_size == 4 || long_size == 8);
> > +
> > +     TEST_ASSERT_VAL("wrong nr",   data->mask32_data.nr == 1);
> >
> >       for (i = 0; i < 20; i++) {
> > -             TEST_ASSERT_VAL("wrong cpu", test_bit(i, mask->mask));
> > +             TEST_ASSERT_VAL("wrong cpu", perf_record_cpu_map_data__test_bit(i, data));
> >       }
> >
> >       map = cpu_map__new_data(data);
> > @@ -51,7 +53,6 @@ static int process_event_cpus(struct perf_tool *tool __maybe_unused,
> >                        struct machine *machine __maybe_unused)
> >  {
> >       struct perf_record_cpu_map *map_event = &event->cpu_map;
> > -     struct cpu_map_entries *cpus;
> >       struct perf_record_cpu_map_data *data;
> >       struct perf_cpu_map *map;
> >
> > @@ -59,11 +60,9 @@ static int process_event_cpus(struct perf_tool *tool __maybe_unused,
> >
> >       TEST_ASSERT_VAL("wrong type", data->type == PERF_CPU_MAP__CPUS);
> >
> > -     cpus = (struct cpu_map_entries *)data->data;
> > -
> > -     TEST_ASSERT_VAL("wrong nr",   cpus->nr == 2);
> > -     TEST_ASSERT_VAL("wrong cpu",  cpus->cpu[0] == 1);
> > -     TEST_ASSERT_VAL("wrong cpu",  cpus->cpu[1] == 256);
> > +     TEST_ASSERT_VAL("wrong nr",   data->cpus_data.nr == 2);
> > +     TEST_ASSERT_VAL("wrong cpu",  data->cpus_data.cpu[0] == 1);
> > +     TEST_ASSERT_VAL("wrong cpu",  data->cpus_data.cpu[1] == 256);
> >
> >       map = cpu_map__new_data(data);
> >       TEST_ASSERT_VAL("wrong nr",  perf_cpu_map__nr(map) == 2);
> > diff --git a/tools/perf/util/cpumap.c b/tools/perf/util/cpumap.c
> > index 12b2243222b0..ae43fb88f444 100644
> > --- a/tools/perf/util/cpumap.c
> > +++ b/tools/perf/util/cpumap.c
> > @@ -22,54 +22,102 @@ static int max_node_num;
> >   */
> >  static int *cpunode_map;
> >
> > -static struct perf_cpu_map *cpu_map__from_entries(struct cpu_map_entries *cpus)
> > +bool perf_record_cpu_map_data__test_bit(int i,
> > +                                     const struct perf_record_cpu_map_data *data)
> > +{
> > +     int bit_word32 = i / 32;
> > +     __u32 bit_mask32 = 1U << (i & 31);
> > +     int bit_word64 = i / 64;
> > +     __u64 bit_mask64 = ((__u64)1) << (i & 63);
> > +
> > +     return (data->mask32_data.long_size == 4)
> > +             ? (bit_word32 < data->mask32_data.nr) &&
> > +             (data->mask32_data.mask[bit_word32] & bit_mask32) != 0
> > +             : (bit_word64 < data->mask64_data.nr) &&
> > +             (data->mask64_data.mask[bit_word64] & bit_mask64) != 0;
> > +}
> > +
> > +/* Read ith mask value from data into the given 64-bit sized bitmap */
> > +static void perf_record_cpu_map_data__read_one_mask(const struct perf_record_cpu_map_data *data,
> > +                                                 int i, unsigned long *bitmap)
> > +{
> > +#if __SIZEOF_LONG__ == 8
> > +     if (data->mask32_data.long_size == 4)
> > +             bitmap[0] = data->mask32_data.mask[i];
> > +     else
> > +             bitmap[0] = data->mask64_data.mask[i];
> > +#else
> > +     if (data->mask32_data.long_size == 4) {
> > +             bitmap[0] = data->mask32_data.mask[i];
> > +             bitmap[1] = 0;
> > +     } else {
> > +#if __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
> > +             bitmap[0] = (unsigned long)(data->mask64_data.mask[i] >> 32);
> > +             bitmap[1] = (unsigned long)data->mask64_data.mask[i];
> > +#else
> > +             bitmap[0] = (unsigned long)data->mask64_data.mask[i];
> > +             bitmap[1] = (unsigned long)(data->mask64_data.mask[i] >> 32);
> > +#endif
> > +     }
> > +#endif
> > +}
> > +static struct perf_cpu_map *cpu_map__from_entries(const struct perf_record_cpu_map_data *data)
> >  {
> >       struct perf_cpu_map *map;
> >
> > -     map = perf_cpu_map__empty_new(cpus->nr);
> > +     map = perf_cpu_map__empty_new(data->cpus_data.nr);
> >       if (map) {
> >               unsigned i;
> >
> > -             for (i = 0; i < cpus->nr; i++) {
> > +             for (i = 0; i < data->cpus_data.nr; i++) {
> >                       /*
> >                        * Special treatment for -1, which is not real cpu number,
> >                        * and we need to use (int) -1 to initialize map[i],
> >                        * otherwise it would become 65535.
> >                        */
> > -                     if (cpus->cpu[i] == (u16) -1)
> > +                     if (data->cpus_data.cpu[i] == (u16) -1)
> >                               map->map[i].cpu = -1;
> >                       else
> > -                             map->map[i].cpu = (int) cpus->cpu[i];
> > +                             map->map[i].cpu = (int) data->cpus_data.cpu[i];
> >               }
> >       }
> >
> >       return map;
> >  }
> >
> > -static struct perf_cpu_map *cpu_map__from_mask(struct perf_record_record_cpu_map *mask)
> > +static struct perf_cpu_map *cpu_map__from_mask(const struct perf_record_cpu_map_data *data)
> >  {
> > +     DECLARE_BITMAP(local_copy, 64);
> > +     int weight = 0, mask_nr = data->mask32_data.nr;
> >       struct perf_cpu_map *map;
> > -     int nr, nbits = mask->nr * mask->long_size * BITS_PER_BYTE;
> >
> > -     nr = bitmap_weight(mask->mask, nbits);
> > +     for (int i = 0; i < mask_nr; i++) {
> > +             perf_record_cpu_map_data__read_one_mask(data, i, local_copy);
> > +             weight += bitmap_weight(local_copy, 64);
> > +     }
> > +
> > +     map = perf_cpu_map__empty_new(weight);
> > +     if (!map)
> > +             return NULL;
> >
> > -     map = perf_cpu_map__empty_new(nr);
> > -     if (map) {
> > -             int cpu, i = 0;
> > +     for (int i = 0, j = 0; i < mask_nr; i++) {
> > +             int cpus_per_i = (i * data->mask32_data.long_size  * BITS_PER_BYTE);
> > +             int cpu;
> >
> > -             for_each_set_bit(cpu, mask->mask, nbits)
> > -                     map->map[i++].cpu = cpu;
> > +             perf_record_cpu_map_data__read_one_mask(data, i, local_copy);
> > +             for_each_set_bit(cpu, local_copy, 64)
> > +                     map->map[j++].cpu = cpu + cpus_per_i;
> >       }
> >       return map;
> >
> >  }
> >
> > -struct perf_cpu_map *cpu_map__new_data(struct perf_record_cpu_map_data *data)
> > +struct perf_cpu_map *cpu_map__new_data(const struct perf_record_cpu_map_data *data)
> >  {
> >       if (data->type == PERF_CPU_MAP__CPUS)
> > -             return cpu_map__from_entries((struct cpu_map_entries *)data->data);
> > +             return cpu_map__from_entries(data);
> >       else
> > -             return cpu_map__from_mask((struct perf_record_record_cpu_map *)data->data);
> > +             return cpu_map__from_mask(data);
> >  }
> >
> >  size_t cpu_map__fprintf(struct perf_cpu_map *map, FILE *fp)
> > diff --git a/tools/perf/util/cpumap.h b/tools/perf/util/cpumap.h
> > index 703ae6d3386e..fa8a5acdcae1 100644
> > --- a/tools/perf/util/cpumap.h
> > +++ b/tools/perf/util/cpumap.h
> > @@ -37,9 +37,11 @@ struct cpu_aggr_map {
> >
> >  struct perf_record_cpu_map_data;
> >
> > +bool perf_record_cpu_map_data__test_bit(int i, const struct perf_record_cpu_map_data *data);
> > +
> >  struct perf_cpu_map *perf_cpu_map__empty_new(int nr);
> >
> > -struct perf_cpu_map *cpu_map__new_data(struct perf_record_cpu_map_data *data);
> > +struct perf_cpu_map *cpu_map__new_data(const struct perf_record_cpu_map_data *data);
> >  size_t cpu_map__snprint(struct perf_cpu_map *map, char *buf, size_t size);
> >  size_t cpu_map__snprint_mask(struct perf_cpu_map *map, char *buf, size_t size);
> >  size_t cpu_map__fprintf(struct perf_cpu_map *map, FILE *fp);
> > diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
> > index 0aa818977d2b..d52a39ba48e3 100644
> > --- a/tools/perf/util/session.c
> > +++ b/tools/perf/util/session.c
> > @@ -914,30 +914,30 @@ static void perf_event__cpu_map_swap(union perf_event *event,
> >                                    bool sample_id_all __maybe_unused)
> >  {
> >       struct perf_record_cpu_map_data *data = &event->cpu_map.data;
> > -     struct cpu_map_entries *cpus;
> > -     struct perf_record_record_cpu_map *mask;
> > -     unsigned i;
> >
> >       data->type = bswap_16(data->type);
> >
> >       switch (data->type) {
> >       case PERF_CPU_MAP__CPUS:
> > -             cpus = (struct cpu_map_entries *)data->data;
> > -
> > -             cpus->nr = bswap_16(cpus->nr);
> > +             data->cpus_data.nr = bswap_16(data->cpus_data.nr);
> >
> > -             for (i = 0; i < cpus->nr; i++)
> > -                     cpus->cpu[i] = bswap_16(cpus->cpu[i]);
> > +             for (unsigned i = 0; i < data->cpus_data.nr; i++)
> > +                     data->cpus_data.cpu[i] = bswap_16(data->cpus_data.cpu[i]);
> >               break;
> >       case PERF_CPU_MAP__MASK:
> > -             mask = (struct perf_record_record_cpu_map *)data->data;
> > -
> > -             mask->nr = bswap_16(mask->nr);
> > -             mask->long_size = bswap_16(mask->long_size);
> > +             data->mask32_data.long_size = bswap_16(data->mask32_data.long_size);
> >
> > -             switch (mask->long_size) {
> > -             case 4: mem_bswap_32(&mask->mask, mask->nr); break;
> > -             case 8: mem_bswap_64(&mask->mask, mask->nr); break;
> > +             switch (data->mask32_data.long_size) {
> > +             case 4:
> > +                     data->mask32_data.nr = bswap_16(data->mask32_data.nr);
> > +                     for (unsigned i = 0; i < data->mask32_data.nr; i++)
> > +                             data->mask32_data.mask[i] = bswap_32(data->mask32_data.mask[i]);
> > +                     break;
> > +             case 8:
> > +                     data->mask64_data.nr = bswap_16(data->mask64_data.nr);
> > +                     for (unsigned i = 0; i < data->mask64_data.nr; i++)
> > +                             data->mask64_data.mask[i] = bswap_64(data->mask64_data.mask[i]);
> > +                     break;
> >               default:
> >                       pr_err("cpu_map swap: unsupported long size\n");
> >               }
> > diff --git a/tools/perf/util/synthetic-events.c b/tools/perf/util/synthetic-events.c
> > index 0d87df20ec44..4fa7d0d7dbcf 100644
> > --- a/tools/perf/util/synthetic-events.c
> > +++ b/tools/perf/util/synthetic-events.c
> > @@ -1183,27 +1183,33 @@ int perf_event__synthesize_thread_map2(struct perf_tool *tool,
> >       return err;
> >  }
> >
> > -static void synthesize_cpus(struct cpu_map_entries *cpus,
> > +static void synthesize_cpus(struct perf_record_cpu_map_data *data,
> >                           const struct perf_cpu_map *map)
> >  {
> >       int i, map_nr = perf_cpu_map__nr(map);
> >
> > -     cpus->nr = map_nr;
> > +     data->cpus_data.nr = map_nr;
> >
> >       for (i = 0; i < map_nr; i++)
> > -             cpus->cpu[i] = perf_cpu_map__cpu(map, i).cpu;
> > +             data->cpus_data.cpu[i] = perf_cpu_map__cpu(map, i).cpu;
> >  }
> >
> > -static void synthesize_mask(struct perf_record_record_cpu_map *mask,
> > +static void synthesize_mask(struct perf_record_cpu_map_data *data,
> >                           const struct perf_cpu_map *map, int max)
> >  {
> > -     int i;
> > +     int idx;
> > +     struct perf_cpu cpu;
> > +
> > +     /* Due to padding, the 4bytes per entry mask variant is always smaller. */
> > +     data->mask32_data.nr = BITS_TO_U32(max);
> > +     data->mask32_data.long_size = 4;
> >
> > -     mask->nr = BITS_TO_LONGS(max);
> > -     mask->long_size = sizeof(long);
> > +     perf_cpu_map__for_each_cpu(cpu, idx, map) {
> > +             int bit_word = cpu.cpu / 32;
> > +             __u32 bit_mask = 1U << (cpu.cpu & 31);
> >
> > -     for (i = 0; i < perf_cpu_map__nr(map); i++)
> > -             set_bit(perf_cpu_map__cpu(map, i).cpu, mask->mask);
> > +             data->mask32_data.mask[bit_word] |= bit_mask;
> > +     }
> >  }
> >
> >  static size_t cpus_size(const struct perf_cpu_map *map)
> > @@ -1214,7 +1220,7 @@ static size_t cpus_size(const struct perf_cpu_map *map)
> >  static size_t mask_size(const struct perf_cpu_map *map, int *max)
> >  {
> >       *max = perf_cpu_map__max(map).cpu;
> > -     return sizeof(struct perf_record_record_cpu_map) + BITS_TO_LONGS(*max) * sizeof(long);
> > +     return sizeof(struct perf_record_mask_cpu_map32) + BITS_TO_U32(*max) * sizeof(__u32);
> >  }
> >
> >  static void *cpu_map_data__alloc(const struct perf_cpu_map *map, size_t *size,
> > @@ -1247,7 +1253,7 @@ static void *cpu_map_data__alloc(const struct perf_cpu_map *map, size_t *size,
> >               *type  = PERF_CPU_MAP__MASK;
> >       }
> >
> > -     *size += sizeof(struct perf_record_cpu_map_data);
> > +     *size += sizeof(__u16); /* For perf_record_cpu_map_data.type. */
> >       *size = PERF_ALIGN(*size, sizeof(u64));
> >       return zalloc(*size);
> >  }
> > @@ -1260,10 +1266,10 @@ static void cpu_map_data__synthesize(struct perf_record_cpu_map_data *data,
> >
> >       switch (type) {
> >       case PERF_CPU_MAP__CPUS:
> > -             synthesize_cpus((struct cpu_map_entries *) data->data, map);
> > +             synthesize_cpus(data, map);
> >               break;
> >       case PERF_CPU_MAP__MASK:
> > -             synthesize_mask((struct perf_record_record_cpu_map *)data->data, map, max);
> > +             synthesize_mask(data, map, max);
> >       default:
> >               break;
> >       }
> > @@ -1271,7 +1277,7 @@ static void cpu_map_data__synthesize(struct perf_record_cpu_map_data *data,
> >
> >  static struct perf_record_cpu_map *cpu_map_event__new(const struct perf_cpu_map *map)
> >  {
> > -     size_t size = sizeof(struct perf_record_cpu_map);
> > +     size_t size = sizeof(struct perf_event_header);
> >       struct perf_record_cpu_map *event;
> >       int max;
> >       u16 type;
> > --
> > 2.36.1.476.g0c4daa206d-goog
>
> --
>
> - Arnaldo

Re: [PATCH v2 4/6] perf cpumap: Fix alignment for masks in event encoding

[Arnaldo Carvalho de Melo]

Em Thu, Aug 18, 2022 at 06:50:00PM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Tue, Jun 14, 2022 at 07:33:51AM -0700, Ian Rogers escreveu:
> > A mask encoding of a cpu map is laid out as:
> >   u16 nr
> >   u16 long_size
> >   unsigned long mask[];
> > However, the mask may be 8-byte aligned meaning there is a 4-byte pad
> > after long_size. This means 32-bit and 64-bit builds see the mask as
> > being at different offsets. On top of this the structure is in the byte
> > data[] encoded as:
> >   u16 type
> >   char data[]
> > This means the mask's struct isn't the required 4 or 8 byte aligned, but
> > is offset by 2. Consequently the long reads and writes are causing
> > undefined behavior as the alignment is broken.
> > 
> > Fix the mask struct by creating explicit 32 and 64-bit variants, use a
> > union to avoid data[] and casts; the struct must be packed so the
> > layout matches the existing perf.data layout. Taking an address of a
> > member of a packed struct breaks alignment so pass the packed
> > perf_record_cpu_map_data to functions, so they can access variables with
> > the right alignment.
> > 
> > As the 64-bit version has 4 bytes of padding, optimizing writing to only
> > write the 32-bit version.
> > 
> > Signed-off-by: Ian Rogers <irogers@google.com>
> > ---
> >  tools/lib/perf/include/perf/event.h | 36 +++++++++++--
> >  tools/perf/tests/cpumap.c           | 19 ++++---
> >  tools/perf/util/cpumap.c            | 80 +++++++++++++++++++++++------
> >  tools/perf/util/cpumap.h            |  4 +-
> >  tools/perf/util/session.c           | 30 +++++------
> >  tools/perf/util/synthetic-events.c  | 34 +++++++-----
> >  6 files changed, 143 insertions(+), 60 deletions(-)
> > 
> > diff --git a/tools/lib/perf/include/perf/event.h b/tools/lib/perf/include/perf/event.h
> > index e7758707cadd..d2d32589758a 100644
> > --- a/tools/lib/perf/include/perf/event.h
> > +++ b/tools/lib/perf/include/perf/event.h
> > @@ -6,6 +6,7 @@
> >  #include <linux/types.h>
> >  #include <linux/limits.h>
> >  #include <linux/bpf.h>
> > +#include <linux/compiler.h>
> >  #include <sys/types.h> /* pid_t */
> >  
> >  #define event_contains(obj, mem) ((obj).header.size > offsetof(typeof(obj), mem))
> > @@ -153,20 +154,47 @@ enum {
> >  	PERF_CPU_MAP__MASK = 1,
> >  };
> >  
> > +/*
> > + * Array encoding of a perf_cpu_map where nr is the number of entries in cpu[]
> > + * and each entry is a value for a CPU in the map.
> > + */
> >  struct cpu_map_entries {
> >  	__u16			 nr;
> >  	__u16			 cpu[];
> >  };
> >  
> > -struct perf_record_record_cpu_map {
> > +/* Bitmap encoding of a perf_cpu_map where bitmap entries are 32-bit. */
> > +struct perf_record_mask_cpu_map32 {
> > +	/* Number of mask values. */
> >  	__u16			 nr;
> > +	/* Constant 4. */
> >  	__u16			 long_size;
> > -	unsigned long		 mask[];
> > +	/* Bitmap data. */
> > +	__u32			 mask[];
> >  };
> >  
> > -struct perf_record_cpu_map_data {
> > +/* Bitmap encoding of a perf_cpu_map where bitmap entries are 64-bit. */
> > +struct perf_record_mask_cpu_map64 {
> > +	/* Number of mask values. */
> > +	__u16			 nr;
> > +	/* Constant 8. */
> > +	__u16			 long_size;
> > +	/* Legacy padding. */
> > +	char                     __pad[4];
> > +	/* Bitmap data. */
> > +	__u64			 mask[];
> > +};
> > +
> > +struct __packed perf_record_cpu_map_data {
> 
> In various places I'm getting this:
> 
> [perfbuilder@five x-riscv]$ export BUILD_TARBALL=http://192.168.86.14/perf/perf-6.0.0-rc1.tar.xz
> [perfbuilder@five x-riscv]$ time dm .
>    1     5.47 ubuntu:22.04-x-riscv64        : FAIL gcc version 11.2.0 (Ubuntu 11.2.0-16ubuntu1)
>     In file included from mmap.c:10:
>     /git/perf-6.0.0-rc1/tools/lib/perf/include/perf/event.h:190:34: error: packed attribute causes inefficient alignment for 'type' [-Werror=attributes]
>       190 |         __u16                    type;
>           |                                  ^~~~
>     cc1: all warnings being treated as errors
>     In file included from util/event.h:12,
>                      from builtin-diff.c:12:
>     /git/perf-6.0.0-rc1/tools/lib/perf/include/perf/event.h:190:34: error: packed attribute causes inefficient alignment for 'type' [-Werror=attributes]
>       190 |         __u16                    type;
>           |                                  ^~~~
>     In file included from util/events_stats.h:6,
>                      from util/evlist.h:12,
>                      from builtin-evlist.c:11:
>     /git/perf-6.0.0-rc1/tools/lib/perf/include/perf/event.h:190:34: error: packed attribute causes inefficient alignment for 'type' [-Werror=attributes]
>       190 |         __u16                    type;
>           |                                  ^~~~
> 
> So probably we need to disable this -Werror=attributes in some
> architectures?

Slapped this there:

#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Wpacked"
#pragma GCC diagnostic ignored "-Wattributes"

struct __packed perf_record_cpu_map_data {
        __u16                    type;
        union {
                /* Used when type == PERF_CPU_MAP__CPUS. */
                struct cpu_map_entries cpus_data;
                /* Used when type == PERF_CPU_MAP__MASK and long_size == 4. */
                struct perf_record_mask_cpu_map32 mask32_data;
                /* Used when type == PERF_CPU_MAP__MASK and long_size == 8. */
                struct perf_record_mask_cpu_map64 mask64_data;
        };
};

#pragma GCC diagnostic pop

Re: [PATCH v2 4/6] perf cpumap: Fix alignment for masks in event encoding

[Ian Rogers]

On Fri, Aug 19, 2022 at 8:58 AM Arnaldo Carvalho de Melo
<acme@kernel.org> wrote:
>
> Em Thu, Aug 18, 2022 at 06:50:00PM -0300, Arnaldo Carvalho de Melo escreveu:
> > Em Tue, Jun 14, 2022 at 07:33:51AM -0700, Ian Rogers escreveu:
> > > A mask encoding of a cpu map is laid out as:
> > >   u16 nr
> > >   u16 long_size
> > >   unsigned long mask[];
> > > However, the mask may be 8-byte aligned meaning there is a 4-byte pad
> > > after long_size. This means 32-bit and 64-bit builds see the mask as
> > > being at different offsets. On top of this the structure is in the byte
> > > data[] encoded as:
> > >   u16 type
> > >   char data[]
> > > This means the mask's struct isn't the required 4 or 8 byte aligned, but
> > > is offset by 2. Consequently the long reads and writes are causing
> > > undefined behavior as the alignment is broken.
> > >
> > > Fix the mask struct by creating explicit 32 and 64-bit variants, use a
> > > union to avoid data[] and casts; the struct must be packed so the
> > > layout matches the existing perf.data layout. Taking an address of a
> > > member of a packed struct breaks alignment so pass the packed
> > > perf_record_cpu_map_data to functions, so they can access variables with
> > > the right alignment.
> > >
> > > As the 64-bit version has 4 bytes of padding, optimizing writing to only
> > > write the 32-bit version.
> > >
> > > Signed-off-by: Ian Rogers <irogers@google.com>
> > > ---
> > >  tools/lib/perf/include/perf/event.h | 36 +++++++++++--
> > >  tools/perf/tests/cpumap.c           | 19 ++++---
> > >  tools/perf/util/cpumap.c            | 80 +++++++++++++++++++++++------
> > >  tools/perf/util/cpumap.h            |  4 +-
> > >  tools/perf/util/session.c           | 30 +++++------
> > >  tools/perf/util/synthetic-events.c  | 34 +++++++-----
> > >  6 files changed, 143 insertions(+), 60 deletions(-)
> > >
> > > diff --git a/tools/lib/perf/include/perf/event.h b/tools/lib/perf/include/perf/event.h
> > > index e7758707cadd..d2d32589758a 100644
> > > --- a/tools/lib/perf/include/perf/event.h
> > > +++ b/tools/lib/perf/include/perf/event.h
> > > @@ -6,6 +6,7 @@
> > >  #include <linux/types.h>
> > >  #include <linux/limits.h>
> > >  #include <linux/bpf.h>
> > > +#include <linux/compiler.h>
> > >  #include <sys/types.h> /* pid_t */
> > >
> > >  #define event_contains(obj, mem) ((obj).header.size > offsetof(typeof(obj), mem))
> > > @@ -153,20 +154,47 @@ enum {
> > >     PERF_CPU_MAP__MASK = 1,
> > >  };
> > >
> > > +/*
> > > + * Array encoding of a perf_cpu_map where nr is the number of entries in cpu[]
> > > + * and each entry is a value for a CPU in the map.
> > > + */
> > >  struct cpu_map_entries {
> > >     __u16                    nr;
> > >     __u16                    cpu[];
> > >  };
> > >
> > > -struct perf_record_record_cpu_map {
> > > +/* Bitmap encoding of a perf_cpu_map where bitmap entries are 32-bit. */
> > > +struct perf_record_mask_cpu_map32 {
> > > +   /* Number of mask values. */
> > >     __u16                    nr;
> > > +   /* Constant 4. */
> > >     __u16                    long_size;
> > > -   unsigned long            mask[];
> > > +   /* Bitmap data. */
> > > +   __u32                    mask[];
> > >  };
> > >
> > > -struct perf_record_cpu_map_data {
> > > +/* Bitmap encoding of a perf_cpu_map where bitmap entries are 64-bit. */
> > > +struct perf_record_mask_cpu_map64 {
> > > +   /* Number of mask values. */
> > > +   __u16                    nr;
> > > +   /* Constant 8. */
> > > +   __u16                    long_size;
> > > +   /* Legacy padding. */
> > > +   char                     __pad[4];
> > > +   /* Bitmap data. */
> > > +   __u64                    mask[];
> > > +};
> > > +
> > > +struct __packed perf_record_cpu_map_data {
> >
> > In various places I'm getting this:
> >
> > [perfbuilder@five x-riscv]$ export BUILD_TARBALL=http://192.168.86.14/perf/perf-6.0.0-rc1.tar.xz
> > [perfbuilder@five x-riscv]$ time dm .
> >    1     5.47 ubuntu:22.04-x-riscv64        : FAIL gcc version 11.2.0 (Ubuntu 11.2.0-16ubuntu1)
> >     In file included from mmap.c:10:
> >     /git/perf-6.0.0-rc1/tools/lib/perf/include/perf/event.h:190:34: error: packed attribute causes inefficient alignment for 'type' [-Werror=attributes]
> >       190 |         __u16                    type;
> >           |                                  ^~~~
> >     cc1: all warnings being treated as errors
> >     In file included from util/event.h:12,
> >                      from builtin-diff.c:12:
> >     /git/perf-6.0.0-rc1/tools/lib/perf/include/perf/event.h:190:34: error: packed attribute causes inefficient alignment for 'type' [-Werror=attributes]
> >       190 |         __u16                    type;
> >           |                                  ^~~~
> >     In file included from util/events_stats.h:6,
> >                      from util/evlist.h:12,
> >                      from builtin-evlist.c:11:
> >     /git/perf-6.0.0-rc1/tools/lib/perf/include/perf/event.h:190:34: error: packed attribute causes inefficient alignment for 'type' [-Werror=attributes]
> >       190 |         __u16                    type;
> >           |                                  ^~~~
> >
> > So probably we need to disable this -Werror=attributes in some
> > architectures?
>
> Slapped this there:
>
> #pragma GCC diagnostic push
> #pragma GCC diagnostic ignored "-Wpacked"
> #pragma GCC diagnostic ignored "-Wattributes"
>
> struct __packed perf_record_cpu_map_data {
>         __u16                    type;
>         union {
>                 /* Used when type == PERF_CPU_MAP__CPUS. */
>                 struct cpu_map_entries cpus_data;
>                 /* Used when type == PERF_CPU_MAP__MASK and long_size == 4. */
>                 struct perf_record_mask_cpu_map32 mask32_data;
>                 /* Used when type == PERF_CPU_MAP__MASK and long_size == 8. */
>                 struct perf_record_mask_cpu_map64 mask64_data;
>         };
> };
>
> #pragma GCC diagnostic pop

Perhaps we should also carry a comment like:
perf_record_cpu_map_data is packed as unfortunately an earlier version
had unaligned data and we wish to retain file format compatibility.

Thanks,
Ian

Re: [PATCH v2 4/6] perf cpumap: Fix alignment for masks in event encoding

[Arnaldo Carvalho de Melo]

On August 19, 2022 2:09:09 PM GMT-03:00, Ian Rogers <irogers@google.com> wrote:
>On Fri, Aug 19, 2022 at 8:58 AM Arnaldo Carvalho de Melo
><acme@kernel.org> wrote:
>>
>> Em Thu, Aug 18, 2022 at 06:50:00PM -0300, Arnaldo Carvalho de Melo escreveu:
>> > Em Tue, Jun 14, 2022 at 07:33:51AM -0700, Ian Rogers escreveu:
>> > > A mask encoding of a cpu map is laid out as:
>> > >   u16 nr
>> > >   u16 long_size
>> > >   unsigned long mask[];
>> > > However, the mask may be 8-byte aligned meaning there is a 4-byte pad
>> > > after long_size. This means 32-bit and 64-bit builds see the mask as
>> > > being at different offsets. On top of this the structure is in the byte
>> > > data[] encoded as:
>> > >   u16 type
>> > >   char data[]
>> > > This means the mask's struct isn't the required 4 or 8 byte aligned, but
>> > > is offset by 2. Consequently the long reads and writes are causing
>> > > undefined behavior as the alignment is broken.
>> > >
>> > > Fix the mask struct by creating explicit 32 and 64-bit variants, use a
>> > > union to avoid data[] and casts; the struct must be packed so the
>> > > layout matches the existing perf.data layout. Taking an address of a
>> > > member of a packed struct breaks alignment so pass the packed
>> > > perf_record_cpu_map_data to functions, so they can access variables with
>> > > the right alignment.
>> > >
>> > > As the 64-bit version has 4 bytes of padding, optimizing writing to only
>> > > write the 32-bit version.
>> > >
>> > > Signed-off-by: Ian Rogers <irogers@google.com>
>> > > ---
>> > >  tools/lib/perf/include/perf/event.h | 36 +++++++++++--
>> > >  tools/perf/tests/cpumap.c           | 19 ++++---
>> > >  tools/perf/util/cpumap.c            | 80 +++++++++++++++++++++++------
>> > >  tools/perf/util/cpumap.h            |  4 +-
>> > >  tools/perf/util/session.c           | 30 +++++------
>> > >  tools/perf/util/synthetic-events.c  | 34 +++++++-----
>> > >  6 files changed, 143 insertions(+), 60 deletions(-)
>> > >
>> > > diff --git a/tools/lib/perf/include/perf/event.h b/tools/lib/perf/include/perf/event.h
>> > > index e7758707cadd..d2d32589758a 100644
>> > > --- a/tools/lib/perf/include/perf/event.h
>> > > +++ b/tools/lib/perf/include/perf/event.h
>> > > @@ -6,6 +6,7 @@
>> > >  #include <linux/types.h>
>> > >  #include <linux/limits.h>
>> > >  #include <linux/bpf.h>
>> > > +#include <linux/compiler.h>
>> > >  #include <sys/types.h> /* pid_t */
>> > >
>> > >  #define event_contains(obj, mem) ((obj).header.size > offsetof(typeof(obj), mem))
>> > > @@ -153,20 +154,47 @@ enum {
>> > >     PERF_CPU_MAP__MASK = 1,
>> > >  };
>> > >
>> > > +/*
>> > > + * Array encoding of a perf_cpu_map where nr is the number of entries in cpu[]
>> > > + * and each entry is a value for a CPU in the map.
>> > > + */
>> > >  struct cpu_map_entries {
>> > >     __u16                    nr;
>> > >     __u16                    cpu[];
>> > >  };
>> > >
>> > > -struct perf_record_record_cpu_map {
>> > > +/* Bitmap encoding of a perf_cpu_map where bitmap entries are 32-bit. */
>> > > +struct perf_record_mask_cpu_map32 {
>> > > +   /* Number of mask values. */
>> > >     __u16                    nr;
>> > > +   /* Constant 4. */
>> > >     __u16                    long_size;
>> > > -   unsigned long            mask[];
>> > > +   /* Bitmap data. */
>> > > +   __u32                    mask[];
>> > >  };
>> > >
>> > > -struct perf_record_cpu_map_data {
>> > > +/* Bitmap encoding of a perf_cpu_map where bitmap entries are 64-bit. */
>> > > +struct perf_record_mask_cpu_map64 {
>> > > +   /* Number of mask values. */
>> > > +   __u16                    nr;
>> > > +   /* Constant 8. */
>> > > +   __u16                    long_size;
>> > > +   /* Legacy padding. */
>> > > +   char                     __pad[4];
>> > > +   /* Bitmap data. */
>> > > +   __u64                    mask[];
>> > > +};
>> > > +
>> > > +struct __packed perf_record_cpu_map_data {
>> >
>> > In various places I'm getting this:
>> >
>> > [perfbuilder@five x-riscv]$ export BUILD_TARBALL=http://192.168.86.14/perf/perf-6.0.0-rc1.tar.xz
>> > [perfbuilder@five x-riscv]$ time dm .
>> >    1     5.47 ubuntu:22.04-x-riscv64        : FAIL gcc version 11.2.0 (Ubuntu 11.2.0-16ubuntu1)
>> >     In file included from mmap.c:10:
>> >     /git/perf-6.0.0-rc1/tools/lib/perf/include/perf/event.h:190:34: error: packed attribute causes inefficient alignment for 'type' [-Werror=attributes]
>> >       190 |         __u16                    type;
>> >           |                                  ^~~~
>> >     cc1: all warnings being treated as errors
>> >     In file included from util/event.h:12,
>> >                      from builtin-diff.c:12:
>> >     /git/perf-6.0.0-rc1/tools/lib/perf/include/perf/event.h:190:34: error: packed attribute causes inefficient alignment for 'type' [-Werror=attributes]
>> >       190 |         __u16                    type;
>> >           |                                  ^~~~
>> >     In file included from util/events_stats.h:6,
>> >                      from util/evlist.h:12,
>> >                      from builtin-evlist.c:11:
>> >     /git/perf-6.0.0-rc1/tools/lib/perf/include/perf/event.h:190:34: error: packed attribute causes inefficient alignment for 'type' [-Werror=attributes]
>> >       190 |         __u16                    type;
>> >           |                                  ^~~~
>> >
>> > So probably we need to disable this -Werror=attributes in some
>> > architectures?
>>
>> Slapped this there:
>>
>> #pragma GCC diagnostic push
>> #pragma GCC diagnostic ignored "-Wpacked"
>> #pragma GCC diagnostic ignored "-Wattributes"
>>
>> struct __packed perf_record_cpu_map_data {
>>         __u16                    type;
>>         union {
>>                 /* Used when type == PERF_CPU_MAP__CPUS. */
>>                 struct cpu_map_entries cpus_data;
>>                 /* Used when type == PERF_CPU_MAP__MASK and long_size == 4. */
>>                 struct perf_record_mask_cpu_map32 mask32_data;
>>                 /* Used when type == PERF_CPU_MAP__MASK and long_size == 8. */
>>                 struct perf_record_mask_cpu_map64 mask64_data;
>>         };
>> };
>>
>> #pragma GCC diagnostic pop
>
>Perhaps we should also carry a comment like:
>perf_record_cpu_map_data is packed as unfortunately an earlier version
>had unaligned data and we wish to retain file format compatibility.


Thanks, I'll add it.

- Arnaldo 

Re: [PATCH v2 4/6] perf cpumap: Fix alignment for masks in event encoding

[Alexander Gordeev]

On Tue, Jun 14, 2022 at 07:33:51AM -0700, Ian Rogers wrote:

Hi Ian,

Although it is committed, still have a question.

> index e7758707cadd..d2d32589758a 100644
> --- a/tools/lib/perf/include/perf/event.h
> +++ b/tools/lib/perf/include/perf/event.h
> @@ -6,6 +6,7 @@
>  #include <linux/types.h>
>  #include <linux/limits.h>
>  #include <linux/bpf.h>
> +#include <linux/compiler.h>

Is it correct approach to include it into user-exposed headers?
AFAICT headers_install.sh strips #include <linux/compiler.h> and
compiler*.h itself do not get installed with make headers_install.

[...]

> +struct __packed perf_record_cpu_map_data {

And it is only needed to pull __packed macro, right?

Thanks!

Re: [PATCH v2 4/6] perf cpumap: Fix alignment for masks in event encoding

[Ian Rogers]

On Fri, Aug 26, 2022 at 5:57 AM Alexander Gordeev
<agordeev@linux.ibm.com> wrote:
>
> On Tue, Jun 14, 2022 at 07:33:51AM -0700, Ian Rogers wrote:
>
> Hi Ian,
>
> Although it is committed, still have a question.
>
> > index e7758707cadd..d2d32589758a 100644
> > --- a/tools/lib/perf/include/perf/event.h
> > +++ b/tools/lib/perf/include/perf/event.h
> > @@ -6,6 +6,7 @@
> >  #include <linux/types.h>
> >  #include <linux/limits.h>
> >  #include <linux/bpf.h>
> > +#include <linux/compiler.h>
>
> Is it correct approach to include it into user-exposed headers?
> AFAICT headers_install.sh strips #include <linux/compiler.h> and
> compiler*.h itself do not get installed with make headers_install.
>
> [...]
>
> > +struct __packed perf_record_cpu_map_data {
>
> And it is only needed to pull __packed macro, right?
>
> Thanks!

Hi Alexander,

I can see your point about compiler.h, it is true that it is just
being used for the __packed definition. Why don't you write your
proposed change as a patch and send it to LKML? libperf is more of an
experimental library than a stable API. We don't currently have any
build tests for things outside of the kernel tree.

Thanks,
Ian

[PATCH v2 5/6] perf events: Prefer union over variable length array

[Ian Rogers]

It is possible for casts to introduce alignment issues, prefer a union
for perf_record_event_update.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/lib/perf/include/perf/event.h | 11 ++++++++++-
 tools/perf/tests/event_update.c     | 14 ++++----------
 tools/perf/util/header.c            | 24 ++++++++----------------
 tools/perf/util/synthetic-events.c  | 12 +++++-------
 4 files changed, 27 insertions(+), 34 deletions(-)

diff --git a/tools/lib/perf/include/perf/event.h b/tools/lib/perf/include/perf/event.h
index d2d32589758a..21170f5afb61 100644
--- a/tools/lib/perf/include/perf/event.h
+++ b/tools/lib/perf/include/perf/event.h
@@ -221,7 +221,16 @@ struct perf_record_event_update {
 	struct perf_event_header header;
 	__u64			 type;
 	__u64			 id;
-	char			 data[];
+	union {
+		/* Used when type == PERF_EVENT_UPDATE__SCALE. */
+		struct perf_record_event_update_scale scale;
+		/* Used when type == PERF_EVENT_UPDATE__UNIT. */
+		char unit[0];
+		/* Used when type == PERF_EVENT_UPDATE__NAME. */
+		char name[0];
+		/* Used when type == PERF_EVENT_UPDATE__CPUS. */
+		struct perf_record_event_update_cpus cpus;
+	};
 };
 
 #define MAX_EVENT_NAME 64
diff --git a/tools/perf/tests/event_update.c b/tools/perf/tests/event_update.c
index 78db4d704e76..d093a9b878d1 100644
--- a/tools/perf/tests/event_update.c
+++ b/tools/perf/tests/event_update.c
@@ -21,7 +21,7 @@ static int process_event_unit(struct perf_tool *tool __maybe_unused,
 
 	TEST_ASSERT_VAL("wrong id", ev->id == 123);
 	TEST_ASSERT_VAL("wrong id", ev->type == PERF_EVENT_UPDATE__UNIT);
-	TEST_ASSERT_VAL("wrong unit", !strcmp(ev->data, "KRAVA"));
+	TEST_ASSERT_VAL("wrong unit", !strcmp(ev->unit, "KRAVA"));
 	return 0;
 }
 
@@ -31,13 +31,10 @@ static int process_event_scale(struct perf_tool *tool __maybe_unused,
 			       struct machine *machine __maybe_unused)
 {
 	struct perf_record_event_update *ev = (struct perf_record_event_update *)event;
-	struct perf_record_event_update_scale *ev_data;
-
-	ev_data = (struct perf_record_event_update_scale *)ev->data;
 
 	TEST_ASSERT_VAL("wrong id", ev->id == 123);
 	TEST_ASSERT_VAL("wrong id", ev->type == PERF_EVENT_UPDATE__SCALE);
-	TEST_ASSERT_VAL("wrong scale", ev_data->scale == 0.123);
+	TEST_ASSERT_VAL("wrong scale", ev->scale.scale == 0.123);
 	return 0;
 }
 
@@ -56,7 +53,7 @@ static int process_event_name(struct perf_tool *tool,
 
 	TEST_ASSERT_VAL("wrong id", ev->id == 123);
 	TEST_ASSERT_VAL("wrong id", ev->type == PERF_EVENT_UPDATE__NAME);
-	TEST_ASSERT_VAL("wrong name", !strcmp(ev->data, tmp->name));
+	TEST_ASSERT_VAL("wrong name", !strcmp(ev->name, tmp->name));
 	return 0;
 }
 
@@ -66,12 +63,9 @@ static int process_event_cpus(struct perf_tool *tool __maybe_unused,
 			      struct machine *machine __maybe_unused)
 {
 	struct perf_record_event_update *ev = (struct perf_record_event_update *)event;
-	struct perf_record_event_update_cpus *ev_data;
 	struct perf_cpu_map *map;
 
-	ev_data = (struct perf_record_event_update_cpus *) ev->data;
-
-	map = cpu_map__new_data(&ev_data->cpus);
+	map = cpu_map__new_data(&ev->cpus.cpus);
 
 	TEST_ASSERT_VAL("wrong id", ev->id == 123);
 	TEST_ASSERT_VAL("wrong type", ev->type == PERF_EVENT_UPDATE__CPUS);
diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
index 53332da100e8..108c8da5d2e3 100644
--- a/tools/perf/util/header.c
+++ b/tools/perf/util/header.c
@@ -4264,8 +4264,6 @@ int perf_event__process_feature(struct perf_session *session,
 size_t perf_event__fprintf_event_update(union perf_event *event, FILE *fp)
 {
 	struct perf_record_event_update *ev = &event->event_update;
-	struct perf_record_event_update_scale *ev_scale;
-	struct perf_record_event_update_cpus *ev_cpus;
 	struct perf_cpu_map *map;
 	size_t ret;
 
@@ -4273,20 +4271,18 @@ size_t perf_event__fprintf_event_update(union perf_event *event, FILE *fp)
 
 	switch (ev->type) {
 	case PERF_EVENT_UPDATE__SCALE:
-		ev_scale = (struct perf_record_event_update_scale *)ev->data;
-		ret += fprintf(fp, "... scale: %f\n", ev_scale->scale);
+		ret += fprintf(fp, "... scale: %f\n", ev->scale.scale);
 		break;
 	case PERF_EVENT_UPDATE__UNIT:
-		ret += fprintf(fp, "... unit:  %s\n", ev->data);
+		ret += fprintf(fp, "... unit:  %s\n", ev->unit);
 		break;
 	case PERF_EVENT_UPDATE__NAME:
-		ret += fprintf(fp, "... name:  %s\n", ev->data);
+		ret += fprintf(fp, "... name:  %s\n", ev->name);
 		break;
 	case PERF_EVENT_UPDATE__CPUS:
-		ev_cpus = (struct perf_record_event_update_cpus *)ev->data;
 		ret += fprintf(fp, "... ");
 
-		map = cpu_map__new_data(&ev_cpus->cpus);
+		map = cpu_map__new_data(&ev->cpus.cpus);
 		if (map)
 			ret += cpu_map__fprintf(map, fp);
 		else
@@ -4343,8 +4339,6 @@ int perf_event__process_event_update(struct perf_tool *tool __maybe_unused,
 				     struct evlist **pevlist)
 {
 	struct perf_record_event_update *ev = &event->event_update;
-	struct perf_record_event_update_scale *ev_scale;
-	struct perf_record_event_update_cpus *ev_cpus;
 	struct evlist *evlist;
 	struct evsel *evsel;
 	struct perf_cpu_map *map;
@@ -4361,19 +4355,17 @@ int perf_event__process_event_update(struct perf_tool *tool __maybe_unused,
 	switch (ev->type) {
 	case PERF_EVENT_UPDATE__UNIT:
 		free((char *)evsel->unit);
-		evsel->unit = strdup(ev->data);
+		evsel->unit = strdup(ev->unit);
 		break;
 	case PERF_EVENT_UPDATE__NAME:
 		free(evsel->name);
-		evsel->name = strdup(ev->data);
+		evsel->name = strdup(ev->name);
 		break;
 	case PERF_EVENT_UPDATE__SCALE:
-		ev_scale = (struct perf_record_event_update_scale *)ev->data;
-		evsel->scale = ev_scale->scale;
+		evsel->scale = ev->scale.scale;
 		break;
 	case PERF_EVENT_UPDATE__CPUS:
-		ev_cpus = (struct perf_record_event_update_cpus *)ev->data;
-		map = cpu_map__new_data(&ev_cpus->cpus);
+		map = cpu_map__new_data(&ev->cpus.cpus);
 		if (map) {
 			perf_cpu_map__put(evsel->core.own_cpus);
 			evsel->core.own_cpus = map;
diff --git a/tools/perf/util/synthetic-events.c b/tools/perf/util/synthetic-events.c
index 4fa7d0d7dbcf..ec54ac1ed96f 100644
--- a/tools/perf/util/synthetic-events.c
+++ b/tools/perf/util/synthetic-events.c
@@ -1848,7 +1848,7 @@ int perf_event__synthesize_event_update_unit(struct perf_tool *tool, struct evse
 	if (ev == NULL)
 		return -ENOMEM;
 
-	strlcpy(ev->data, evsel->unit, size + 1);
+	strlcpy(ev->unit, evsel->unit, size + 1);
 	err = process(tool, (union perf_event *)ev, NULL, NULL);
 	free(ev);
 	return err;
@@ -1865,8 +1865,7 @@ int perf_event__synthesize_event_update_scale(struct perf_tool *tool, struct evs
 	if (ev == NULL)
 		return -ENOMEM;
 
-	ev_data = (struct perf_record_event_update_scale *)ev->data;
-	ev_data->scale = evsel->scale;
+	ev->scale.scale = evsel->scale;
 	err = process(tool, (union perf_event *)ev, NULL, NULL);
 	free(ev);
 	return err;
@@ -1883,7 +1882,7 @@ int perf_event__synthesize_event_update_name(struct perf_tool *tool, struct evse
 	if (ev == NULL)
 		return -ENOMEM;
 
-	strlcpy(ev->data, evsel->name, len + 1);
+	strlcpy(ev->name, evsel->name, len + 1);
 	err = process(tool, (union perf_event *)ev, NULL, NULL);
 	free(ev);
 	return err;
@@ -1892,7 +1891,7 @@ int perf_event__synthesize_event_update_name(struct perf_tool *tool, struct evse
 int perf_event__synthesize_event_update_cpus(struct perf_tool *tool, struct evsel *evsel,
 					     perf_event__handler_t process)
 {
-	size_t size = sizeof(struct perf_record_event_update);
+	size_t size = sizeof(struct perf_event_header) + sizeof(u64) + sizeof(u64);
 	struct perf_record_event_update *ev;
 	int max, err;
 	u16 type;
@@ -1909,8 +1908,7 @@ int perf_event__synthesize_event_update_cpus(struct perf_tool *tool, struct evse
 	ev->type	= PERF_EVENT_UPDATE__CPUS;
 	ev->id		= evsel->core.id[0];
 
-	cpu_map_data__synthesize((struct perf_record_cpu_map_data *)ev->data,
-				 evsel->core.own_cpus, type, max);
+	cpu_map_data__synthesize(&ev->cpus.cpus, evsel->core.own_cpus, type, max);
 
 	err = process(tool, (union perf_event *)ev, NULL, NULL);
 	free(ev);
-- 
2.36.1.476.g0c4daa206d-goog

[PATCH v2 6/6] perf cpumap: Add range data encoding

[Ian Rogers]

Often cpumaps encode a range of all CPUs, add a compact encoding that
doesn't require a bit mask or list of all CPUs.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/lib/perf/include/perf/event.h |  14 +++
 tools/perf/tests/cpumap.c           |  52 ++++++++--
 tools/perf/util/cpumap.c            |  31 +++++-
 tools/perf/util/session.c           |   5 +
 tools/perf/util/synthetic-events.c  | 151 ++++++++++++++--------------
 5 files changed, 166 insertions(+), 87 deletions(-)

diff --git a/tools/lib/perf/include/perf/event.h b/tools/lib/perf/include/perf/event.h
index 21170f5afb61..43f990b8c58b 100644
--- a/tools/lib/perf/include/perf/event.h
+++ b/tools/lib/perf/include/perf/event.h
@@ -152,6 +152,7 @@ struct perf_record_header_attr {
 enum {
 	PERF_CPU_MAP__CPUS = 0,
 	PERF_CPU_MAP__MASK = 1,
+	PERF_CPU_MAP__RANGE_CPUS = 2,
 };
 
 /*
@@ -185,6 +186,17 @@ struct perf_record_mask_cpu_map64 {
 	__u64			 mask[];
 };
 
+/*
+ * An encoding of a CPU map for a range starting at start_cpu through to
+ * end_cpu. If any_cpu is 1, an any CPU (-1) value (aka dummy value) is present.
+ */
+struct perf_record_range_cpu_map {
+	__u8 any_cpu;
+	__u8 __pad;
+	__u16 start_cpu;
+	__u16 end_cpu;
+};
+
 struct __packed perf_record_cpu_map_data {
 	__u16			 type;
 	union {
@@ -194,6 +206,8 @@ struct __packed perf_record_cpu_map_data {
 		struct perf_record_mask_cpu_map32 mask32_data;
 		/* Used when type == PERF_CPU_MAP__MASK and long_size == 8. */
 		struct perf_record_mask_cpu_map64 mask64_data;
+		/* Used when type == PERF_CPU_MAP__RANGE_CPUS. */
+		struct perf_record_range_cpu_map range_cpu_data;
 	};
 };
 
diff --git a/tools/perf/tests/cpumap.c b/tools/perf/tests/cpumap.c
index 7ea150cdc137..7c873c6ae3eb 100644
--- a/tools/perf/tests/cpumap.c
+++ b/tools/perf/tests/cpumap.c
@@ -19,7 +19,6 @@ static int process_event_mask(struct perf_tool *tool __maybe_unused,
 	struct perf_record_cpu_map *map_event = &event->cpu_map;
 	struct perf_record_cpu_map_data *data;
 	struct perf_cpu_map *map;
-	int i;
 	unsigned int long_size;
 
 	data = &map_event->data;
@@ -32,16 +31,17 @@ static int process_event_mask(struct perf_tool *tool __maybe_unused,
 
 	TEST_ASSERT_VAL("wrong nr",   data->mask32_data.nr == 1);
 
-	for (i = 0; i < 20; i++) {
+	TEST_ASSERT_VAL("wrong cpu", perf_record_cpu_map_data__test_bit(0, data));
+	TEST_ASSERT_VAL("wrong cpu", !perf_record_cpu_map_data__test_bit(1, data));
+	for (int i = 2; i <= 20; i++)
 		TEST_ASSERT_VAL("wrong cpu", perf_record_cpu_map_data__test_bit(i, data));
-	}
 
 	map = cpu_map__new_data(data);
 	TEST_ASSERT_VAL("wrong nr",  perf_cpu_map__nr(map) == 20);
 
-	for (i = 0; i < 20; i++) {
-		TEST_ASSERT_VAL("wrong cpu", perf_cpu_map__cpu(map, i).cpu == i);
-	}
+	TEST_ASSERT_VAL("wrong cpu", perf_cpu_map__cpu(map, 0).cpu == 0);
+	for (int i = 2; i <= 20; i++)
+		TEST_ASSERT_VAL("wrong cpu", perf_cpu_map__cpu(map, i - 1).cpu == i);
 
 	perf_cpu_map__put(map);
 	return 0;
@@ -73,25 +73,59 @@ static int process_event_cpus(struct perf_tool *tool __maybe_unused,
 	return 0;
 }
 
+static int process_event_range_cpus(struct perf_tool *tool __maybe_unused,
+				union perf_event *event,
+				struct perf_sample *sample __maybe_unused,
+				struct machine *machine __maybe_unused)
+{
+	struct perf_record_cpu_map *map_event = &event->cpu_map;
+	struct perf_record_cpu_map_data *data;
+	struct perf_cpu_map *map;
+
+	data = &map_event->data;
+
+	TEST_ASSERT_VAL("wrong type", data->type == PERF_CPU_MAP__RANGE_CPUS);
+
+	TEST_ASSERT_VAL("wrong any_cpu",   data->range_cpu_data.any_cpu == 0);
+	TEST_ASSERT_VAL("wrong start_cpu", data->range_cpu_data.start_cpu == 1);
+	TEST_ASSERT_VAL("wrong end_cpu",   data->range_cpu_data.end_cpu == 256);
+
+	map = cpu_map__new_data(data);
+	TEST_ASSERT_VAL("wrong nr",  perf_cpu_map__nr(map) == 256);
+	TEST_ASSERT_VAL("wrong cpu", perf_cpu_map__cpu(map, 0).cpu == 1);
+	TEST_ASSERT_VAL("wrong cpu", perf_cpu_map__max(map).cpu == 256);
+	TEST_ASSERT_VAL("wrong refcnt", refcount_read(&map->refcnt) == 1);
+	perf_cpu_map__put(map);
+	return 0;
+}
+
 
 static int test__cpu_map_synthesize(struct test_suite *test __maybe_unused, int subtest __maybe_unused)
 {
 	struct perf_cpu_map *cpus;
 
-	/* This one is better stores in mask. */
-	cpus = perf_cpu_map__new("0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19");
+	/* This one is better stored in a mask. */
+	cpus = perf_cpu_map__new("0,2-20");
 
 	TEST_ASSERT_VAL("failed to synthesize map",
 		!perf_event__synthesize_cpu_map(NULL, cpus, process_event_mask, NULL));
 
 	perf_cpu_map__put(cpus);
 
-	/* This one is better stores in cpu values. */
+	/* This one is better stored in cpu values. */
 	cpus = perf_cpu_map__new("1,256");
 
 	TEST_ASSERT_VAL("failed to synthesize map",
 		!perf_event__synthesize_cpu_map(NULL, cpus, process_event_cpus, NULL));
 
+	perf_cpu_map__put(cpus);
+
+	/* This one is better stored as a range. */
+	cpus = perf_cpu_map__new("1-256");
+
+	TEST_ASSERT_VAL("failed to synthesize map",
+		!perf_event__synthesize_cpu_map(NULL, cpus, process_event_range_cpus, NULL));
+
 	perf_cpu_map__put(cpus);
 	return 0;
 }
diff --git a/tools/perf/util/cpumap.c b/tools/perf/util/cpumap.c
index ae43fb88f444..2389bd3e19b8 100644
--- a/tools/perf/util/cpumap.c
+++ b/tools/perf/util/cpumap.c
@@ -112,12 +112,39 @@ static struct perf_cpu_map *cpu_map__from_mask(const struct perf_record_cpu_map_
 
 }
 
+static struct perf_cpu_map *cpu_map__from_range(const struct perf_record_cpu_map_data *data)
+{
+	struct perf_cpu_map *map;
+	unsigned int i = 0;
+
+	map = perf_cpu_map__empty_new(data->range_cpu_data.end_cpu -
+				data->range_cpu_data.start_cpu + 1 + data->range_cpu_data.any_cpu);
+	if (!map)
+		return NULL;
+
+	if (data->range_cpu_data.any_cpu)
+		map->map[i++].cpu = -1;
+
+	for (int cpu = data->range_cpu_data.start_cpu; cpu <= data->range_cpu_data.end_cpu;
+	     i++, cpu++)
+		map->map[i].cpu = cpu;
+
+	return map;
+}
+
 struct perf_cpu_map *cpu_map__new_data(const struct perf_record_cpu_map_data *data)
 {
-	if (data->type == PERF_CPU_MAP__CPUS)
+	switch (data->type) {
+	case PERF_CPU_MAP__CPUS:
 		return cpu_map__from_entries(data);
-	else
+	case PERF_CPU_MAP__MASK:
 		return cpu_map__from_mask(data);
+	case PERF_CPU_MAP__RANGE_CPUS:
+		return cpu_map__from_range(data);
+	default:
+		pr_err("cpu_map__new_data unknown type %d\n", data->type);
+		return NULL;
+	}
 }
 
 size_t cpu_map__fprintf(struct perf_cpu_map *map, FILE *fp)
diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
index d52a39ba48e3..0acb9de54b06 100644
--- a/tools/perf/util/session.c
+++ b/tools/perf/util/session.c
@@ -941,6 +941,11 @@ static void perf_event__cpu_map_swap(union perf_event *event,
 		default:
 			pr_err("cpu_map swap: unsupported long size\n");
 		}
+		break;
+	case PERF_CPU_MAP__RANGE_CPUS:
+		data->range_cpu_data.start_cpu = bswap_16(data->range_cpu_data.start_cpu);
+		data->range_cpu_data.end_cpu = bswap_16(data->range_cpu_data.end_cpu);
+		break;
 	default:
 		break;
 	}
diff --git a/tools/perf/util/synthetic-events.c b/tools/perf/util/synthetic-events.c
index ec54ac1ed96f..76beda3e1a10 100644
--- a/tools/perf/util/synthetic-events.c
+++ b/tools/perf/util/synthetic-events.c
@@ -1183,93 +1183,97 @@ int perf_event__synthesize_thread_map2(struct perf_tool *tool,
 	return err;
 }
 
-static void synthesize_cpus(struct perf_record_cpu_map_data *data,
-			    const struct perf_cpu_map *map)
-{
-	int i, map_nr = perf_cpu_map__nr(map);
-
-	data->cpus_data.nr = map_nr;
+struct synthesize_cpu_map_data {
+	const struct perf_cpu_map *map;
+	int nr;
+	int min_cpu;
+	int max_cpu;
+	int has_any_cpu;
+	int type;
+	size_t size;
+	struct perf_record_cpu_map_data *data;
+};
 
-	for (i = 0; i < map_nr; i++)
-		data->cpus_data.cpu[i] = perf_cpu_map__cpu(map, i).cpu;
+static void synthesize_cpus(struct synthesize_cpu_map_data *data)
+{
+	data->data->type = PERF_CPU_MAP__CPUS;
+	data->data->cpus_data.nr = data->nr;
+	for (int i = 0; i < data->nr; i++)
+		data->data->cpus_data.cpu[i] = perf_cpu_map__cpu(data->map, i).cpu;
 }
 
-static void synthesize_mask(struct perf_record_cpu_map_data *data,
-			    const struct perf_cpu_map *map, int max)
+static void synthesize_mask(struct synthesize_cpu_map_data *data)
 {
 	int idx;
 	struct perf_cpu cpu;
 
 	/* Due to padding, the 4bytes per entry mask variant is always smaller. */
-	data->mask32_data.nr = BITS_TO_U32(max);
-	data->mask32_data.long_size = 4;
+	data->data->type = PERF_CPU_MAP__MASK;
+	data->data->mask32_data.nr = BITS_TO_U32(data->max_cpu);
+	data->data->mask32_data.long_size = 4;
 
-	perf_cpu_map__for_each_cpu(cpu, idx, map) {
+	perf_cpu_map__for_each_cpu(cpu, idx, data->map) {
 		int bit_word = cpu.cpu / 32;
-		__u32 bit_mask = 1U << (cpu.cpu & 31);
+		u32 bit_mask = 1U << (cpu.cpu & 31);
 
-		data->mask32_data.mask[bit_word] |= bit_mask;
+		data->data->mask32_data.mask[bit_word] |= bit_mask;
 	}
 }
 
-static size_t cpus_size(const struct perf_cpu_map *map)
+static void synthesize_range_cpus(struct synthesize_cpu_map_data *data)
 {
-	return sizeof(struct cpu_map_entries) + perf_cpu_map__nr(map) * sizeof(u16);
+	data->data->type = PERF_CPU_MAP__RANGE_CPUS;
+	data->data->range_cpu_data.any_cpu = data->has_any_cpu;
+	data->data->range_cpu_data.start_cpu = data->min_cpu;
+	data->data->range_cpu_data.end_cpu = data->max_cpu;
 }
 
-static size_t mask_size(const struct perf_cpu_map *map, int *max)
-{
-	*max = perf_cpu_map__max(map).cpu;
-	return sizeof(struct perf_record_mask_cpu_map32) + BITS_TO_U32(*max) * sizeof(__u32);
-}
-
-static void *cpu_map_data__alloc(const struct perf_cpu_map *map, size_t *size,
-				 u16 *type, int *max)
+static void *cpu_map_data__alloc(struct synthesize_cpu_map_data *syn_data,
+				 size_t header_size)
 {
 	size_t size_cpus, size_mask;
-	bool is_dummy = perf_cpu_map__empty(map);
 
-	/*
-	 * Both array and mask data have variable size based
-	 * on the number of cpus and their actual values.
-	 * The size of the 'struct perf_record_cpu_map_data' is:
-	 *
-	 *   array = size of 'struct cpu_map_entries' +
-	 *           number of cpus * sizeof(u64)
-	 *
-	 *   mask  = size of 'struct perf_record_record_cpu_map' +
-	 *           maximum cpu bit converted to size of longs
-	 *
-	 * and finally + the size of 'struct perf_record_cpu_map_data'.
-	 */
-	size_cpus = cpus_size(map);
-	size_mask = mask_size(map, max);
+	syn_data->nr = perf_cpu_map__nr(syn_data->map);
+	syn_data->has_any_cpu = (perf_cpu_map__cpu(syn_data->map, 0).cpu == -1) ? 1 : 0;
 
-	if (is_dummy || (size_cpus < size_mask)) {
-		*size += size_cpus;
-		*type  = PERF_CPU_MAP__CPUS;
-	} else {
-		*size += size_mask;
-		*type  = PERF_CPU_MAP__MASK;
+	syn_data->min_cpu = perf_cpu_map__cpu(syn_data->map, syn_data->has_any_cpu).cpu;
+	syn_data->max_cpu = perf_cpu_map__max(syn_data->map).cpu;
+	if (syn_data->max_cpu - syn_data->min_cpu + 1 == syn_data->nr - syn_data->has_any_cpu) {
+		/* A consecutive range of CPUs can be encoded using a range. */
+		assert(sizeof(u16) + sizeof(struct perf_record_range_cpu_map) == sizeof(u64));
+		syn_data->type = PERF_CPU_MAP__RANGE_CPUS;
+		syn_data->size = header_size + sizeof(u64);
+		return zalloc(syn_data->size);
 	}
 
-	*size += sizeof(__u16); /* For perf_record_cpu_map_data.type. */
-	*size = PERF_ALIGN(*size, sizeof(u64));
-	return zalloc(*size);
+	size_cpus = sizeof(u16) + sizeof(struct cpu_map_entries) + syn_data->nr * sizeof(u16);
+	/* Due to padding, the 4bytes per entry mask variant is always smaller. */
+	size_mask = sizeof(u16) + sizeof(struct perf_record_mask_cpu_map32) +
+		BITS_TO_U32(syn_data->max_cpu) * sizeof(__u32);
+	if (syn_data->has_any_cpu || size_cpus < size_mask) {
+		/* Follow the CPU map encoding. */
+		syn_data->type = PERF_CPU_MAP__CPUS;
+		syn_data->size = header_size + PERF_ALIGN(size_cpus, sizeof(u64));
+		return zalloc(syn_data->size);
+	}
+	/* Encode using a bitmask. */
+	syn_data->type = PERF_CPU_MAP__MASK;
+	syn_data->size = header_size + PERF_ALIGN(size_mask, sizeof(u64));
+	return zalloc(syn_data->size);
 }
 
-static void cpu_map_data__synthesize(struct perf_record_cpu_map_data *data,
-				     const struct perf_cpu_map *map,
-				     u16 type, int max)
+static void cpu_map_data__synthesize(struct synthesize_cpu_map_data *data)
 {
-	data->type = type;
-
-	switch (type) {
+	switch (data->type) {
 	case PERF_CPU_MAP__CPUS:
-		synthesize_cpus(data, map);
+		synthesize_cpus(data);
 		break;
 	case PERF_CPU_MAP__MASK:
-		synthesize_mask(data, map, max);
+		synthesize_mask(data);
+		break;
+	case PERF_CPU_MAP__RANGE_CPUS:
+		synthesize_range_cpus(data);
+		break;
 	default:
 		break;
 	}
@@ -1277,23 +1281,22 @@ static void cpu_map_data__synthesize(struct perf_record_cpu_map_data *data,
 
 static struct perf_record_cpu_map *cpu_map_event__new(const struct perf_cpu_map *map)
 {
-	size_t size = sizeof(struct perf_event_header);
+	struct synthesize_cpu_map_data syn_data = { .map = map };
 	struct perf_record_cpu_map *event;
-	int max;
-	u16 type;
 
-	event = cpu_map_data__alloc(map, &size, &type, &max);
+
+	event = cpu_map_data__alloc(&syn_data, sizeof(struct perf_event_header));
 	if (!event)
 		return NULL;
 
+	syn_data.data = &event->data;
 	event->header.type = PERF_RECORD_CPU_MAP;
-	event->header.size = size;
-	event->data.type   = type;
-
-	cpu_map_data__synthesize(&event->data, map, type, max);
+	event->header.size = syn_data.size;
+	cpu_map_data__synthesize(&syn_data);
 	return event;
 }
 
+
 int perf_event__synthesize_cpu_map(struct perf_tool *tool,
 				   const struct perf_cpu_map *map,
 				   perf_event__handler_t process,
@@ -1891,24 +1894,20 @@ int perf_event__synthesize_event_update_name(struct perf_tool *tool, struct evse
 int perf_event__synthesize_event_update_cpus(struct perf_tool *tool, struct evsel *evsel,
 					     perf_event__handler_t process)
 {
-	size_t size = sizeof(struct perf_event_header) + sizeof(u64) + sizeof(u64);
+	struct synthesize_cpu_map_data syn_data = { .map = evsel->core.own_cpus };
 	struct perf_record_event_update *ev;
-	int max, err;
-	u16 type;
-
-	if (!evsel->core.own_cpus)
-		return 0;
+	int err;
 
-	ev = cpu_map_data__alloc(evsel->core.own_cpus, &size, &type, &max);
+	ev = cpu_map_data__alloc(&syn_data, sizeof(struct perf_event_header) + 2 * sizeof(u64));
 	if (!ev)
 		return -ENOMEM;
 
+	syn_data.data = &ev->cpus.cpus;
 	ev->header.type = PERF_RECORD_EVENT_UPDATE;
-	ev->header.size = (u16)size;
+	ev->header.size = (u16)syn_data.size;
 	ev->type	= PERF_EVENT_UPDATE__CPUS;
 	ev->id		= evsel->core.id[0];
-
-	cpu_map_data__synthesize(&ev->cpus.cpus, evsel->core.own_cpus, type, max);
+	cpu_map_data__synthesize(&syn_data);
 
 	err = process(tool, (union perf_event *)ev, NULL, NULL);
 	free(ev);
-- 
2.36.1.476.g0c4daa206d-goog

Re: [PATCH v2 6/6] perf cpumap: Add range data encoding

[Jiri Olsa]

On Tue, Jun 14, 2022 at 07:33:53AM -0700, Ian Rogers wrote:

SNIP

> -static size_t cpus_size(const struct perf_cpu_map *map)
> +static void synthesize_range_cpus(struct synthesize_cpu_map_data *data)
>  {
> -	return sizeof(struct cpu_map_entries) + perf_cpu_map__nr(map) * sizeof(u16);
> +	data->data->type = PERF_CPU_MAP__RANGE_CPUS;
> +	data->data->range_cpu_data.any_cpu = data->has_any_cpu;
> +	data->data->range_cpu_data.start_cpu = data->min_cpu;
> +	data->data->range_cpu_data.end_cpu = data->max_cpu;
>  }
>  
> -static size_t mask_size(const struct perf_cpu_map *map, int *max)
> -{
> -	*max = perf_cpu_map__max(map).cpu;
> -	return sizeof(struct perf_record_mask_cpu_map32) + BITS_TO_U32(*max) * sizeof(__u32);
> -}
> -
> -static void *cpu_map_data__alloc(const struct perf_cpu_map *map, size_t *size,
> -				 u16 *type, int *max)
> +static void *cpu_map_data__alloc(struct synthesize_cpu_map_data *syn_data,
> +				 size_t header_size)
>  {
>  	size_t size_cpus, size_mask;
> -	bool is_dummy = perf_cpu_map__empty(map);
>  
> -	/*
> -	 * Both array and mask data have variable size based
> -	 * on the number of cpus and their actual values.
> -	 * The size of the 'struct perf_record_cpu_map_data' is:
> -	 *
> -	 *   array = size of 'struct cpu_map_entries' +
> -	 *           number of cpus * sizeof(u64)
> -	 *
> -	 *   mask  = size of 'struct perf_record_record_cpu_map' +
> -	 *           maximum cpu bit converted to size of longs
> -	 *
> -	 * and finally + the size of 'struct perf_record_cpu_map_data'.
> -	 */
> -	size_cpus = cpus_size(map);
> -	size_mask = mask_size(map, max);
> +	syn_data->nr = perf_cpu_map__nr(syn_data->map);
> +	syn_data->has_any_cpu = (perf_cpu_map__cpu(syn_data->map, 0).cpu == -1) ? 1 : 0;

I'm bit lost in the logic in here.. should it be '.cpu != -1' ?
has_any_cpu is named as bool but used as index below ;-)

could you please keep/update the comment above and explain
the conditions when each cpu_map type is taken

thanks,
jirka

>  
> -	if (is_dummy || (size_cpus < size_mask)) {
> -		*size += size_cpus;
> -		*type  = PERF_CPU_MAP__CPUS;
> -	} else {
> -		*size += size_mask;
> -		*type  = PERF_CPU_MAP__MASK;
> +	syn_data->min_cpu = perf_cpu_map__cpu(syn_data->map, syn_data->has_any_cpu).cpu;
> +	syn_data->max_cpu = perf_cpu_map__max(syn_data->map).cpu;
> +	if (syn_data->max_cpu - syn_data->min_cpu + 1 == syn_data->nr - syn_data->has_any_cpu) {
> +		/* A consecutive range of CPUs can be encoded using a range. */
> +		assert(sizeof(u16) + sizeof(struct perf_record_range_cpu_map) == sizeof(u64));
> +		syn_data->type = PERF_CPU_MAP__RANGE_CPUS;
> +		syn_data->size = header_size + sizeof(u64);
> +		return zalloc(syn_data->size);
>  	}
>  
> -	*size += sizeof(__u16); /* For perf_record_cpu_map_data.type. */
> -	*size = PERF_ALIGN(*size, sizeof(u64));
> -	return zalloc(*size);
> +	size_cpus = sizeof(u16) + sizeof(struct cpu_map_entries) + syn_data->nr * sizeof(u16);
> +	/* Due to padding, the 4bytes per entry mask variant is always smaller. */
> +	size_mask = sizeof(u16) + sizeof(struct perf_record_mask_cpu_map32) +
> +		BITS_TO_U32(syn_data->max_cpu) * sizeof(__u32);
> +	if (syn_data->has_any_cpu || size_cpus < size_mask) {
> +		/* Follow the CPU map encoding. */
> +		syn_data->type = PERF_CPU_MAP__CPUS;
> +		syn_data->size = header_size + PERF_ALIGN(size_cpus, sizeof(u64));
> +		return zalloc(syn_data->size);
> +	}
> +	/* Encode using a bitmask. */
> +	syn_data->type = PERF_CPU_MAP__MASK;
> +	syn_data->size = header_size + PERF_ALIGN(size_mask, sizeof(u64));
> +	return zalloc(syn_data->size);

SNIP

Re: [PATCH v2 6/6] perf cpumap: Add range data encoding

[Ian Rogers]

On Wed, Jun 29, 2022 at 2:31 AM Jiri Olsa <olsajiri@gmail.com> wrote:
>
> On Tue, Jun 14, 2022 at 07:33:53AM -0700, Ian Rogers wrote:
>
> SNIP
>
> > -static size_t cpus_size(const struct perf_cpu_map *map)
> > +static void synthesize_range_cpus(struct synthesize_cpu_map_data *data)
> >  {
> > -     return sizeof(struct cpu_map_entries) + perf_cpu_map__nr(map) * sizeof(u16);
> > +     data->data->type = PERF_CPU_MAP__RANGE_CPUS;
> > +     data->data->range_cpu_data.any_cpu = data->has_any_cpu;
> > +     data->data->range_cpu_data.start_cpu = data->min_cpu;
> > +     data->data->range_cpu_data.end_cpu = data->max_cpu;
> >  }
> >
> > -static size_t mask_size(const struct perf_cpu_map *map, int *max)
> > -{
> > -     *max = perf_cpu_map__max(map).cpu;
> > -     return sizeof(struct perf_record_mask_cpu_map32) + BITS_TO_U32(*max) * sizeof(__u32);
> > -}
> > -
> > -static void *cpu_map_data__alloc(const struct perf_cpu_map *map, size_t *size,
> > -                              u16 *type, int *max)
> > +static void *cpu_map_data__alloc(struct synthesize_cpu_map_data *syn_data,
> > +                              size_t header_size)
> >  {
> >       size_t size_cpus, size_mask;
> > -     bool is_dummy = perf_cpu_map__empty(map);
> >
> > -     /*
> > -      * Both array and mask data have variable size based
> > -      * on the number of cpus and their actual values.
> > -      * The size of the 'struct perf_record_cpu_map_data' is:
> > -      *
> > -      *   array = size of 'struct cpu_map_entries' +
> > -      *           number of cpus * sizeof(u64)
> > -      *
> > -      *   mask  = size of 'struct perf_record_record_cpu_map' +
> > -      *           maximum cpu bit converted to size of longs
> > -      *
> > -      * and finally + the size of 'struct perf_record_cpu_map_data'.
> > -      */
> > -     size_cpus = cpus_size(map);
> > -     size_mask = mask_size(map, max);
> > +     syn_data->nr = perf_cpu_map__nr(syn_data->map);
> > +     syn_data->has_any_cpu = (perf_cpu_map__cpu(syn_data->map, 0).cpu == -1) ? 1 : 0;
>
> I'm bit lost in the logic in here.. should it be '.cpu != -1' ?
> has_any_cpu is named as bool but used as index below ;-)
>
> could you please keep/update the comment above and explain
> the conditions when each cpu_map type is taken

So this relates to the CPU map "empty" problem that I've been moaning
about for a time with Adrian, Arnaldo, etc. The CPU map can contain -1
for the "any CPU" case in perf_event_open, it can also contain CPU
values after this as the result of a merge. Having only -1 in the CPU
map is referred to as a dummy (presumably because the dummy event uses
it), what does it mean if you have the -1 and CPU values? Things get
really messy when you look at what the definition of empty is. Here
I've used the term from perf_event_open that -1 means this can be on
any CPU. As the CPU map data is sorted we can take advantage that the
"any CPU" value always comes first. If we have it we set the bit here.
We also use the bit below so that we ignore the first array element if
it has -1 in it. As the -1 adjusts the CPU map size, there are some
other adjustments as well. Hopefully this makes things clearer and I
want to rewrite parts of the CPU map API to make them clearer too -
empty should mean == NULL or nr == 0, dummy is more confusing to me
than "any CPU", etc.

Thanks,
Ian

> thanks,
> jirka
>
> >
> > -     if (is_dummy || (size_cpus < size_mask)) {
> > -             *size += size_cpus;
> > -             *type  = PERF_CPU_MAP__CPUS;
> > -     } else {
> > -             *size += size_mask;
> > -             *type  = PERF_CPU_MAP__MASK;
> > +     syn_data->min_cpu = perf_cpu_map__cpu(syn_data->map, syn_data->has_any_cpu).cpu;
> > +     syn_data->max_cpu = perf_cpu_map__max(syn_data->map).cpu;
> > +     if (syn_data->max_cpu - syn_data->min_cpu + 1 == syn_data->nr - syn_data->has_any_cpu) {
> > +             /* A consecutive range of CPUs can be encoded using a range. */
> > +             assert(sizeof(u16) + sizeof(struct perf_record_range_cpu_map) == sizeof(u64));
> > +             syn_data->type = PERF_CPU_MAP__RANGE_CPUS;
> > +             syn_data->size = header_size + sizeof(u64);
> > +             return zalloc(syn_data->size);
> >       }
> >
> > -     *size += sizeof(__u16); /* For perf_record_cpu_map_data.type. */
> > -     *size = PERF_ALIGN(*size, sizeof(u64));
> > -     return zalloc(*size);
> > +     size_cpus = sizeof(u16) + sizeof(struct cpu_map_entries) + syn_data->nr * sizeof(u16);
> > +     /* Due to padding, the 4bytes per entry mask variant is always smaller. */
> > +     size_mask = sizeof(u16) + sizeof(struct perf_record_mask_cpu_map32) +
> > +             BITS_TO_U32(syn_data->max_cpu) * sizeof(__u32);
> > +     if (syn_data->has_any_cpu || size_cpus < size_mask) {
> > +             /* Follow the CPU map encoding. */
> > +             syn_data->type = PERF_CPU_MAP__CPUS;
> > +             syn_data->size = header_size + PERF_ALIGN(size_cpus, sizeof(u64));
> > +             return zalloc(syn_data->size);
> > +     }
> > +     /* Encode using a bitmask. */
> > +     syn_data->type = PERF_CPU_MAP__MASK;
> > +     syn_data->size = header_size + PERF_ALIGN(size_mask, sizeof(u64));
> > +     return zalloc(syn_data->size);
>
> SNIP

Re: [PATCH v2 6/6] perf cpumap: Add range data encoding

[Jiri Olsa]

On Tue, Jun 14, 2022 at 07:33:53AM -0700, Ian Rogers wrote:

SNIP

> +	event = cpu_map_data__alloc(&syn_data, sizeof(struct perf_event_header));
>  	if (!event)
>  		return NULL;
>  
> +	syn_data.data = &event->data;
>  	event->header.type = PERF_RECORD_CPU_MAP;
> -	event->header.size = size;
> -	event->data.type   = type;
> -
> -	cpu_map_data__synthesize(&event->data, map, type, max);
> +	event->header.size = syn_data.size;
> +	cpu_map_data__synthesize(&syn_data);
>  	return event;
>  }
>  
> +
>  int perf_event__synthesize_cpu_map(struct perf_tool *tool,
>  				   const struct perf_cpu_map *map,
>  				   perf_event__handler_t process,
> @@ -1891,24 +1894,20 @@ int perf_event__synthesize_event_update_name(struct perf_tool *tool, struct evse
>  int perf_event__synthesize_event_update_cpus(struct perf_tool *tool, struct evsel *evsel,
>  					     perf_event__handler_t process)
>  {
> -	size_t size = sizeof(struct perf_event_header) + sizeof(u64) + sizeof(u64);
> +	struct synthesize_cpu_map_data syn_data = { .map = evsel->core.own_cpus };
>  	struct perf_record_event_update *ev;
> -	int max, err;
> -	u16 type;
> -
> -	if (!evsel->core.own_cpus)
> -		return 0;

all seems fine, just looks like we no longer do this check,
might not be needed anymore, as that changed in past

thanks,
jirka

> +	int err;
>  
> -	ev = cpu_map_data__alloc(evsel->core.own_cpus, &size, &type, &max);
> +	ev = cpu_map_data__alloc(&syn_data, sizeof(struct perf_event_header) + 2 * sizeof(u64));
>  	if (!ev)
>  		return -ENOMEM;
>  
> +	syn_data.data = &ev->cpus.cpus;
>  	ev->header.type = PERF_RECORD_EVENT_UPDATE;
> -	ev->header.size = (u16)size;
> +	ev->header.size = (u16)syn_data.size;
>  	ev->type	= PERF_EVENT_UPDATE__CPUS;
>  	ev->id		= evsel->core.id[0];
> -
> -	cpu_map_data__synthesize(&ev->cpus.cpus, evsel->core.own_cpus, type, max);
> +	cpu_map_data__synthesize(&syn_data);
>  
>  	err = process(tool, (union perf_event *)ev, NULL, NULL);
>  	free(ev);
> -- 
> 2.36.1.476.g0c4daa206d-goog
> 

Re: [PATCH v2 6/6] perf cpumap: Add range data encoding

[Ian Rogers]

On Sun, Jul 31, 2022 at 5:39 AM Jiri Olsa <olsajiri@gmail.com> wrote:
>
> On Tue, Jun 14, 2022 at 07:33:53AM -0700, Ian Rogers wrote:
>
> SNIP
>
> > +     event = cpu_map_data__alloc(&syn_data, sizeof(struct perf_event_header));
> >       if (!event)
> >               return NULL;
> >
> > +     syn_data.data = &event->data;
> >       event->header.type = PERF_RECORD_CPU_MAP;
> > -     event->header.size = size;
> > -     event->data.type   = type;
> > -
> > -     cpu_map_data__synthesize(&event->data, map, type, max);
> > +     event->header.size = syn_data.size;
> > +     cpu_map_data__synthesize(&syn_data);
> >       return event;
> >  }
> >
> > +
> >  int perf_event__synthesize_cpu_map(struct perf_tool *tool,
> >                                  const struct perf_cpu_map *map,
> >                                  perf_event__handler_t process,
> > @@ -1891,24 +1894,20 @@ int perf_event__synthesize_event_update_name(struct perf_tool *tool, struct evse
> >  int perf_event__synthesize_event_update_cpus(struct perf_tool *tool, struct evsel *evsel,
> >                                            perf_event__handler_t process)
> >  {
> > -     size_t size = sizeof(struct perf_event_header) + sizeof(u64) + sizeof(u64);
> > +     struct synthesize_cpu_map_data syn_data = { .map = evsel->core.own_cpus };
> >       struct perf_record_event_update *ev;
> > -     int max, err;
> > -     u16 type;
> > -
> > -     if (!evsel->core.own_cpus)
> > -             return 0;
>
> all seems fine, just looks like we no longer do this check,
> might not be needed anymore, as that changed in past

This function is called in a test and in this file. The caller already
does this test and so the check is redundant plus a little confusing:
https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/tree/tools/perf/util/synthetic-events.c?h=perf/core#n2062
As you say, it wasn't needed any more and so I removed it.

Thanks,
Ian

> thanks,
> jirka
>
> > +     int err;
> >
> > -     ev = cpu_map_data__alloc(evsel->core.own_cpus, &size, &type, &max);
> > +     ev = cpu_map_data__alloc(&syn_data, sizeof(struct perf_event_header) + 2 * sizeof(u64));
> >       if (!ev)
> >               return -ENOMEM;
> >
> > +     syn_data.data = &ev->cpus.cpus;
> >       ev->header.type = PERF_RECORD_EVENT_UPDATE;
> > -     ev->header.size = (u16)size;
> > +     ev->header.size = (u16)syn_data.size;
> >       ev->type        = PERF_EVENT_UPDATE__CPUS;
> >       ev->id          = evsel->core.id[0];
> > -
> > -     cpu_map_data__synthesize(&ev->cpus.cpus, evsel->core.own_cpus, type, max);
> > +     cpu_map_data__synthesize(&syn_data);
> >
> >       err = process(tool, (union perf_event *)ev, NULL, NULL);
> >       free(ev);
> > --
> > 2.36.1.476.g0c4daa206d-goog
> >

Re: [PATCH v2 6/6] perf cpumap: Add range data encoding

[Ian Rogers]

On Thu, Aug 4, 2022 at 12:30 PM Ian Rogers <irogers@google.com> wrote:
>
> On Sun, Jul 31, 2022 at 5:39 AM Jiri Olsa <olsajiri@gmail.com> wrote:
> >
> > On Tue, Jun 14, 2022 at 07:33:53AM -0700, Ian Rogers wrote:
> >
> > SNIP
> >
> > > +     event = cpu_map_data__alloc(&syn_data, sizeof(struct perf_event_header));
> > >       if (!event)
> > >               return NULL;
> > >
> > > +     syn_data.data = &event->data;
> > >       event->header.type = PERF_RECORD_CPU_MAP;
> > > -     event->header.size = size;
> > > -     event->data.type   = type;
> > > -
> > > -     cpu_map_data__synthesize(&event->data, map, type, max);
> > > +     event->header.size = syn_data.size;
> > > +     cpu_map_data__synthesize(&syn_data);
> > >       return event;
> > >  }
> > >
> > > +
> > >  int perf_event__synthesize_cpu_map(struct perf_tool *tool,
> > >                                  const struct perf_cpu_map *map,
> > >                                  perf_event__handler_t process,
> > > @@ -1891,24 +1894,20 @@ int perf_event__synthesize_event_update_name(struct perf_tool *tool, struct evse
> > >  int perf_event__synthesize_event_update_cpus(struct perf_tool *tool, struct evsel *evsel,
> > >                                            perf_event__handler_t process)
> > >  {
> > > -     size_t size = sizeof(struct perf_event_header) + sizeof(u64) + sizeof(u64);
> > > +     struct synthesize_cpu_map_data syn_data = { .map = evsel->core.own_cpus };
> > >       struct perf_record_event_update *ev;
> > > -     int max, err;
> > > -     u16 type;
> > > -
> > > -     if (!evsel->core.own_cpus)
> > > -             return 0;
> >
> > all seems fine, just looks like we no longer do this check,
> > might not be needed anymore, as that changed in past
>
> This function is called in a test and in this file. The caller already
> does this test and so the check is redundant plus a little confusing:
> https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/tree/tools/perf/util/synthetic-events.c?h=perf/core#n2062
> As you say, it wasn't needed any more and so I removed it.
>
> Thanks,
> Ian

Would be nice to land this imo. Anything outstanding?

Thanks,
Ian

> > thanks,
> > jirka
> >
> > > +     int err;
> > >
> > > -     ev = cpu_map_data__alloc(evsel->core.own_cpus, &size, &type, &max);
> > > +     ev = cpu_map_data__alloc(&syn_data, sizeof(struct perf_event_header) + 2 * sizeof(u64));
> > >       if (!ev)
> > >               return -ENOMEM;
> > >
> > > +     syn_data.data = &ev->cpus.cpus;
> > >       ev->header.type = PERF_RECORD_EVENT_UPDATE;
> > > -     ev->header.size = (u16)size;
> > > +     ev->header.size = (u16)syn_data.size;
> > >       ev->type        = PERF_EVENT_UPDATE__CPUS;
> > >       ev->id          = evsel->core.id[0];
> > > -
> > > -     cpu_map_data__synthesize(&ev->cpus.cpus, evsel->core.own_cpus, type, max);
> > > +     cpu_map_data__synthesize(&syn_data);
> > >
> > >       err = process(tool, (union perf_event *)ev, NULL, NULL);
> > >       free(ev);
> > > --
> > > 2.36.1.476.g0c4daa206d-goog
> > >

Re: [PATCH v2 6/6] perf cpumap: Add range data encoding

[Arnaldo Carvalho de Melo]

On September 7, 2022 7:41:19 PM GMT-03:00, Ian Rogers <irogers@google.com> wrote:
>On Thu, Aug 4, 2022 at 12:30 PM Ian Rogers <irogers@google.com> wrote:
>>
>> On Sun, Jul 31, 2022 at 5:39 AM Jiri Olsa <olsajiri@gmail.com> wrote:
>> >
>> > On Tue, Jun 14, 2022 at 07:33:53AM -0700, Ian Rogers wrote:
>> >
>> > SNIP
>> >
>> > > +     event = cpu_map_data__alloc(&syn_data, sizeof(struct perf_event_header));
>> > >       if (!event)
>> > >               return NULL;
>> > >
>> > > +     syn_data.data = &event->data;
>> > >       event->header.type = PERF_RECORD_CPU_MAP;
>> > > -     event->header.size = size;
>> > > -     event->data.type   = type;
>> > > -
>> > > -     cpu_map_data__synthesize(&event->data, map, type, max);
>> > > +     event->header.size = syn_data.size;
>> > > +     cpu_map_data__synthesize(&syn_data);
>> > >       return event;
>> > >  }
>> > >
>> > > +
>> > >  int perf_event__synthesize_cpu_map(struct perf_tool *tool,
>> > >                                  const struct perf_cpu_map *map,
>> > >                                  perf_event__handler_t process,
>> > > @@ -1891,24 +1894,20 @@ int perf_event__synthesize_event_update_name(struct perf_tool *tool, struct evse
>> > >  int perf_event__synthesize_event_update_cpus(struct perf_tool *tool, struct evsel *evsel,
>> > >                                            perf_event__handler_t process)
>> > >  {
>> > > -     size_t size = sizeof(struct perf_event_header) + sizeof(u64) + sizeof(u64);
>> > > +     struct synthesize_cpu_map_data syn_data = { .map = evsel->core.own_cpus };
>> > >       struct perf_record_event_update *ev;
>> > > -     int max, err;
>> > > -     u16 type;
>> > > -
>> > > -     if (!evsel->core.own_cpus)
>> > > -             return 0;
>> >
>> > all seems fine, just looks like we no longer do this check,
>> > might not be needed anymore, as that changed in past
>>
>> This function is called in a test and in this file. The caller already
>> does this test and so the check is redundant plus a little confusing:
>> https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/tree/tools/perf/util/synthetic-events.c?h=perf/core#n2062
>> As you say, it wasn't needed any more and so I removed it.
>>
>> Thanks,
>> Ian
>
>Would be nice to land this imo. Anything outstanding?

I'll check first hour tomorrow, today was a holiday down here.

- Arnaldo

>
>Thanks,
>Ian
>
>> > thanks,
>> > jirka
>> >
>> > > +     int err;
>> > >
>> > > -     ev = cpu_map_data__alloc(evsel->core.own_cpus, &size, &type, &max);
>> > > +     ev = cpu_map_data__alloc(&syn_data, sizeof(struct perf_event_header) + 2 * sizeof(u64));
>> > >       if (!ev)
>> > >               return -ENOMEM;
>> > >
>> > > +     syn_data.data = &ev->cpus.cpus;
>> > >       ev->header.type = PERF_RECORD_EVENT_UPDATE;
>> > > -     ev->header.size = (u16)size;
>> > > +     ev->header.size = (u16)syn_data.size;
>> > >       ev->type        = PERF_EVENT_UPDATE__CPUS;
>> > >       ev->id          = evsel->core.id[0];
>> > > -
>> > > -     cpu_map_data__synthesize(&ev->cpus.cpus, evsel->core.own_cpus, type, max);
>> > > +     cpu_map_data__synthesize(&syn_data);
>> > >
>> > >       err = process(tool, (union perf_event *)ev, NULL, NULL);
>> > >       free(ev);
>> > > --
>> > > 2.36.1.476.g0c4daa206d-goog
>> > >

Re: [PATCH v2 6/6] perf cpumap: Add range data encoding

[Arnaldo Carvalho de Melo]

Em Wed, Sep 07, 2022 at 03:41:19PM -0700, Ian Rogers escreveu:
> On Thu, Aug 4, 2022 at 12:30 PM Ian Rogers <irogers@google.com> wrote:
> >
> > On Sun, Jul 31, 2022 at 5:39 AM Jiri Olsa <olsajiri@gmail.com> wrote:
> > >
> > > On Tue, Jun 14, 2022 at 07:33:53AM -0700, Ian Rogers wrote:
> > >
> > > SNIP
> > >
> > > > +     event = cpu_map_data__alloc(&syn_data, sizeof(struct perf_event_header));
> > > >       if (!event)
> > > >               return NULL;
> > > >
> > > > +     syn_data.data = &event->data;
> > > >       event->header.type = PERF_RECORD_CPU_MAP;
> > > > -     event->header.size = size;
> > > > -     event->data.type   = type;
> > > > -
> > > > -     cpu_map_data__synthesize(&event->data, map, type, max);
> > > > +     event->header.size = syn_data.size;
> > > > +     cpu_map_data__synthesize(&syn_data);
> > > >       return event;
> > > >  }
> > > >
> > > > +
> > > >  int perf_event__synthesize_cpu_map(struct perf_tool *tool,
> > > >                                  const struct perf_cpu_map *map,
> > > >                                  perf_event__handler_t process,
> > > > @@ -1891,24 +1894,20 @@ int perf_event__synthesize_event_update_name(struct perf_tool *tool, struct evse
> > > >  int perf_event__synthesize_event_update_cpus(struct perf_tool *tool, struct evsel *evsel,
> > > >                                            perf_event__handler_t process)
> > > >  {
> > > > -     size_t size = sizeof(struct perf_event_header) + sizeof(u64) + sizeof(u64);
> > > > +     struct synthesize_cpu_map_data syn_data = { .map = evsel->core.own_cpus };
> > > >       struct perf_record_event_update *ev;
> > > > -     int max, err;
> > > > -     u16 type;
> > > > -
> > > > -     if (!evsel->core.own_cpus)
> > > > -             return 0;
> > >
> > > all seems fine, just looks like we no longer do this check,
> > > might not be needed anymore, as that changed in past
> >
> > This function is called in a test and in this file. The caller already
> > does this test and so the check is redundant plus a little confusing:
> > https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/tree/tools/perf/util/synthetic-events.c?h=perf/core#n2062
> > As you say, it wasn't needed any more and so I removed it.
> >
> > Thanks,
> > Ian
> 
> Would be nice to land this imo. Anything outstanding?

So the last two patches were outstanding, 5/6 applied cleanly, 6/6 had
some fuzzes, minimal stuff, applied, I'll push it out soon, please
check.

- Arnaldo

Re: [PATCH v2 0/6] Corrections to cpu map event encoding

[Ian Rogers]

On Tue, Jun 14, 2022 at 7:33 AM Ian Rogers <irogers@google.com> wrote:
>
> A mask encoding of a cpu map is laid out as:
>   u16 nr
>   u16 long_size
>   unsigned long mask[];
> However, the mask may be 8-byte aligned meaning there is a 4-byte pad
> after long_size. This means 32-bit and 64-bit builds see the mask as
> being at different offsets. On top of this the structure is in the byte
> data[] encoded as:
>   u16 type
>   char data[]
> This means the mask's struct isn't the required 4 or 8 byte aligned, but
> is offset by 2. Consequently the long reads and writes are causing
> undefined behavior as the alignment is broken.
>
> These changes do minor clean up with const, visibility of functions
> and using the constant time max function. It then adds 32 and 64-bit
> mask encoding variants, packed to match current alignment. Taking the
> address of a packed struct leads to unaligned data, so function
> arguments are altered to be passed the packed struct. To compact the
> mask encoding further and drop the padding, the 4-byte variant is
> preferred. Finally a new range encoding is added, that reduces the
> size of the common case of a range of CPUs to a single u64.
>
> On a 72 CPU (hyperthread) machine the original encoding of all CPUs is:
> 0x9a98 [0x28]: event: 74
> .
> . ... raw event: size 40 bytes
> .  0000:  4a 00 00 00 00 00 28 00 01 00 02 00 08 00 00 00  J.....(.........
> .  0010:  00 00 ff ff ff ff ff ff ff ff ff 00 00 00 00 00  ................
> .  0020:  00 00 00 00 00 00 00 00                          ........
>
> 0 0 0x9a98 [0x28]: PERF_RECORD_CPU_MAP
>
> Using the 4-byte encoding it is:
> 0x9a98@pipe [0x20]: event: 74
> .
> . ... raw event: size 32 bytes
> .  0000:  4a 00 00 00 00 00 20 00 01 00 03 00 04 00 ff ff  J..... .........
> .  0010:  ff ff ff ff ff ff ff 00 00 00 00 00 00 00 00 00  ................
>
> 0 0 0x9a98 [0x20]: PERF_RECORD_CPU_MAP
>
> Finally, with the range encoding it is:
> 0x9ab8@pipe [0x10]: event: 74
> .
> . ... raw event: size 16 bytes
> .  0000:  4a 00 00 00 00 00 10 00 02 00 00 00 00 00 47 00  J.............G.
>
> 0 0 0x9ab8 [0x10]: PERF_RECORD_CPU_MAP
>
> v2. Fixes a bug in the size computation of the update header
>     introduced by the last patch (Add range data encoding) and caught
>     by address sanitizer.
>
> Ian Rogers (6):
>   perf cpumap: Const map for max
>   perf cpumap: Synthetic events and const/static
>   perf cpumap: Compute mask size in constant time
>   perf cpumap: Fix alignment for masks in event encoding
>   perf events: Prefer union over variable length array
>   perf cpumap: Add range data encoding

Ping. There was some feedback on this change but nothing to create a
v3. Feedback/acked-by/reviewed-by appreciated.

Thanks,
Ian

>  tools/lib/perf/cpumap.c              |   2 +-
>  tools/lib/perf/include/perf/cpumap.h |   2 +-
>  tools/lib/perf/include/perf/event.h  |  61 ++++++++-
>  tools/perf/tests/cpumap.c            |  71 ++++++++---
>  tools/perf/tests/event_update.c      |  14 +--
>  tools/perf/util/cpumap.c             | 111 +++++++++++++---
>  tools/perf/util/cpumap.h             |   4 +-
>  tools/perf/util/event.h              |   4 -
>  tools/perf/util/header.c             |  24 ++--
>  tools/perf/util/session.c            |  35 +++---
>  tools/perf/util/synthetic-events.c   | 182 +++++++++++++--------------
>  tools/perf/util/synthetic-events.h   |   2 +-
>  12 files changed, 327 insertions(+), 185 deletions(-)
>
> --
> 2.36.1.476.g0c4daa206d-goog
>

Re: [PATCH v2 0/6] Corrections to cpu map event encoding

[Ian Rogers]

On Fri, Jul 15, 2022 at 10:01 AM Ian Rogers <irogers@google.com> wrote:
>
> On Tue, Jun 14, 2022 at 7:33 AM Ian Rogers <irogers@google.com> wrote:
> >
> > A mask encoding of a cpu map is laid out as:
> >   u16 nr
> >   u16 long_size
> >   unsigned long mask[];
> > However, the mask may be 8-byte aligned meaning there is a 4-byte pad
> > after long_size. This means 32-bit and 64-bit builds see the mask as
> > being at different offsets. On top of this the structure is in the byte
> > data[] encoded as:
> >   u16 type
> >   char data[]
> > This means the mask's struct isn't the required 4 or 8 byte aligned, but
> > is offset by 2. Consequently the long reads and writes are causing
> > undefined behavior as the alignment is broken.
> >
> > These changes do minor clean up with const, visibility of functions
> > and using the constant time max function. It then adds 32 and 64-bit
> > mask encoding variants, packed to match current alignment. Taking the
> > address of a packed struct leads to unaligned data, so function
> > arguments are altered to be passed the packed struct. To compact the
> > mask encoding further and drop the padding, the 4-byte variant is
> > preferred. Finally a new range encoding is added, that reduces the
> > size of the common case of a range of CPUs to a single u64.
> >
> > On a 72 CPU (hyperthread) machine the original encoding of all CPUs is:
> > 0x9a98 [0x28]: event: 74
> > .
> > . ... raw event: size 40 bytes
> > .  0000:  4a 00 00 00 00 00 28 00 01 00 02 00 08 00 00 00  J.....(.........
> > .  0010:  00 00 ff ff ff ff ff ff ff ff ff 00 00 00 00 00  ................
> > .  0020:  00 00 00 00 00 00 00 00                          ........
> >
> > 0 0 0x9a98 [0x28]: PERF_RECORD_CPU_MAP
> >
> > Using the 4-byte encoding it is:
> > 0x9a98@pipe [0x20]: event: 74
> > .
> > . ... raw event: size 32 bytes
> > .  0000:  4a 00 00 00 00 00 20 00 01 00 03 00 04 00 ff ff  J..... .........
> > .  0010:  ff ff ff ff ff ff ff 00 00 00 00 00 00 00 00 00  ................
> >
> > 0 0 0x9a98 [0x20]: PERF_RECORD_CPU_MAP
> >
> > Finally, with the range encoding it is:
> > 0x9ab8@pipe [0x10]: event: 74
> > .
> > . ... raw event: size 16 bytes
> > .  0000:  4a 00 00 00 00 00 10 00 02 00 00 00 00 00 47 00  J.............G.
> >
> > 0 0 0x9ab8 [0x10]: PERF_RECORD_CPU_MAP
> >
> > v2. Fixes a bug in the size computation of the update header
> >     introduced by the last patch (Add range data encoding) and caught
> >     by address sanitizer.
> >
> > Ian Rogers (6):
> >   perf cpumap: Const map for max
> >   perf cpumap: Synthetic events and const/static
> >   perf cpumap: Compute mask size in constant time
> >   perf cpumap: Fix alignment for masks in event encoding
> >   perf events: Prefer union over variable length array
> >   perf cpumap: Add range data encoding
>
> Ping. There was some feedback on this change but nothing to create a
> v3. Feedback/acked-by/reviewed-by appreciated.

Ping. Feedback appreciated.

Thanks,
Ian

> Thanks,
> Ian
>
> >  tools/lib/perf/cpumap.c              |   2 +-
> >  tools/lib/perf/include/perf/cpumap.h |   2 +-
> >  tools/lib/perf/include/perf/event.h  |  61 ++++++++-
> >  tools/perf/tests/cpumap.c            |  71 ++++++++---
> >  tools/perf/tests/event_update.c      |  14 +--
> >  tools/perf/util/cpumap.c             | 111 +++++++++++++---
> >  tools/perf/util/cpumap.h             |   4 +-
> >  tools/perf/util/event.h              |   4 -
> >  tools/perf/util/header.c             |  24 ++--
> >  tools/perf/util/session.c            |  35 +++---
> >  tools/perf/util/synthetic-events.c   | 182 +++++++++++++--------------
> >  tools/perf/util/synthetic-events.h   |   2 +-
> >  12 files changed, 327 insertions(+), 185 deletions(-)
> >
> > --
> > 2.36.1.476.g0c4daa206d-goog
> >

Re: [PATCH v2 0/6] Corrections to cpu map event encoding

[Jiri Olsa]

On Thu, Jul 28, 2022 at 07:01:09PM -0700, Ian Rogers wrote:
> On Fri, Jul 15, 2022 at 10:01 AM Ian Rogers <irogers@google.com> wrote:
> >
> > On Tue, Jun 14, 2022 at 7:33 AM Ian Rogers <irogers@google.com> wrote:
> > >
> > > A mask encoding of a cpu map is laid out as:
> > >   u16 nr
> > >   u16 long_size
> > >   unsigned long mask[];
> > > However, the mask may be 8-byte aligned meaning there is a 4-byte pad
> > > after long_size. This means 32-bit and 64-bit builds see the mask as
> > > being at different offsets. On top of this the structure is in the byte
> > > data[] encoded as:
> > >   u16 type
> > >   char data[]
> > > This means the mask's struct isn't the required 4 or 8 byte aligned, but
> > > is offset by 2. Consequently the long reads and writes are causing
> > > undefined behavior as the alignment is broken.
> > >
> > > These changes do minor clean up with const, visibility of functions
> > > and using the constant time max function. It then adds 32 and 64-bit
> > > mask encoding variants, packed to match current alignment. Taking the
> > > address of a packed struct leads to unaligned data, so function
> > > arguments are altered to be passed the packed struct. To compact the
> > > mask encoding further and drop the padding, the 4-byte variant is
> > > preferred. Finally a new range encoding is added, that reduces the
> > > size of the common case of a range of CPUs to a single u64.
> > >
> > > On a 72 CPU (hyperthread) machine the original encoding of all CPUs is:
> > > 0x9a98 [0x28]: event: 74
> > > .
> > > . ... raw event: size 40 bytes
> > > .  0000:  4a 00 00 00 00 00 28 00 01 00 02 00 08 00 00 00  J.....(.........
> > > .  0010:  00 00 ff ff ff ff ff ff ff ff ff 00 00 00 00 00  ................
> > > .  0020:  00 00 00 00 00 00 00 00                          ........
> > >
> > > 0 0 0x9a98 [0x28]: PERF_RECORD_CPU_MAP
> > >
> > > Using the 4-byte encoding it is:
> > > 0x9a98@pipe [0x20]: event: 74
> > > .
> > > . ... raw event: size 32 bytes
> > > .  0000:  4a 00 00 00 00 00 20 00 01 00 03 00 04 00 ff ff  J..... .........
> > > .  0010:  ff ff ff ff ff ff ff 00 00 00 00 00 00 00 00 00  ................
> > >
> > > 0 0 0x9a98 [0x20]: PERF_RECORD_CPU_MAP
> > >
> > > Finally, with the range encoding it is:
> > > 0x9ab8@pipe [0x10]: event: 74
> > > .
> > > . ... raw event: size 16 bytes
> > > .  0000:  4a 00 00 00 00 00 10 00 02 00 00 00 00 00 47 00  J.............G.
> > >
> > > 0 0 0x9ab8 [0x10]: PERF_RECORD_CPU_MAP
> > >
> > > v2. Fixes a bug in the size computation of the update header
> > >     introduced by the last patch (Add range data encoding) and caught
> > >     by address sanitizer.
> > >
> > > Ian Rogers (6):
> > >   perf cpumap: Const map for max
> > >   perf cpumap: Synthetic events and const/static
> > >   perf cpumap: Compute mask size in constant time
> > >   perf cpumap: Fix alignment for masks in event encoding
> > >   perf events: Prefer union over variable length array
> > >   perf cpumap: Add range data encoding
> >
> > Ping. There was some feedback on this change but nothing to create a
> > v3. Feedback/acked-by/reviewed-by appreciated.
> 
> Ping. Feedback appreciated.

hi,
there's some unanswered feedback:
https://lore.kernel.org/linux-perf-users/YrwY3SP+jsTwrRBw@krava/

jirka

Re: [PATCH v2 0/6] Corrections to cpu map event encoding

[Ian Rogers]

On Fri, Jul 29, 2022 at 4:35 AM Jiri Olsa <olsajiri@gmail.com> wrote:
>
> On Thu, Jul 28, 2022 at 07:01:09PM -0700, Ian Rogers wrote:
> > On Fri, Jul 15, 2022 at 10:01 AM Ian Rogers <irogers@google.com> wrote:
> > >
> > > On Tue, Jun 14, 2022 at 7:33 AM Ian Rogers <irogers@google.com> wrote:
> > > >
> > > > A mask encoding of a cpu map is laid out as:
> > > >   u16 nr
> > > >   u16 long_size
> > > >   unsigned long mask[];
> > > > However, the mask may be 8-byte aligned meaning there is a 4-byte pad
> > > > after long_size. This means 32-bit and 64-bit builds see the mask as
> > > > being at different offsets. On top of this the structure is in the byte
> > > > data[] encoded as:
> > > >   u16 type
> > > >   char data[]
> > > > This means the mask's struct isn't the required 4 or 8 byte aligned, but
> > > > is offset by 2. Consequently the long reads and writes are causing
> > > > undefined behavior as the alignment is broken.
> > > >
> > > > These changes do minor clean up with const, visibility of functions
> > > > and using the constant time max function. It then adds 32 and 64-bit
> > > > mask encoding variants, packed to match current alignment. Taking the
> > > > address of a packed struct leads to unaligned data, so function
> > > > arguments are altered to be passed the packed struct. To compact the
> > > > mask encoding further and drop the padding, the 4-byte variant is
> > > > preferred. Finally a new range encoding is added, that reduces the
> > > > size of the common case of a range of CPUs to a single u64.
> > > >
> > > > On a 72 CPU (hyperthread) machine the original encoding of all CPUs is:
> > > > 0x9a98 [0x28]: event: 74
> > > > .
> > > > . ... raw event: size 40 bytes
> > > > .  0000:  4a 00 00 00 00 00 28 00 01 00 02 00 08 00 00 00  J.....(.........
> > > > .  0010:  00 00 ff ff ff ff ff ff ff ff ff 00 00 00 00 00  ................
> > > > .  0020:  00 00 00 00 00 00 00 00                          ........
> > > >
> > > > 0 0 0x9a98 [0x28]: PERF_RECORD_CPU_MAP
> > > >
> > > > Using the 4-byte encoding it is:
> > > > 0x9a98@pipe [0x20]: event: 74
> > > > .
> > > > . ... raw event: size 32 bytes
> > > > .  0000:  4a 00 00 00 00 00 20 00 01 00 03 00 04 00 ff ff  J..... .........
> > > > .  0010:  ff ff ff ff ff ff ff 00 00 00 00 00 00 00 00 00  ................
> > > >
> > > > 0 0 0x9a98 [0x20]: PERF_RECORD_CPU_MAP
> > > >
> > > > Finally, with the range encoding it is:
> > > > 0x9ab8@pipe [0x10]: event: 74
> > > > .
> > > > . ... raw event: size 16 bytes
> > > > .  0000:  4a 00 00 00 00 00 10 00 02 00 00 00 00 00 47 00  J.............G.
> > > >
> > > > 0 0 0x9ab8 [0x10]: PERF_RECORD_CPU_MAP
> > > >
> > > > v2. Fixes a bug in the size computation of the update header
> > > >     introduced by the last patch (Add range data encoding) and caught
> > > >     by address sanitizer.
> > > >
> > > > Ian Rogers (6):
> > > >   perf cpumap: Const map for max
> > > >   perf cpumap: Synthetic events and const/static
> > > >   perf cpumap: Compute mask size in constant time
> > > >   perf cpumap: Fix alignment for masks in event encoding
> > > >   perf events: Prefer union over variable length array
> > > >   perf cpumap: Add range data encoding
> > >
> > > Ping. There was some feedback on this change but nothing to create a
> > > v3. Feedback/acked-by/reviewed-by appreciated.
> >
> > Ping. Feedback appreciated.
>
> hi,
> there's some unanswered feedback:
> https://lore.kernel.org/linux-perf-users/YrwY3SP+jsTwrRBw@krava/
>
> jirka

Thanks Jirka,

Was there a comment in particular? My reply was here:
https://lore.kernel.org/linux-perf-users/CAP-5=fU=BpP4OT2axZLSfRnKxQxmv-HXj8khBgmx3XQMS+abgA@mail.gmail.com/
I double checked, everyone of your comments was answered.

Thanks,
Ian

Re: [PATCH v2 0/6] Corrections to cpu map event encoding

[Jiri Olsa]

On Fri, Jul 29, 2022 at 07:28:36AM -0700, Ian Rogers wrote:
> On Fri, Jul 29, 2022 at 4:35 AM Jiri Olsa <olsajiri@gmail.com> wrote:
> >
> > On Thu, Jul 28, 2022 at 07:01:09PM -0700, Ian Rogers wrote:
> > > On Fri, Jul 15, 2022 at 10:01 AM Ian Rogers <irogers@google.com> wrote:
> > > >
> > > > On Tue, Jun 14, 2022 at 7:33 AM Ian Rogers <irogers@google.com> wrote:
> > > > >
> > > > > A mask encoding of a cpu map is laid out as:
> > > > >   u16 nr
> > > > >   u16 long_size
> > > > >   unsigned long mask[];
> > > > > However, the mask may be 8-byte aligned meaning there is a 4-byte pad
> > > > > after long_size. This means 32-bit and 64-bit builds see the mask as
> > > > > being at different offsets. On top of this the structure is in the byte
> > > > > data[] encoded as:
> > > > >   u16 type
> > > > >   char data[]
> > > > > This means the mask's struct isn't the required 4 or 8 byte aligned, but
> > > > > is offset by 2. Consequently the long reads and writes are causing
> > > > > undefined behavior as the alignment is broken.
> > > > >
> > > > > These changes do minor clean up with const, visibility of functions
> > > > > and using the constant time max function. It then adds 32 and 64-bit
> > > > > mask encoding variants, packed to match current alignment. Taking the
> > > > > address of a packed struct leads to unaligned data, so function
> > > > > arguments are altered to be passed the packed struct. To compact the
> > > > > mask encoding further and drop the padding, the 4-byte variant is
> > > > > preferred. Finally a new range encoding is added, that reduces the
> > > > > size of the common case of a range of CPUs to a single u64.
> > > > >
> > > > > On a 72 CPU (hyperthread) machine the original encoding of all CPUs is:
> > > > > 0x9a98 [0x28]: event: 74
> > > > > .
> > > > > . ... raw event: size 40 bytes
> > > > > .  0000:  4a 00 00 00 00 00 28 00 01 00 02 00 08 00 00 00  J.....(.........
> > > > > .  0010:  00 00 ff ff ff ff ff ff ff ff ff 00 00 00 00 00  ................
> > > > > .  0020:  00 00 00 00 00 00 00 00                          ........
> > > > >
> > > > > 0 0 0x9a98 [0x28]: PERF_RECORD_CPU_MAP
> > > > >
> > > > > Using the 4-byte encoding it is:
> > > > > 0x9a98@pipe [0x20]: event: 74
> > > > > .
> > > > > . ... raw event: size 32 bytes
> > > > > .  0000:  4a 00 00 00 00 00 20 00 01 00 03 00 04 00 ff ff  J..... .........
> > > > > .  0010:  ff ff ff ff ff ff ff 00 00 00 00 00 00 00 00 00  ................
> > > > >
> > > > > 0 0 0x9a98 [0x20]: PERF_RECORD_CPU_MAP
> > > > >
> > > > > Finally, with the range encoding it is:
> > > > > 0x9ab8@pipe [0x10]: event: 74
> > > > > .
> > > > > . ... raw event: size 16 bytes
> > > > > .  0000:  4a 00 00 00 00 00 10 00 02 00 00 00 00 00 47 00  J.............G.
> > > > >
> > > > > 0 0 0x9ab8 [0x10]: PERF_RECORD_CPU_MAP
> > > > >
> > > > > v2. Fixes a bug in the size computation of the update header
> > > > >     introduced by the last patch (Add range data encoding) and caught
> > > > >     by address sanitizer.
> > > > >
> > > > > Ian Rogers (6):
> > > > >   perf cpumap: Const map for max
> > > > >   perf cpumap: Synthetic events and const/static
> > > > >   perf cpumap: Compute mask size in constant time
> > > > >   perf cpumap: Fix alignment for masks in event encoding
> > > > >   perf events: Prefer union over variable length array
> > > > >   perf cpumap: Add range data encoding
> > > >
> > > > Ping. There was some feedback on this change but nothing to create a
> > > > v3. Feedback/acked-by/reviewed-by appreciated.
> > >
> > > Ping. Feedback appreciated.
> >
> > hi,
> > there's some unanswered feedback:
> > https://lore.kernel.org/linux-perf-users/YrwY3SP+jsTwrRBw@krava/
> >
> > jirka
> 
> Thanks Jirka,
> 
> Was there a comment in particular? My reply was here:
> https://lore.kernel.org/linux-perf-users/CAP-5=fU=BpP4OT2axZLSfRnKxQxmv-HXj8khBgmx3XQMS+abgA@mail.gmail.com/
> I double checked, everyone of your comments was answered.

ugh sorry, seems it did not get into my inbox for some reason

jirka

Re: [PATCH v2 0/6] Corrections to cpu map event encoding

[Jiri Olsa]

On Tue, Jun 14, 2022 at 07:33:47AM -0700, Ian Rogers wrote:
> A mask encoding of a cpu map is laid out as:
>   u16 nr
>   u16 long_size
>   unsigned long mask[];
> However, the mask may be 8-byte aligned meaning there is a 4-byte pad
> after long_size. This means 32-bit and 64-bit builds see the mask as
> being at different offsets. On top of this the structure is in the byte
> data[] encoded as:
>   u16 type
>   char data[]
> This means the mask's struct isn't the required 4 or 8 byte aligned, but
> is offset by 2. Consequently the long reads and writes are causing
> undefined behavior as the alignment is broken.
> 
> These changes do minor clean up with const, visibility of functions
> and using the constant time max function. It then adds 32 and 64-bit
> mask encoding variants, packed to match current alignment. Taking the
> address of a packed struct leads to unaligned data, so function
> arguments are altered to be passed the packed struct. To compact the
> mask encoding further and drop the padding, the 4-byte variant is
> preferred. Finally a new range encoding is added, that reduces the
> size of the common case of a range of CPUs to a single u64.
> 
> On a 72 CPU (hyperthread) machine the original encoding of all CPUs is:
> 0x9a98 [0x28]: event: 74
> .
> . ... raw event: size 40 bytes
> .  0000:  4a 00 00 00 00 00 28 00 01 00 02 00 08 00 00 00  J.....(.........
> .  0010:  00 00 ff ff ff ff ff ff ff ff ff 00 00 00 00 00  ................
> .  0020:  00 00 00 00 00 00 00 00                          ........        
> 
> 0 0 0x9a98 [0x28]: PERF_RECORD_CPU_MAP
> 
> Using the 4-byte encoding it is:
> 0x9a98@pipe [0x20]: event: 74
> .
> . ... raw event: size 32 bytes
> .  0000:  4a 00 00 00 00 00 20 00 01 00 03 00 04 00 ff ff  J..... .........
> .  0010:  ff ff ff ff ff ff ff 00 00 00 00 00 00 00 00 00  ................
> 
> 0 0 0x9a98 [0x20]: PERF_RECORD_CPU_MAP
> 
> Finally, with the range encoding it is:
> 0x9ab8@pipe [0x10]: event: 74
> .
> . ... raw event: size 16 bytes
> .  0000:  4a 00 00 00 00 00 10 00 02 00 00 00 00 00 47 00  J.............G.
> 
> 0 0 0x9ab8 [0x10]: PERF_RECORD_CPU_MAP
> 
> v2. Fixes a bug in the size computation of the update header
>     introduced by the last patch (Add range data encoding) and caught
>     by address sanitizer.
> 
> Ian Rogers (6):
>   perf cpumap: Const map for max
>   perf cpumap: Synthetic events and const/static
>   perf cpumap: Compute mask size in constant time
>   perf cpumap: Fix alignment for masks in event encoding
>   perf events: Prefer union over variable length array
>   perf cpumap: Add range data encoding

Acked-by: Jiri Olsa <jolsa@kernel.org>

thanks,
jirka

> 
>  tools/lib/perf/cpumap.c              |   2 +-
>  tools/lib/perf/include/perf/cpumap.h |   2 +-
>  tools/lib/perf/include/perf/event.h  |  61 ++++++++-
>  tools/perf/tests/cpumap.c            |  71 ++++++++---
>  tools/perf/tests/event_update.c      |  14 +--
>  tools/perf/util/cpumap.c             | 111 +++++++++++++---
>  tools/perf/util/cpumap.h             |   4 +-
>  tools/perf/util/event.h              |   4 -
>  tools/perf/util/header.c             |  24 ++--
>  tools/perf/util/session.c            |  35 +++---
>  tools/perf/util/synthetic-events.c   | 182 +++++++++++++--------------
>  tools/perf/util/synthetic-events.h   |   2 +-
>  12 files changed, 327 insertions(+), 185 deletions(-)
> 
> -- 
> 2.36.1.476.g0c4daa206d-goog
> 

Re: [PATCH v2 0/6] Corrections to cpu map event encoding

[Arnaldo Carvalho de Melo]

Em Sun, Aug 14, 2022 at 04:05:19PM -0700, Ian Rogers escreveu:
> Hi,
> 
> It looks like we missed this one. Can it get into the next PR as a fix?

I've queued up to 4/6 for perf/urgent, 5 and 6 will go to perf/core once
I merge perf/urgent there, i.e. after I send this to Linus.

- Arnaldo
 
> Thanks,
> Ian
> 
> 
> On Thu, Aug 4, 2022 at 1:23 PM Jiri Olsa <olsajiri@gmail.com> wrote:
> 
> > On Tue, Jun 14, 2022 at 07:33:47AM -0700, Ian Rogers wrote:
> > > A mask encoding of a cpu map is laid out as:
> > >   u16 nr
> > >   u16 long_size
> > >   unsigned long mask[];
> > > However, the mask may be 8-byte aligned meaning there is a 4-byte pad
> > > after long_size. This means 32-bit and 64-bit builds see the mask as
> > > being at different offsets. On top of this the structure is in the byte
> > > data[] encoded as:
> > >   u16 type
> > >   char data[]
> > > This means the mask's struct isn't the required 4 or 8 byte aligned, but
> > > is offset by 2. Consequently the long reads and writes are causing
> > > undefined behavior as the alignment is broken.
> > >
> > > These changes do minor clean up with const, visibility of functions
> > > and using the constant time max function. It then adds 32 and 64-bit
> > > mask encoding variants, packed to match current alignment. Taking the
> > > address of a packed struct leads to unaligned data, so function
> > > arguments are altered to be passed the packed struct. To compact the
> > > mask encoding further and drop the padding, the 4-byte variant is
> > > preferred. Finally a new range encoding is added, that reduces the
> > > size of the common case of a range of CPUs to a single u64.
> > >
> > > On a 72 CPU (hyperthread) machine the original encoding of all CPUs is:
> > > 0x9a98 [0x28]: event: 74
> > > .
> > > . ... raw event: size 40 bytes
> > > .  0000:  4a 00 00 00 00 00 28 00 01 00 02 00 08 00 00 00
> > J.....(.........
> > > .  0010:  00 00 ff ff ff ff ff ff ff ff ff 00 00 00 00 00
> > ................
> > > .  0020:  00 00 00 00 00 00 00 00                          ........
> >
> > >
> > > 0 0 0x9a98 [0x28]: PERF_RECORD_CPU_MAP
> > >
> > > Using the 4-byte encoding it is:
> > > 0x9a98@pipe [0x20]: event: 74
> > > .
> > > . ... raw event: size 32 bytes
> > > .  0000:  4a 00 00 00 00 00 20 00 01 00 03 00 04 00 ff ff  J.....
> > .........
> > > .  0010:  ff ff ff ff ff ff ff 00 00 00 00 00 00 00 00 00
> > ................
> > >
> > > 0 0 0x9a98 [0x20]: PERF_RECORD_CPU_MAP
> > >
> > > Finally, with the range encoding it is:
> > > 0x9ab8@pipe [0x10]: event: 74
> > > .
> > > . ... raw event: size 16 bytes
> > > .  0000:  4a 00 00 00 00 00 10 00 02 00 00 00 00 00 47 00
> > J.............G.
> > >
> > > 0 0 0x9ab8 [0x10]: PERF_RECORD_CPU_MAP
> > >
> > > v2. Fixes a bug in the size computation of the update header
> > >     introduced by the last patch (Add range data encoding) and caught
> > >     by address sanitizer.
> > >
> > > Ian Rogers (6):
> > >   perf cpumap: Const map for max
> > >   perf cpumap: Synthetic events and const/static
> > >   perf cpumap: Compute mask size in constant time
> > >   perf cpumap: Fix alignment for masks in event encoding
> > >   perf events: Prefer union over variable length array
> > >   perf cpumap: Add range data encoding
> >
> > Acked-by: Jiri Olsa <jolsa@kernel.org>
> >
> > thanks,
> > jirka
> >
> > >
> > >  tools/lib/perf/cpumap.c              |   2 +-
> > >  tools/lib/perf/include/perf/cpumap.h |   2 +-
> > >  tools/lib/perf/include/perf/event.h  |  61 ++++++++-
> > >  tools/perf/tests/cpumap.c            |  71 ++++++++---
> > >  tools/perf/tests/event_update.c      |  14 +--
> > >  tools/perf/util/cpumap.c             | 111 +++++++++++++---
> > >  tools/perf/util/cpumap.h             |   4 +-
> > >  tools/perf/util/event.h              |   4 -
> > >  tools/perf/util/header.c             |  24 ++--
> > >  tools/perf/util/session.c            |  35 +++---
> > >  tools/perf/util/synthetic-events.c   | 182 +++++++++++++--------------
> > >  tools/perf/util/synthetic-events.h   |   2 +-
> > >  12 files changed, 327 insertions(+), 185 deletions(-)
> > >
> > > --
> > > 2.36.1.476.g0c4daa206d-goog
> > >
> >

-- 

- Arnaldo