[PATCH v3 0/5] perf file align features, avoid UB

[PATCH v3 0/5] perf file align features, avoid UB

[Ian Rogers]

Features like hostname may not be 8-byte aligned. Add padding to make
the feature aligned to avoid undefined behavior (UB). Modify the
writing of the CPU topology features to always write die_cpus_list
even if empty, as the size could have been aligned and not allow the
missing list to be detected. As die_id may be invalid (< 0) ensure it
isn't for platforms like ARM. Avoid UB also in machine where a NULL
may be incremented.

v3: Additional patch to avoid invalid die_ids
v2: Fix CPU topology as described in replies to v1.
v1: https://lore.kernel.org/lkml/20241212080530.1329601-1-irogers@google.com/

Ian Rogers (5):
  perf cpumap: If the die_id is missing use socket/physical_package_id
  perf header: Write out even empty die_cpus_list
  perf synthetic-events: Ensure features are aligned
  perf machine: Avoid UB by delaying computing branch entries
  perf record: Assert synthesized events are 8-byte aligned

 tools/perf/builtin-record.c        |  5 ++++-
 tools/perf/util/cpumap.c           |  3 ++-
 tools/perf/util/header.c           | 10 ++++------
 tools/perf/util/machine.c          |  2 +-
 tools/perf/util/synthetic-events.c |  2 ++
 5 files changed, 13 insertions(+), 9 deletions(-)

-- 
2.47.1.613.gc27f4b7a9f-goog

[PATCH v3 1/5] perf cpumap: If the die_id is missing use socket/physical_package_id

[Ian Rogers]

An error value for a missing die_id may be written into things like
the cpu_topology_map. As the topology needs to be fully written out,
including the die_id, to allow perf.data file features to be aligned
we can't allow error values to be written out. Instead base the
missing die_id value off of the socket/physical_package_id assuming
they correlate 1:1.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/cpumap.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/cpumap.c b/tools/perf/util/cpumap.c
index 27094211edd8..d362272f8466 100644
--- a/tools/perf/util/cpumap.c
+++ b/tools/perf/util/cpumap.c
@@ -283,7 +283,8 @@ int cpu__get_die_id(struct perf_cpu cpu)
 {
 	int value, ret = cpu__get_topology_int(cpu.cpu, "die_id", &value);
 
-	return ret ?: value;
+        /* If die_id is missing fallback on using the socket/physical_package_id. */
+	return ret || value < 0 ? cpu__get_socket_id(cpu) : value;
 }
 
 struct aggr_cpu_id aggr_cpu_id__die(struct perf_cpu cpu, void *data)
-- 
2.47.1.613.gc27f4b7a9f-goog

[PATCH v3 2/5] perf header: Write out even empty die_cpus_list

[Ian Rogers]

Reading the CPU topology is tolerant to the dies_cpus_list being
missing by using the feature's size in the data file/pipe. However,
the feature's size is just the header size and may be
unaligned. Making the header size aligned breaks detecting a missing
die_cpus_list. To allow the header size to be aligned always write the
die_cpus_lists even if empty.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/header.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
index 048e563069bc..03e43a9894d4 100644
--- a/tools/perf/util/header.c
+++ b/tools/perf/util/header.c
@@ -628,9 +628,6 @@ static int write_cpu_topology(struct feat_fd *ff,
 			return ret;
 	}
 
-	if (!tp->die_cpus_lists)
-		goto done;
-
 	ret = do_write(ff, &tp->die_cpus_lists, sizeof(tp->die_cpus_lists));
 	if (ret < 0)
 		goto done;
-- 
2.47.1.613.gc27f4b7a9f-goog

[PATCH v3 3/5] perf synthetic-events: Ensure features are aligned

[Ian Rogers]

Features like hostname have arbitrary size and break the assumed
8-byte alignment of perf events. Pad all feature events until 8-byte
alignment is restored.

Rather than invent a new mechanism, reuse write_padded but pass an
empty buffer as what to write. As no alignment may be necessary,
update write_padded to be tolerant of 0 as a count and count_aligned
value.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/header.c           | 7 ++++---
 tools/perf/util/synthetic-events.c | 2 ++
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
index 03e43a9894d4..d10703090e83 100644
--- a/tools/perf/util/header.c
+++ b/tools/perf/util/header.c
@@ -168,11 +168,12 @@ int write_padded(struct feat_fd *ff, const void *bf,
 		 size_t count, size_t count_aligned)
 {
 	static const char zero_buf[NAME_ALIGN];
-	int err = do_write(ff, bf, count);
+	int err = count > 0 ? do_write(ff, bf, count) : 0;
 
-	if (!err)
+	if (!err && count_aligned > count) {
+		assert(count_aligned - count < sizeof(zero_buf));
 		err = do_write(ff, zero_buf, count_aligned - count);
-
+	}
 	return err;
 }
 
diff --git a/tools/perf/util/synthetic-events.c b/tools/perf/util/synthetic-events.c
index f8ac2ac2da45..ee6e9c2fb11b 100644
--- a/tools/perf/util/synthetic-events.c
+++ b/tools/perf/util/synthetic-events.c
@@ -2401,6 +2401,8 @@ int perf_event__synthesize_features(const struct perf_tool *tool, struct perf_se
 			pr_debug("Error writing feature\n");
 			continue;
 		}
+		write_padded(&ff, /*bf=*/NULL, /*count=*/0,
+			     /*count_aligned=*/PERF_ALIGN(ff.offset, sizeof(u64)) - ff.offset);
 		/* ff.buf may have changed due to realloc in do_write() */
 		fe = ff.buf;
 		memset(fe, 0, sizeof(*fe));
-- 
2.47.1.613.gc27f4b7a9f-goog

[PATCH v3 4/5] perf machine: Avoid UB by delaying computing branch entries

[Ian Rogers]

If the branch_stack is NULL then perf_sample__branch_entries may
return NULL+1 which triggers ubsan (undefined behavior
sanitizer). Avoid this by making the computation conditional on branch
existing.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/machine.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
index 84661d950104..7b1e1c254c17 100644
--- a/tools/perf/util/machine.c
+++ b/tools/perf/util/machine.c
@@ -2670,7 +2670,6 @@ static int thread__resolve_callchain_sample(struct thread *thread,
 					    bool symbols)
 {
 	struct branch_stack *branch = sample->branch_stack;
-	struct branch_entry *entries = perf_sample__branch_entries(sample);
 	struct ip_callchain *chain = sample->callchain;
 	int chain_nr = 0;
 	u8 cpumode = PERF_RECORD_MISC_USER;
@@ -2712,6 +2711,7 @@ static int thread__resolve_callchain_sample(struct thread *thread,
 	 */
 
 	if (branch && callchain_param.branch_callstack) {
+		struct branch_entry *entries = perf_sample__branch_entries(sample);
 		int nr = min(max_stack, (int)branch->nr);
 		struct branch_entry be[nr];
 		struct iterations iter[nr];
-- 
2.47.1.613.gc27f4b7a9f-goog

[PATCH v3 5/5] perf record: Assert synthesized events are 8-byte aligned

[Ian Rogers]

Capture that events are 8-byte aligned and avoid later misaligned
event problems.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/builtin-record.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index adbaf80b398c..a5689d0e93ad 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -624,7 +624,10 @@ static int process_synthesized_event(const struct perf_tool *tool,
 				     struct machine *machine __maybe_unused)
 {
 	struct record *rec = container_of(tool, struct record, tool);
-	return record__write(rec, NULL, event, event->header.size);
+	size_t size = event->header.size;
+
+	assert(PERF_ALIGN(size, sizeof(u64)) == size);
+	return record__write(rec, NULL, event, size);
 }
 
 static struct mutex synth_lock;
-- 
2.47.1.613.gc27f4b7a9f-goog

Re: [PATCH v3 1/5] perf cpumap: If the die_id is missing use socket/physical_package_id

[James Clark]

On 16/12/2024 11:24 pm, Ian Rogers wrote:
> An error value for a missing die_id may be written into things like
> the cpu_topology_map. As the topology needs to be fully written out,
> including the die_id, to allow perf.data file features to be aligned
> we can't allow error values to be written out. Instead base the
> missing die_id value off of the socket/physical_package_id assuming
> they correlate 1:1.
> 
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>   tools/perf/util/cpumap.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/perf/util/cpumap.c b/tools/perf/util/cpumap.c
> index 27094211edd8..d362272f8466 100644
> --- a/tools/perf/util/cpumap.c
> +++ b/tools/perf/util/cpumap.c
> @@ -283,7 +283,8 @@ int cpu__get_die_id(struct perf_cpu cpu)
>   {
>   	int value, ret = cpu__get_topology_int(cpu.cpu, "die_id", &value);
>   
> -	return ret ?: value;
> +        /* If die_id is missing fallback on using the socket/physical_package_id. */
> +	return ret || value < 0 ? cpu__get_socket_id(cpu) : value;
>   }
>   
>   struct aggr_cpu_id aggr_cpu_id__die(struct perf_cpu cpu, void *data)

Hi Ian,

I sent a fix for the same or a similar problem here [1]. For this one 
I'm not sure why we'd want to use the socket ID for die when it's always 
been 0 for not present. I wonder if this change is mingling two things: 
fixing the negative error value appearing and replacing die with socket ID.

Personally I would prefer to keep the 0 to fix the error value, that way 
nobody gets surprised by the change.

Also it looks like cpu__get_cluster_id() can suffer from the same issue, 
and if we do it this way we should drop these as they aren't valid anymore:

	/* There is no die_id on legacy system. */
	if (die < 0)
		die = 0;

And last minor comment this could do with a fixes: 05be17eed774

[1]: 
https://lore.kernel.org/linux-perf-users/20241218115552.912517-1-james.clark@linaro.org/T/#u

Re: [PATCH v3 1/5] perf cpumap: If the die_id is missing use socket/physical_package_id

[Ian Rogers]

On Wed, Dec 18, 2024 at 4:04 AM James Clark <james.clark@linaro.org> wrote:
>
>
>
> On 16/12/2024 11:24 pm, Ian Rogers wrote:
> > An error value for a missing die_id may be written into things like
> > the cpu_topology_map. As the topology needs to be fully written out,
> > including the die_id, to allow perf.data file features to be aligned
> > we can't allow error values to be written out. Instead base the
> > missing die_id value off of the socket/physical_package_id assuming
> > they correlate 1:1.
> >
> > Signed-off-by: Ian Rogers <irogers@google.com>
> > ---
> >   tools/perf/util/cpumap.c | 3 ++-
> >   1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/tools/perf/util/cpumap.c b/tools/perf/util/cpumap.c
> > index 27094211edd8..d362272f8466 100644
> > --- a/tools/perf/util/cpumap.c
> > +++ b/tools/perf/util/cpumap.c
> > @@ -283,7 +283,8 @@ int cpu__get_die_id(struct perf_cpu cpu)
> >   {
> >       int value, ret = cpu__get_topology_int(cpu.cpu, "die_id", &value);
> >
> > -     return ret ?: value;
> > +        /* If die_id is missing fallback on using the socket/physical_package_id. */
> > +     return ret || value < 0 ? cpu__get_socket_id(cpu) : value;
> >   }
> >
> >   struct aggr_cpu_id aggr_cpu_id__die(struct perf_cpu cpu, void *data)
>
> Hi Ian,
>
> I sent a fix for the same or a similar problem here [1]. For this one
> I'm not sure why we'd want to use the socket ID for die when it's always
> been 0 for not present. I wonder if this change is mingling two things:
> fixing the negative error value appearing and replacing die with socket ID.
>
> Personally I would prefer to keep the 0 to fix the error value, that way
> nobody gets surprised by the change.
>
> Also it looks like cpu__get_cluster_id() can suffer from the same issue,
> and if we do it this way we should drop these as they aren't valid anymore:
>
>         /* There is no die_id on legacy system. */
>         if (die < 0)
>                 die = 0;

I think this breaks the assumption here:
https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/tests/expr.c?h=perf-tools-next#n244
hence using the number of packages. I think we need to make sure there
is order otherwise we'll end up with "if no dies value present" or "if
dies value is 0 ignore" everywhere. What is strange to me is that ARM
may have the die_id present but have it contain the value -1.

> And last minor comment this could do with a fixes: 05be17eed774

Thanks,
Ian

> [1]:
> https://lore.kernel.org/linux-perf-users/20241218115552.912517-1-james.clark@linaro.org/T/#u
>

Re: [PATCH v3 3/5] perf synthetic-events: Ensure features are aligned

[Namhyung Kim]

On Mon, Dec 16, 2024 at 03:24:57PM -0800, Ian Rogers wrote:
> Features like hostname have arbitrary size and break the assumed
> 8-byte alignment of perf events. Pad all feature events until 8-byte
> alignment is restored.

But it seems write_hostname() and others use do_write_string() which
handles the padding already.

thanks,
Namhyung

> 
> Rather than invent a new mechanism, reuse write_padded but pass an
> empty buffer as what to write. As no alignment may be necessary,
> update write_padded to be tolerant of 0 as a count and count_aligned
> value.
> 
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/util/header.c           | 7 ++++---
>  tools/perf/util/synthetic-events.c | 2 ++
>  2 files changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
> index 03e43a9894d4..d10703090e83 100644
> --- a/tools/perf/util/header.c
> +++ b/tools/perf/util/header.c
> @@ -168,11 +168,12 @@ int write_padded(struct feat_fd *ff, const void *bf,
>  		 size_t count, size_t count_aligned)
>  {
>  	static const char zero_buf[NAME_ALIGN];
> -	int err = do_write(ff, bf, count);
> +	int err = count > 0 ? do_write(ff, bf, count) : 0;
>  
> -	if (!err)
> +	if (!err && count_aligned > count) {
> +		assert(count_aligned - count < sizeof(zero_buf));
>  		err = do_write(ff, zero_buf, count_aligned - count);
> -
> +	}
>  	return err;
>  }
>  
> diff --git a/tools/perf/util/synthetic-events.c b/tools/perf/util/synthetic-events.c
> index f8ac2ac2da45..ee6e9c2fb11b 100644
> --- a/tools/perf/util/synthetic-events.c
> +++ b/tools/perf/util/synthetic-events.c
> @@ -2401,6 +2401,8 @@ int perf_event__synthesize_features(const struct perf_tool *tool, struct perf_se
>  			pr_debug("Error writing feature\n");
>  			continue;
>  		}
> +		write_padded(&ff, /*bf=*/NULL, /*count=*/0,
> +			     /*count_aligned=*/PERF_ALIGN(ff.offset, sizeof(u64)) - ff.offset);
>  		/* ff.buf may have changed due to realloc in do_write() */
>  		fe = ff.buf;
>  		memset(fe, 0, sizeof(*fe));
> -- 
> 2.47.1.613.gc27f4b7a9f-goog
> 

Re: [PATCH v3 3/5] perf synthetic-events: Ensure features are aligned

[Ian Rogers]

On Wed, Dec 18, 2024 at 5:08 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> On Mon, Dec 16, 2024 at 03:24:57PM -0800, Ian Rogers wrote:
> > Features like hostname have arbitrary size and break the assumed
> > 8-byte alignment of perf events. Pad all feature events until 8-byte
> > alignment is restored.
>
> But it seems write_hostname() and others use do_write_string() which
> handles the padding already.

Well it does something :-) (I agree my description isn't 100%)
https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/header.c?h=perf-tools-next#n188
```
static int do_write_string(struct feat_fd *ff, const char *str)
{
u32 len, olen;
int ret;

olen = strlen(str) + 1;
len = PERF_ALIGN(olen, NAME_ALIGN);

/* write len, incl. \0 */
ret = do_write(ff, &len, sizeof(len));
if (ret < 0)
return ret;

return write_padded(ff, str, olen, len);
}
```
but NAME_ALIGN is 64:
https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/header.h?h=perf-tools-next#n187
whereas the data file is expecting things aligned to sizeof(u64) which
is 8-bytes. We're potentially writing out 7 u64s worth of 0s for no
alignment gain. It should be safe, given I don't see other use of this
alignment value, to switch the string alignment to being sizeof(u64)
but following this change we could also just get rid of the padding
knowing it will be fixed in the caller.

Thanks,
Ian

Re: [PATCH v3 3/5] perf synthetic-events: Ensure features are aligned

[Namhyung Kim]

On Wed, Dec 18, 2024 at 05:29:01PM -0800, Ian Rogers wrote:
> On Wed, Dec 18, 2024 at 5:08 PM Namhyung Kim <namhyung@kernel.org> wrote:
> >
> > On Mon, Dec 16, 2024 at 03:24:57PM -0800, Ian Rogers wrote:
> > > Features like hostname have arbitrary size and break the assumed
> > > 8-byte alignment of perf events. Pad all feature events until 8-byte
> > > alignment is restored.
> >
> > But it seems write_hostname() and others use do_write_string() which
> > handles the padding already.
> 
> Well it does something :-) (I agree my description isn't 100%)
> https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/header.c?h=perf-tools-next#n188
> ```
> static int do_write_string(struct feat_fd *ff, const char *str)
> {
> u32 len, olen;
> int ret;
> 
> olen = strlen(str) + 1;
> len = PERF_ALIGN(olen, NAME_ALIGN);
> 
> /* write len, incl. \0 */
> ret = do_write(ff, &len, sizeof(len));
> if (ret < 0)
> return ret;
> 
> return write_padded(ff, str, olen, len);
> }
> ```
> but NAME_ALIGN is 64:
> https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/header.h?h=perf-tools-next#n187
> whereas the data file is expecting things aligned to sizeof(u64) which
> is 8-bytes. We're potentially writing out 7 u64s worth of 0s for no
> alignment gain. It should be safe, given I don't see other use of this
> alignment value, to switch the string alignment to being sizeof(u64)
> but following this change we could also just get rid of the padding
> knowing it will be fixed in the caller.

Probably, but it seems simpler just to change NAME_ALIGN to 8. :)

Thanks,
Namhyung

Re: [PATCH v3 1/5] perf cpumap: If the die_id is missing use socket/physical_package_id

[Namhyung Kim]

On Wed, Dec 18, 2024 at 09:42:27AM -0800, Ian Rogers wrote:
> On Wed, Dec 18, 2024 at 4:04 AM James Clark <james.clark@linaro.org> wrote:
> >
> >
> >
> > On 16/12/2024 11:24 pm, Ian Rogers wrote:
> > > An error value for a missing die_id may be written into things like
> > > the cpu_topology_map. As the topology needs to be fully written out,
> > > including the die_id, to allow perf.data file features to be aligned
> > > we can't allow error values to be written out. Instead base the
> > > missing die_id value off of the socket/physical_package_id assuming
> > > they correlate 1:1.
> > >
> > > Signed-off-by: Ian Rogers <irogers@google.com>
> > > ---
> > >   tools/perf/util/cpumap.c | 3 ++-
> > >   1 file changed, 2 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/tools/perf/util/cpumap.c b/tools/perf/util/cpumap.c
> > > index 27094211edd8..d362272f8466 100644
> > > --- a/tools/perf/util/cpumap.c
> > > +++ b/tools/perf/util/cpumap.c
> > > @@ -283,7 +283,8 @@ int cpu__get_die_id(struct perf_cpu cpu)
> > >   {
> > >       int value, ret = cpu__get_topology_int(cpu.cpu, "die_id", &value);
> > >
> > > -     return ret ?: value;
> > > +        /* If die_id is missing fallback on using the socket/physical_package_id. */
> > > +     return ret || value < 0 ? cpu__get_socket_id(cpu) : value;
> > >   }
> > >
> > >   struct aggr_cpu_id aggr_cpu_id__die(struct perf_cpu cpu, void *data)
> >
> > Hi Ian,
> >
> > I sent a fix for the same or a similar problem here [1]. For this one
> > I'm not sure why we'd want to use the socket ID for die when it's always
> > been 0 for not present. I wonder if this change is mingling two things:
> > fixing the negative error value appearing and replacing die with socket ID.
> >
> > Personally I would prefer to keep the 0 to fix the error value, that way
> > nobody gets surprised by the change.
> >
> > Also it looks like cpu__get_cluster_id() can suffer from the same issue,
> > and if we do it this way we should drop these as they aren't valid anymore:
> >
> >         /* There is no die_id on legacy system. */
> >         if (die < 0)
> >                 die = 0;
> 
> I think this breaks the assumption here:
> https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/tests/expr.c?h=perf-tools-next#n244

Hmm.. I'm not sure how it worked before.  The code is already there and
it just changed the condition from == -1 to < 0, right?

Thanks,
Namhyung


> hence using the number of packages. I think we need to make sure there
> is order otherwise we'll end up with "if no dies value present" or "if
> dies value is 0 ignore" everywhere. What is strange to me is that ARM
> may have the die_id present but have it contain the value -1.
> 
> > And last minor comment this could do with a fixes: 05be17eed774
> 
> Thanks,
> Ian
> 
> > [1]:
> > https://lore.kernel.org/linux-perf-users/20241218115552.912517-1-james.clark@linaro.org/T/#u
> >

Re: [PATCH v3 1/5] perf cpumap: If the die_id is missing use socket/physical_package_id

[Ian Rogers]

On Wed, Dec 18, 2024 at 9:32 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> On Wed, Dec 18, 2024 at 09:42:27AM -0800, Ian Rogers wrote:
> > On Wed, Dec 18, 2024 at 4:04 AM James Clark <james.clark@linaro.org> wrote:
> > >
> > >
> > >
> > > On 16/12/2024 11:24 pm, Ian Rogers wrote:
> > > > An error value for a missing die_id may be written into things like
> > > > the cpu_topology_map. As the topology needs to be fully written out,
> > > > including the die_id, to allow perf.data file features to be aligned
> > > > we can't allow error values to be written out. Instead base the
> > > > missing die_id value off of the socket/physical_package_id assuming
> > > > they correlate 1:1.
> > > >
> > > > Signed-off-by: Ian Rogers <irogers@google.com>
> > > > ---
> > > >   tools/perf/util/cpumap.c | 3 ++-
> > > >   1 file changed, 2 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/tools/perf/util/cpumap.c b/tools/perf/util/cpumap.c
> > > > index 27094211edd8..d362272f8466 100644
> > > > --- a/tools/perf/util/cpumap.c
> > > > +++ b/tools/perf/util/cpumap.c
> > > > @@ -283,7 +283,8 @@ int cpu__get_die_id(struct perf_cpu cpu)
> > > >   {
> > > >       int value, ret = cpu__get_topology_int(cpu.cpu, "die_id", &value);
> > > >
> > > > -     return ret ?: value;
> > > > +        /* If die_id is missing fallback on using the socket/physical_package_id. */
> > > > +     return ret || value < 0 ? cpu__get_socket_id(cpu) : value;
> > > >   }
> > > >
> > > >   struct aggr_cpu_id aggr_cpu_id__die(struct perf_cpu cpu, void *data)
> > >
> > > Hi Ian,
> > >
> > > I sent a fix for the same or a similar problem here [1]. For this one
> > > I'm not sure why we'd want to use the socket ID for die when it's always
> > > been 0 for not present. I wonder if this change is mingling two things:
> > > fixing the negative error value appearing and replacing die with socket ID.
> > >
> > > Personally I would prefer to keep the 0 to fix the error value, that way
> > > nobody gets surprised by the change.
> > >
> > > Also it looks like cpu__get_cluster_id() can suffer from the same issue,
> > > and if we do it this way we should drop these as they aren't valid anymore:
> > >
> > >         /* There is no die_id on legacy system. */
> > >         if (die < 0)
> > >                 die = 0;
> >
> > I think this breaks the assumption here:
> > https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/tests/expr.c?h=perf-tools-next#n244
>
> Hmm.. I'm not sure how it worked before.  The code is already there and
> it just changed the condition from == -1 to < 0, right?

You'd need to be testing on a multi-socket machine to see the issue.
If you had say a dual socket Ampere chip and the die_id was missing,
does it make sense for there to be two sockets/packages but only 1
die? I think it is best we assume 1 die per socket when the die_id is
missing, and to some crippled extent (because of the s390 workaround)
the expr test is doing the sanity check.

Thanks,
Ian

Re: [PATCH v3 1/5] perf cpumap: If the die_id is missing use socket/physical_package_id

[Namhyung Kim]

On Thu, Dec 19, 2024 at 09:20:39AM -0800, Ian Rogers wrote:
> On Wed, Dec 18, 2024 at 9:32 PM Namhyung Kim <namhyung@kernel.org> wrote:
> >
> > On Wed, Dec 18, 2024 at 09:42:27AM -0800, Ian Rogers wrote:
> > > On Wed, Dec 18, 2024 at 4:04 AM James Clark <james.clark@linaro.org> wrote:
> > > >
> > > >
> > > >
> > > > On 16/12/2024 11:24 pm, Ian Rogers wrote:
> > > > > An error value for a missing die_id may be written into things like
> > > > > the cpu_topology_map. As the topology needs to be fully written out,
> > > > > including the die_id, to allow perf.data file features to be aligned
> > > > > we can't allow error values to be written out. Instead base the
> > > > > missing die_id value off of the socket/physical_package_id assuming
> > > > > they correlate 1:1.
> > > > >
> > > > > Signed-off-by: Ian Rogers <irogers@google.com>
> > > > > ---
> > > > >   tools/perf/util/cpumap.c | 3 ++-
> > > > >   1 file changed, 2 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/tools/perf/util/cpumap.c b/tools/perf/util/cpumap.c
> > > > > index 27094211edd8..d362272f8466 100644
> > > > > --- a/tools/perf/util/cpumap.c
> > > > > +++ b/tools/perf/util/cpumap.c
> > > > > @@ -283,7 +283,8 @@ int cpu__get_die_id(struct perf_cpu cpu)
> > > > >   {
> > > > >       int value, ret = cpu__get_topology_int(cpu.cpu, "die_id", &value);
> > > > >
> > > > > -     return ret ?: value;
> > > > > +        /* If die_id is missing fallback on using the socket/physical_package_id. */
> > > > > +     return ret || value < 0 ? cpu__get_socket_id(cpu) : value;
> > > > >   }
> > > > >
> > > > >   struct aggr_cpu_id aggr_cpu_id__die(struct perf_cpu cpu, void *data)
> > > >
> > > > Hi Ian,
> > > >
> > > > I sent a fix for the same or a similar problem here [1]. For this one
> > > > I'm not sure why we'd want to use the socket ID for die when it's always
> > > > been 0 for not present. I wonder if this change is mingling two things:
> > > > fixing the negative error value appearing and replacing die with socket ID.
> > > >
> > > > Personally I would prefer to keep the 0 to fix the error value, that way
> > > > nobody gets surprised by the change.
> > > >
> > > > Also it looks like cpu__get_cluster_id() can suffer from the same issue,
> > > > and if we do it this way we should drop these as they aren't valid anymore:
> > > >
> > > >         /* There is no die_id on legacy system. */
> > > >         if (die < 0)
> > > >                 die = 0;
> > >
> > > I think this breaks the assumption here:
> > > https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/tests/expr.c?h=perf-tools-next#n244
> >
> > Hmm.. I'm not sure how it worked before.  The code is already there and
> > it just changed the condition from == -1 to < 0, right?
> 
> You'd need to be testing on a multi-socket machine to see the issue.
> If you had say a dual socket Ampere chip and the die_id was missing,
> does it make sense for there to be two sockets/packages but only 1
> die? I think it is best we assume 1 die per socket when the die_id is
> missing, and to some crippled extent (because of the s390 workaround)
> the expr test is doing the sanity check.

AFAICS die ID is always used with socket ID already so I guess it means
an ID inside a socket.

Thanks,
Namhyung

Re: [PATCH v3 1/5] perf cpumap: If the die_id is missing use socket/physical_package_id

[James Clark]

On 19/12/2024 9:53 pm, Namhyung Kim wrote:
> On Thu, Dec 19, 2024 at 09:20:39AM -0800, Ian Rogers wrote:
>> On Wed, Dec 18, 2024 at 9:32 PM Namhyung Kim <namhyung@kernel.org> wrote:
>>>
>>> On Wed, Dec 18, 2024 at 09:42:27AM -0800, Ian Rogers wrote:
>>>> On Wed, Dec 18, 2024 at 4:04 AM James Clark <james.clark@linaro.org> wrote:
>>>>>
>>>>>
>>>>>
>>>>> On 16/12/2024 11:24 pm, Ian Rogers wrote:
>>>>>> An error value for a missing die_id may be written into things like
>>>>>> the cpu_topology_map. As the topology needs to be fully written out,
>>>>>> including the die_id, to allow perf.data file features to be aligned
>>>>>> we can't allow error values to be written out. Instead base the
>>>>>> missing die_id value off of the socket/physical_package_id assuming
>>>>>> they correlate 1:1.
>>>>>>
>>>>>> Signed-off-by: Ian Rogers <irogers@google.com>
>>>>>> ---
>>>>>>    tools/perf/util/cpumap.c | 3 ++-
>>>>>>    1 file changed, 2 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/tools/perf/util/cpumap.c b/tools/perf/util/cpumap.c
>>>>>> index 27094211edd8..d362272f8466 100644
>>>>>> --- a/tools/perf/util/cpumap.c
>>>>>> +++ b/tools/perf/util/cpumap.c
>>>>>> @@ -283,7 +283,8 @@ int cpu__get_die_id(struct perf_cpu cpu)
>>>>>>    {
>>>>>>        int value, ret = cpu__get_topology_int(cpu.cpu, "die_id", &value);
>>>>>>
>>>>>> -     return ret ?: value;
>>>>>> +        /* If die_id is missing fallback on using the socket/physical_package_id. */
>>>>>> +     return ret || value < 0 ? cpu__get_socket_id(cpu) : value;
>>>>>>    }
>>>>>>
>>>>>>    struct aggr_cpu_id aggr_cpu_id__die(struct perf_cpu cpu, void *data)
>>>>>
>>>>> Hi Ian,
>>>>>
>>>>> I sent a fix for the same or a similar problem here [1]. For this one
>>>>> I'm not sure why we'd want to use the socket ID for die when it's always
>>>>> been 0 for not present. I wonder if this change is mingling two things:
>>>>> fixing the negative error value appearing and replacing die with socket ID.
>>>>>
>>>>> Personally I would prefer to keep the 0 to fix the error value, that way
>>>>> nobody gets surprised by the change.
>>>>>
>>>>> Also it looks like cpu__get_cluster_id() can suffer from the same issue,
>>>>> and if we do it this way we should drop these as they aren't valid anymore:
>>>>>
>>>>>          /* There is no die_id on legacy system. */
>>>>>          if (die < 0)
>>>>>                  die = 0;
>>>>
>>>> I think this breaks the assumption here:
>>>> https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/tests/expr.c?h=perf-tools-next#n244
 >>>>>> Hmm.. I'm not sure how it worked before.  The code is already 
there and
>>> it just changed the condition from == -1 to < 0, right?
>>
>> You'd need to be testing on a multi-socket machine to see the issue.
>> If you had say a dual socket Ampere chip and the die_id was missing,
>> does it make sense for there to be two sockets/packages but only 1
>> die? I think it is best we assume 1 die per socket when the die_id is
>> missing, and to some crippled extent (because of the s390 workaround)
>> the expr test is doing the sanity check.
> 
> AFAICS die ID is always used with socket ID already so I guess it means
> an ID inside a socket.
> 
> Thanks,
> Namhyung
> 

Don't we then need to update has_die_topology() to always return true if 
we're using socket ID as a placeholder? And the #num_dies expression 
should potentially become non-zero for consistency?

Seems like there are two options:

  - Fully support "no die reported"
   * -errno is saved into the topology header, but it's still converted
     to 0 for Perf stat aggregation and display
   * #num_dies == 0 as it currently is
   * Anywhere else die is used it's checked for error values first

  - Replace die with package
    * #num_dies == #num_packages
    * has_die_topology() == true
    * delete code to display die as 0 in perf stat

With this patch we get somewhere between the two.

Thanks
James

Re: [PATCH v3 1/5] perf cpumap: If the die_id is missing use socket/physical_package_id

[Ian Rogers]

On Fri, Dec 20, 2024 at 2:28 AM James Clark <james.clark@linaro.org> wrote:
>
> On 19/12/2024 9:53 pm, Namhyung Kim wrote:
> > On Thu, Dec 19, 2024 at 09:20:39AM -0800, Ian Rogers wrote:
> >> On Wed, Dec 18, 2024 at 9:32 PM Namhyung Kim <namhyung@kernel.org> wrote:
> >>>
> >>> On Wed, Dec 18, 2024 at 09:42:27AM -0800, Ian Rogers wrote:
> >>>> On Wed, Dec 18, 2024 at 4:04 AM James Clark <james.clark@linaro.org> wrote:
> >>>>>
> >>>>>
> >>>>>
> >>>>> On 16/12/2024 11:24 pm, Ian Rogers wrote:
> >>>>>> An error value for a missing die_id may be written into things like
> >>>>>> the cpu_topology_map. As the topology needs to be fully written out,
> >>>>>> including the die_id, to allow perf.data file features to be aligned
> >>>>>> we can't allow error values to be written out. Instead base the
> >>>>>> missing die_id value off of the socket/physical_package_id assuming
> >>>>>> they correlate 1:1.
> >>>>>>
> >>>>>> Signed-off-by: Ian Rogers <irogers@google.com>
> >>>>>> ---
> >>>>>>    tools/perf/util/cpumap.c | 3 ++-
> >>>>>>    1 file changed, 2 insertions(+), 1 deletion(-)
> >>>>>>
> >>>>>> diff --git a/tools/perf/util/cpumap.c b/tools/perf/util/cpumap.c
> >>>>>> index 27094211edd8..d362272f8466 100644
> >>>>>> --- a/tools/perf/util/cpumap.c
> >>>>>> +++ b/tools/perf/util/cpumap.c
> >>>>>> @@ -283,7 +283,8 @@ int cpu__get_die_id(struct perf_cpu cpu)
> >>>>>>    {
> >>>>>>        int value, ret = cpu__get_topology_int(cpu.cpu, "die_id", &value);
> >>>>>>
> >>>>>> -     return ret ?: value;
> >>>>>> +        /* If die_id is missing fallback on using the socket/physical_package_id. */
> >>>>>> +     return ret || value < 0 ? cpu__get_socket_id(cpu) : value;
> >>>>>>    }
> >>>>>>
> >>>>>>    struct aggr_cpu_id aggr_cpu_id__die(struct perf_cpu cpu, void *data)
> >>>>>
> >>>>> Hi Ian,
> >>>>>
> >>>>> I sent a fix for the same or a similar problem here [1]. For this one
> >>>>> I'm not sure why we'd want to use the socket ID for die when it's always
> >>>>> been 0 for not present. I wonder if this change is mingling two things:
> >>>>> fixing the negative error value appearing and replacing die with socket ID.
> >>>>>
> >>>>> Personally I would prefer to keep the 0 to fix the error value, that way
> >>>>> nobody gets surprised by the change.
> >>>>>
> >>>>> Also it looks like cpu__get_cluster_id() can suffer from the same issue,
> >>>>> and if we do it this way we should drop these as they aren't valid anymore:
> >>>>>
> >>>>>          /* There is no die_id on legacy system. */
> >>>>>          if (die < 0)
> >>>>>                  die = 0;
> >>>>
> >>>> I think this breaks the assumption here:
> >>>> https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/tests/expr.c?h=perf-tools-next#n244
>  >>>>>> Hmm.. I'm not sure how it worked before.  The code is already
> there and
> >>> it just changed the condition from == -1 to < 0, right?
> >>
> >> You'd need to be testing on a multi-socket machine to see the issue.
> >> If you had say a dual socket Ampere chip and the die_id was missing,
> >> does it make sense for there to be two sockets/packages but only 1
> >> die? I think it is best we assume 1 die per socket when the die_id is
> >> missing, and to some crippled extent (because of the s390 workaround)
> >> the expr test is doing the sanity check.
> >
> > AFAICS die ID is always used with socket ID already so I guess it means
> > an ID inside a socket.
> >
> > Thanks,
> > Namhyung
> >
>
> Don't we then need to update has_die_topology() to always return true if
> we're using socket ID as a placeholder? And the #num_dies expression
> should potentially become non-zero for consistency?
>
> Seems like there are two options:
>
>   - Fully support "no die reported"
>    * -errno is saved into the topology header, but it's still converted
>      to 0 for Perf stat aggregation and display
>    * #num_dies == 0 as it currently is
>    * Anywhere else die is used it's checked for error values first
>
>   - Replace die with package
>     * #num_dies == #num_packages
>     * has_die_topology() == true
>     * delete code to display die as 0 in perf stat
>
> With this patch we get somewhere between the two.

Thanks James, I'll try to do a larger cleanup in this area. My purpose
for sending the patches was the unaligned feature fields such as CPU
topology cause data alignment issues in the perf.data file. When these
occur we have builds that turn undefined behavior (such as bad
alignment) into illegal instructions and we see widespread test
regressions. The extended pipe tests in v6.12 did this. So the rough
flow of what I've been going through is:
1) CPU topology has perf.data file alignment issues.
2) aligning the CPU topology breaks "old" perf.data files as the size
of the feature is used to detect whether die_id, etc. are present.
3) I've discovered a latent issue that ARM and S390 aren't exposing
die_id and falling into the "old" perf.data file case.
4) let's write out everything in the CPU topology even on ARM and S390
so that the old file test can be just that, but new files have all the
topology.
5) the CPU topology is a mish-mash of skipped tests and things working
not particularly by design.
6) we need to come up with values for die_id, etc. to fix the topology
but these things aren't well defined. I'm reminded of past discussions
of logical ids (unique per core/die/..) vs the normal ids perf users
which may be duplicated across cores.
Anyway, I'll see what I can do make this have a better API, etc.
inspired from your and Namhyung's feedback.

Thanks,
Ian