* [PATCH v1 1/2] perf build-id: Align records to 8 rather than 64 bytes
@ 2024-08-13 4:34 Ian Rogers
2024-08-13 4:34 ` [PATCH v1 2/2] perf test: Expand pipe/inject test Ian Rogers
2024-08-16 1:44 ` [PATCH v1 1/2] perf build-id: Align records to 8 rather than 64 bytes Ian Rogers
0 siblings, 2 replies; 3+ messages in thread
From: Ian Rogers @ 2024-08-13 4:34 UTC (permalink / raw)
To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Namhyung Kim, Mark Rutland, Alexander Shishkin, Jiri Olsa,
Ian Rogers, Adrian Hunter, Kan Liang, Sun Haiyong, Yanteng Si,
linux-perf-users, linux-kernel
Events are 8-byte aligned in perf.data files, NAME_ALIGN is 64-bytes
which is excessive given the alignment needs. Move the addition the
sizeof so that the alignment considers that the rest of the event is
4-byte aligned.
```
struct perf_record_header_build_id {
struct perf_event_header header; /* 0 8 */
pid_t pid; /* 8 4 */
union {
__u8 build_id[24]; /* 12 24 */
struct {
__u8 data[20]; /* 12 20 */
__u8 size; /* 32 1 */
__u8 reserved1__; /* 33 1 */
__u16 reserved2__; /* 34 2 */
}; /* 12 24 */
}; /* 12 24 */
char filename[]; /* 36 0 */
/* size: 36, cachelines: 1, members: 4 */
/* last cacheline: 36 bytes */
};
```
Signed-off-by: Ian Rogers <irogers@google.com>
---
tools/perf/util/build-id.c | 6 +++---
tools/perf/util/synthetic-events.c | 6 +++---
2 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/tools/perf/util/build-id.c b/tools/perf/util/build-id.c
index 83a1581e8cf1..8fea17d04468 100644
--- a/tools/perf/util/build-id.c
+++ b/tools/perf/util/build-id.c
@@ -309,8 +309,8 @@ static int write_buildid(const char *name, size_t name_len, struct build_id *bid
struct perf_record_header_build_id b;
size_t len;
- len = name_len + 1;
- len = PERF_ALIGN(len, NAME_ALIGN);
+ len = sizeof(b) + name_len + 1;
+ len = PERF_ALIGN(len, sizeof(u64));
memset(&b, 0, sizeof(b));
memcpy(&b.data, bid->data, bid->size);
@@ -318,7 +318,7 @@ static int write_buildid(const char *name, size_t name_len, struct build_id *bid
misc |= PERF_RECORD_MISC_BUILD_ID_SIZE;
b.pid = pid;
b.header.misc = misc;
- b.header.size = sizeof(b) + len;
+ b.header.size = len;
err = do_write(fd, &b, sizeof(b));
if (err < 0)
diff --git a/tools/perf/util/synthetic-events.c b/tools/perf/util/synthetic-events.c
index 5498048f56ea..a52b85bcb6b6 100644
--- a/tools/perf/util/synthetic-events.c
+++ b/tools/perf/util/synthetic-events.c
@@ -2236,14 +2236,14 @@ int perf_event__synthesize_build_id(struct perf_tool *tool, struct dso *pos, u16
memset(&ev, 0, sizeof(ev));
- len = dso__long_name_len(pos) + 1;
- len = PERF_ALIGN(len, NAME_ALIGN);
+ len = sizeof(ev.build_id) + dso__long_name_len(pos) + 1;
+ len = PERF_ALIGN(len, sizeof(u64));
ev.build_id.size = min(dso__bid(pos)->size, sizeof(dso__bid(pos)->data));
memcpy(&ev.build_id.build_id, dso__bid(pos)->data, ev.build_id.size);
ev.build_id.header.type = PERF_RECORD_HEADER_BUILD_ID;
ev.build_id.header.misc = misc | PERF_RECORD_MISC_BUILD_ID_SIZE;
ev.build_id.pid = machine->pid;
- ev.build_id.header.size = sizeof(ev.build_id) + len;
+ ev.build_id.header.size = len;
memcpy(&ev.build_id.filename, dso__long_name(pos), dso__long_name_len(pos));
return process(tool, &ev, NULL, machine);
--
2.46.0.76.ge559c4bf1a-goog
^ permalink raw reply related [flat|nested] 3+ messages in thread* [PATCH v1 2/2] perf test: Expand pipe/inject test
2024-08-13 4:34 [PATCH v1 1/2] perf build-id: Align records to 8 rather than 64 bytes Ian Rogers
@ 2024-08-13 4:34 ` Ian Rogers
2024-08-16 1:44 ` [PATCH v1 1/2] perf build-id: Align records to 8 rather than 64 bytes Ian Rogers
1 sibling, 0 replies; 3+ messages in thread
From: Ian Rogers @ 2024-08-13 4:34 UTC (permalink / raw)
To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Namhyung Kim, Mark Rutland, Alexander Shishkin, Jiri Olsa,
Ian Rogers, Adrian Hunter, Kan Liang, Sun Haiyong, Yanteng Si,
linux-perf-users, linux-kernel
Test recording of call-graphs and injecting --build-all. Add/expand
trap handler.
Signed-off-by: Ian Rogers <irogers@google.com>
---
tools/perf/tests/shell/pipe_test.sh | 101 ++++++++++++++++++++++------
1 file changed, 79 insertions(+), 22 deletions(-)
diff --git a/tools/perf/tests/shell/pipe_test.sh b/tools/perf/tests/shell/pipe_test.sh
index a78d35d2cff0..ad10012fdc29 100755
--- a/tools/perf/tests/shell/pipe_test.sh
+++ b/tools/perf/tests/shell/pipe_test.sh
@@ -1,4 +1,4 @@
-#!/bin/sh
+#!/bin/bash
# perf pipe recording and injection test
# SPDX-License-Identifier: GPL-2.0
@@ -12,30 +12,87 @@ skip_test_missing_symbol ${sym}
data=$(mktemp /tmp/perf.data.XXXXXX)
prog="perf test -w noploop"
-task="perf"
+err=0
-if ! perf record -e task-clock:u -o - ${prog} | perf report -i - --task | grep ${task}; then
- echo "cannot find the test file in the perf report"
- exit 1
-fi
+set -e
-if ! perf record -e task-clock:u -o - ${prog} | perf inject -b | perf report -i - | grep ${sym}; then
- echo "cannot find noploop function in pipe #1"
- exit 1
-fi
+cleanup() {
+ rm -rf "${data}"
+ rm -rf "${data}".old
-perf record -e task-clock:u -o - ${prog} | perf inject -b -o ${data}
-if ! perf report -i ${data} | grep ${sym}; then
- echo "cannot find noploop function in pipe #2"
- exit 1
-fi
+ trap - EXIT TERM INT
+}
-perf record -e task-clock:u -o ${data} ${prog}
-if ! perf inject -b -i ${data} | perf report -i - | grep ${sym}; then
- echo "cannot find noploop function in pipe #3"
- exit 1
-fi
+trap_cleanup() {
+ echo "Unexpected signal in ${FUNCNAME[1]}"
+ cleanup
+ exit 1
+}
+trap trap_cleanup EXIT TERM INT
+test_record_report() {
+ echo
+ echo "Record+report pipe test"
+
+ task="perf"
+ if ! perf record -e task-clock:u -o - ${prog} | perf report -i - --task | grep -q ${task}
+ then
+ echo "Record+report pipe test [Failed - cannot find the test file in the perf report #1]"
+ err=1
+ return
+ fi
+
+ if ! perf record -g -e task-clock:u -o - ${prog} | perf report -i - --task | grep -q ${task}
+ then
+ echo "Record+report pipe test [Failed - cannot find the test file in the perf report #2]"
+ err=1
+ return
+ fi
+
+ echo "Record+report pipe test [Success]"
+}
+
+test_inject_bids() {
+ inject_opt=$1
+
+ echo
+ echo "Inject ${inject_opt} build-ids test"
+
+ if ! perf record -e task-clock:u -o - ${prog} | perf inject ${inject_opt}| perf report -i - | grep -q ${sym}
+ then
+ echo "Inject build-ids test [Failed - cannot find noploop function in pipe #1]"
+ err=1
+ return
+ fi
+
+ if ! perf record -g -e task-clock:u -o - ${prog} | perf inject ${inject_opt} | perf report -i - | grep -q ${sym}
+ then
+ echo "Inject ${inject_opt} build-ids test [Failed - cannot find noploop function in pipe #2]"
+ err=1
+ return
+ fi
+
+ perf record -e task-clock:u -o - ${prog} | perf inject ${inject_opt} -o ${data}
+ if ! perf report -i ${data} | grep -q ${sym}; then
+ echo "Inject ${inject_opt} build-ids test [Failed - cannot find noploop function in pipe #3]"
+ err=1
+ return
+ fi
+
+ perf record -e task-clock:u -o ${data} ${prog}
+ if ! perf inject ${inject_opt} -i ${data} | perf report -i - | grep -q ${sym}; then
+ echo "Inject ${inject_opt} build-ids test [Failed - cannot find noploop function in pipe #4]"
+ err=1
+ return
+ fi
+
+ echo "Inject ${inject_opt} build-ids test [Success]"
+}
+
+test_record_report
+test_inject_bids -b
+test_inject_bids --buildid-all
+
+cleanup
+exit $err
-rm -f ${data} ${data}.old
-exit 0
--
2.46.0.76.ge559c4bf1a-goog
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH v1 1/2] perf build-id: Align records to 8 rather than 64 bytes
2024-08-13 4:34 [PATCH v1 1/2] perf build-id: Align records to 8 rather than 64 bytes Ian Rogers
2024-08-13 4:34 ` [PATCH v1 2/2] perf test: Expand pipe/inject test Ian Rogers
@ 2024-08-16 1:44 ` Ian Rogers
1 sibling, 0 replies; 3+ messages in thread
From: Ian Rogers @ 2024-08-16 1:44 UTC (permalink / raw)
To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Namhyung Kim, Mark Rutland, Alexander Shishkin, Jiri Olsa,
Ian Rogers, Adrian Hunter, Kan Liang, Sun Haiyong, Yanteng Si,
linux-perf-users, linux-kernel
On Mon, Aug 12, 2024 at 9:34 PM Ian Rogers <irogers@google.com> wrote:
>
> Events are 8-byte aligned in perf.data files, NAME_ALIGN is 64-bytes
> which is excessive given the alignment needs. Move the addition the
> sizeof so that the alignment considers that the rest of the event is
> 4-byte aligned.
>
> ```
> struct perf_record_header_build_id {
> struct perf_event_header header; /* 0 8 */
> pid_t pid; /* 8 4 */
> union {
> __u8 build_id[24]; /* 12 24 */
> struct {
> __u8 data[20]; /* 12 20 */
> __u8 size; /* 32 1 */
> __u8 reserved1__; /* 33 1 */
> __u16 reserved2__; /* 34 2 */
> }; /* 12 24 */
> }; /* 12 24 */
> char filename[]; /* 36 0 */
>
> /* size: 36, cachelines: 1, members: 4 */
> /* last cacheline: 36 bytes */
> };
> ```
>
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
> tools/perf/util/build-id.c | 6 +++---
> tools/perf/util/synthetic-events.c | 6 +++---
> 2 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/tools/perf/util/build-id.c b/tools/perf/util/build-id.c
> index 83a1581e8cf1..8fea17d04468 100644
> --- a/tools/perf/util/build-id.c
> +++ b/tools/perf/util/build-id.c
> @@ -309,8 +309,8 @@ static int write_buildid(const char *name, size_t name_len, struct build_id *bid
> struct perf_record_header_build_id b;
> size_t len;
>
> - len = name_len + 1;
> - len = PERF_ALIGN(len, NAME_ALIGN);
> + len = sizeof(b) + name_len + 1;
> + len = PERF_ALIGN(len, sizeof(u64));
>
> memset(&b, 0, sizeof(b));
> memcpy(&b.data, bid->data, bid->size);
> @@ -318,7 +318,7 @@ static int write_buildid(const char *name, size_t name_len, struct build_id *bid
> misc |= PERF_RECORD_MISC_BUILD_ID_SIZE;
> b.pid = pid;
> b.header.misc = misc;
> - b.header.size = sizeof(b) + len;
> + b.header.size = len;
>
> err = do_write(fd, &b, sizeof(b));
> if (err < 0)
> diff --git a/tools/perf/util/synthetic-events.c b/tools/perf/util/synthetic-events.c
> index 5498048f56ea..a52b85bcb6b6 100644
> --- a/tools/perf/util/synthetic-events.c
> +++ b/tools/perf/util/synthetic-events.c
> @@ -2236,14 +2236,14 @@ int perf_event__synthesize_build_id(struct perf_tool *tool, struct dso *pos, u16
>
> memset(&ev, 0, sizeof(ev));
>
> - len = dso__long_name_len(pos) + 1;
> - len = PERF_ALIGN(len, NAME_ALIGN);
> + len = sizeof(ev.build_id) + dso__long_name_len(pos) + 1;
> + len = PERF_ALIGN(len, sizeof(u64));
> ev.build_id.size = min(dso__bid(pos)->size, sizeof(dso__bid(pos)->data));
> memcpy(&ev.build_id.build_id, dso__bid(pos)->data, ev.build_id.size);
> ev.build_id.header.type = PERF_RECORD_HEADER_BUILD_ID;
> ev.build_id.header.misc = misc | PERF_RECORD_MISC_BUILD_ID_SIZE;
> ev.build_id.pid = machine->pid;
> - ev.build_id.header.size = sizeof(ev.build_id) + len;
> + ev.build_id.header.size = len;
> memcpy(&ev.build_id.filename, dso__long_name(pos), dso__long_name_len(pos));
So I think there is a bigger issue here and I'm working on a better
fix. The issue is that the synthesized build_id event doesn't have a
sample id inserted afterward (note, no adding of id_hdr_size above).
By over aligning the filename it is quite likely the sample id reading
is just reading zeros in particular for the time stamp. Not having a
sample id means the build_id events are unordered, this will break if
the data file has two dsos with the same filename but different build
ids - as happens when a file is replaced. The better fix is to 8 byte
align the event and insert a sample id.
Thanks,
Ian
> return process(tool, &ev, NULL, machine);
> --
> 2.46.0.76.ge559c4bf1a-goog
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2024-08-16 1:44 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-08-13 4:34 [PATCH v1 1/2] perf build-id: Align records to 8 rather than 64 bytes Ian Rogers
2024-08-13 4:34 ` [PATCH v1 2/2] perf test: Expand pipe/inject test Ian Rogers
2024-08-16 1:44 ` [PATCH v1 1/2] perf build-id: Align records to 8 rather than 64 bytes Ian Rogers
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).