| Message ID | 20230228093206.821563-8-jolsa@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mm/bpf/perf: Store build id in inode object | expand |
On Tue, Feb 28, 2023 at 1:33 AM Jiri Olsa <jolsa@kernel.org> wrote: > > Replacing extract_build_id with read_build_id that parses out > build id directly from elf without using readelf tool. > > Signed-off-by: Jiri Olsa <jolsa@kernel.org> > --- small nit below, but looks good. Thanks for clean up! Acked-by: Andrii Nakryiko <andrii@kernel.org> > .../bpf/prog_tests/stacktrace_build_id.c | 19 ++++++-------- > .../bpf/prog_tests/stacktrace_build_id_nmi.c | 17 +++++-------- > tools/testing/selftests/bpf/test_progs.c | 25 ------------------- > tools/testing/selftests/bpf/test_progs.h | 1 - > 4 files changed, 13 insertions(+), 49 deletions(-) > > diff --git a/tools/testing/selftests/bpf/prog_tests/stacktrace_build_id.c b/tools/testing/selftests/bpf/prog_tests/stacktrace_build_id.c > index 9ad09a6c538a..9e4b76ee356f 100644 > --- a/tools/testing/selftests/bpf/prog_tests/stacktrace_build_id.c > +++ b/tools/testing/selftests/bpf/prog_tests/stacktrace_build_id.c > @@ -7,13 +7,12 @@ void test_stacktrace_build_id(void) > > int control_map_fd, stackid_hmap_fd, stackmap_fd, stack_amap_fd; > struct test_stacktrace_build_id *skel; > - int err, stack_trace_len; > + int err, stack_trace_len, build_id_size; > __u32 key, prev_key, val, duration = 0; > - char buf[256]; > - int i, j; > + char buf[BPF_BUILD_ID_SIZE]; > struct bpf_stack_build_id id_offs[PERF_MAX_STACK_DEPTH]; > int build_id_matches = 0; > - int retry = 1; > + int i, retry = 1; > > retry: > skel = test_stacktrace_build_id__open_and_load(); > @@ -52,9 +51,10 @@ void test_stacktrace_build_id(void) > "err %d errno %d\n", err, errno)) > goto cleanup; > > - err = extract_build_id(buf, 256); > + build_id_size = read_build_id("./urandom_read", buf); nit: "urandom_read" vs "./urandom_read" matters only when executing binary, not when opening a file. So all these "./" just creates unnecessary confusion, IMO > + err = build_id_size < 0 ? build_id_size : 0; > > - if (CHECK(err, "get build_id with readelf", > + if (CHECK(err, "read_build_id", > "err %d errno %d\n", err, errno)) > goto cleanup; > > @@ -64,8 +64,6 @@ void test_stacktrace_build_id(void) > goto cleanup; > > do { > - char build_id[64]; > - > err = bpf_map_lookup_elem(stackmap_fd, &key, id_offs); > if (CHECK(err, "lookup_elem from stackmap", > "err %d, errno %d\n", err, errno)) > @@ -73,10 +71,7 @@ void test_stacktrace_build_id(void) > for (i = 0; i < PERF_MAX_STACK_DEPTH; ++i) > if (id_offs[i].status == BPF_STACK_BUILD_ID_VALID && > id_offs[i].offset != 0) { > - for (j = 0; j < 20; ++j) > - sprintf(build_id + 2 * j, "%02x", > - id_offs[i].build_id[j] & 0xff); > - if (strstr(buf, build_id) != NULL) > + if (memcmp(buf, id_offs[i].build_id, build_id_size) == 0) > build_id_matches = 1; > } > prev_key = key; > diff --git a/tools/testing/selftests/bpf/prog_tests/stacktrace_build_id_nmi.c b/tools/testing/selftests/bpf/prog_tests/stacktrace_build_id_nmi.c > index f4ea1a215ce4..8d84149ebcc7 100644 > --- a/tools/testing/selftests/bpf/prog_tests/stacktrace_build_id_nmi.c > +++ b/tools/testing/selftests/bpf/prog_tests/stacktrace_build_id_nmi.c > @@ -28,11 +28,10 @@ void test_stacktrace_build_id_nmi(void) > .config = PERF_COUNT_HW_CPU_CYCLES, > }; > __u32 key, prev_key, val, duration = 0; > - char buf[256]; > - int i, j; > + char buf[BPF_BUILD_ID_SIZE]; > struct bpf_stack_build_id id_offs[PERF_MAX_STACK_DEPTH]; > - int build_id_matches = 0; > - int retry = 1; > + int build_id_matches = 0, build_id_size; > + int i, retry = 1; > > attr.sample_freq = read_perf_max_sample_freq(); > > @@ -94,7 +93,8 @@ void test_stacktrace_build_id_nmi(void) > "err %d errno %d\n", err, errno)) > goto cleanup; > > - err = extract_build_id(buf, 256); > + build_id_size = read_build_id("./urandom_read", buf); > + err = build_id_size < 0 ? build_id_size : 0; > > if (CHECK(err, "get build_id with readelf", > "err %d errno %d\n", err, errno)) > @@ -106,8 +106,6 @@ void test_stacktrace_build_id_nmi(void) > goto cleanup; > > do { > - char build_id[64]; > - > err = bpf_map__lookup_elem(skel->maps.stackmap, &key, sizeof(key), > id_offs, sizeof(id_offs), 0); > if (CHECK(err, "lookup_elem from stackmap", > @@ -116,10 +114,7 @@ void test_stacktrace_build_id_nmi(void) > for (i = 0; i < PERF_MAX_STACK_DEPTH; ++i) > if (id_offs[i].status == BPF_STACK_BUILD_ID_VALID && > id_offs[i].offset != 0) { > - for (j = 0; j < 20; ++j) > - sprintf(build_id + 2 * j, "%02x", > - id_offs[i].build_id[j] & 0xff); > - if (strstr(buf, build_id) != NULL) > + if (memcmp(buf, id_offs[i].build_id, build_id_size) == 0) > build_id_matches = 1; > } > prev_key = key; > diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c > index 6d5e3022c75f..9813d53c4878 100644 > --- a/tools/testing/selftests/bpf/test_progs.c > +++ b/tools/testing/selftests/bpf/test_progs.c > @@ -591,31 +591,6 @@ int compare_stack_ips(int smap_fd, int amap_fd, int stack_trace_len) > return err; > } > > -int extract_build_id(char *build_id, size_t size) > -{ > - FILE *fp; > - char *line = NULL; > - size_t len = 0; > - > - fp = popen("readelf -n ./urandom_read | grep 'Build ID'", "r"); > - if (fp == NULL) > - return -1; > - > - if (getline(&line, &len, fp) == -1) > - goto err; > - pclose(fp); > - > - if (len > size) > - len = size; > - memcpy(build_id, line, len); > - build_id[len] = '\0'; > - free(line); > - return 0; > -err: > - pclose(fp); > - return -1; > -} > - > static int finit_module(int fd, const char *param_values, int flags) > { > return syscall(__NR_finit_module, fd, param_values, flags); > diff --git a/tools/testing/selftests/bpf/test_progs.h b/tools/testing/selftests/bpf/test_progs.h > index 9fbdc57c5b57..3825c2797a4b 100644 > --- a/tools/testing/selftests/bpf/test_progs.h > +++ b/tools/testing/selftests/bpf/test_progs.h > @@ -404,7 +404,6 @@ static inline void *u64_to_ptr(__u64 ptr) > int bpf_find_map(const char *test, struct bpf_object *obj, const char *name); > int compare_map_keys(int map1_fd, int map2_fd); > int compare_stack_ips(int smap_fd, int amap_fd, int stack_trace_len); > -int extract_build_id(char *build_id, size_t size); > int kern_sync_rcu(void); > int trigger_module_test_read(int read_sz); > int trigger_module_test_write(int write_sz); > -- > 2.39.2 >
On Tue, Mar 07, 2023 at 05:26:32PM -0800, Andrii Nakryiko wrote: > On Tue, Feb 28, 2023 at 1:33 AM Jiri Olsa <jolsa@kernel.org> wrote: > > > > Replacing extract_build_id with read_build_id that parses out > > build id directly from elf without using readelf tool. > > > > Signed-off-by: Jiri Olsa <jolsa@kernel.org> > > --- > > small nit below, but looks good. Thanks for clean up! > > Acked-by: Andrii Nakryiko <andrii@kernel.org> > > > .../bpf/prog_tests/stacktrace_build_id.c | 19 ++++++-------- > > .../bpf/prog_tests/stacktrace_build_id_nmi.c | 17 +++++-------- > > tools/testing/selftests/bpf/test_progs.c | 25 ------------------- > > tools/testing/selftests/bpf/test_progs.h | 1 - > > 4 files changed, 13 insertions(+), 49 deletions(-) > > > > diff --git a/tools/testing/selftests/bpf/prog_tests/stacktrace_build_id.c b/tools/testing/selftests/bpf/prog_tests/stacktrace_build_id.c > > index 9ad09a6c538a..9e4b76ee356f 100644 > > --- a/tools/testing/selftests/bpf/prog_tests/stacktrace_build_id.c > > +++ b/tools/testing/selftests/bpf/prog_tests/stacktrace_build_id.c > > @@ -7,13 +7,12 @@ void test_stacktrace_build_id(void) > > > > int control_map_fd, stackid_hmap_fd, stackmap_fd, stack_amap_fd; > > struct test_stacktrace_build_id *skel; > > - int err, stack_trace_len; > > + int err, stack_trace_len, build_id_size; > > __u32 key, prev_key, val, duration = 0; > > - char buf[256]; > > - int i, j; > > + char buf[BPF_BUILD_ID_SIZE]; > > struct bpf_stack_build_id id_offs[PERF_MAX_STACK_DEPTH]; > > int build_id_matches = 0; > > - int retry = 1; > > + int i, retry = 1; > > > > retry: > > skel = test_stacktrace_build_id__open_and_load(); > > @@ -52,9 +51,10 @@ void test_stacktrace_build_id(void) > > "err %d errno %d\n", err, errno)) > > goto cleanup; > > > > - err = extract_build_id(buf, 256); > > + build_id_size = read_build_id("./urandom_read", buf); > > nit: "urandom_read" vs "./urandom_read" matters only when executing > binary, not when opening a file. So all these "./" just creates > unnecessary confusion, IMO right, I'll remove it also in the other test thanks, jirka > > > + err = build_id_size < 0 ? build_id_size : 0; > > > > - if (CHECK(err, "get build_id with readelf", > > + if (CHECK(err, "read_build_id", > > "err %d errno %d\n", err, errno)) > > goto cleanup; > > > > @@ -64,8 +64,6 @@ void test_stacktrace_build_id(void) > > goto cleanup; > > > > do { > > - char build_id[64]; > > - > > err = bpf_map_lookup_elem(stackmap_fd, &key, id_offs); > > if (CHECK(err, "lookup_elem from stackmap", > > "err %d, errno %d\n", err, errno)) > > @@ -73,10 +71,7 @@ void test_stacktrace_build_id(void) > > for (i = 0; i < PERF_MAX_STACK_DEPTH; ++i) > > if (id_offs[i].status == BPF_STACK_BUILD_ID_VALID && > > id_offs[i].offset != 0) { > > - for (j = 0; j < 20; ++j) > > - sprintf(build_id + 2 * j, "%02x", > > - id_offs[i].build_id[j] & 0xff); > > - if (strstr(buf, build_id) != NULL) > > + if (memcmp(buf, id_offs[i].build_id, build_id_size) == 0) > > build_id_matches = 1; > > } > > prev_key = key; > > diff --git a/tools/testing/selftests/bpf/prog_tests/stacktrace_build_id_nmi.c b/tools/testing/selftests/bpf/prog_tests/stacktrace_build_id_nmi.c > > index f4ea1a215ce4..8d84149ebcc7 100644 > > --- a/tools/testing/selftests/bpf/prog_tests/stacktrace_build_id_nmi.c > > +++ b/tools/testing/selftests/bpf/prog_tests/stacktrace_build_id_nmi.c > > @@ -28,11 +28,10 @@ void test_stacktrace_build_id_nmi(void) > > .config = PERF_COUNT_HW_CPU_CYCLES, > > }; > > __u32 key, prev_key, val, duration = 0; > > - char buf[256]; > > - int i, j; > > + char buf[BPF_BUILD_ID_SIZE]; > > struct bpf_stack_build_id id_offs[PERF_MAX_STACK_DEPTH]; > > - int build_id_matches = 0; > > - int retry = 1; > > + int build_id_matches = 0, build_id_size; > > + int i, retry = 1; > > > > attr.sample_freq = read_perf_max_sample_freq(); > > > > @@ -94,7 +93,8 @@ void test_stacktrace_build_id_nmi(void) > > "err %d errno %d\n", err, errno)) > > goto cleanup; > > > > - err = extract_build_id(buf, 256); > > + build_id_size = read_build_id("./urandom_read", buf); > > + err = build_id_size < 0 ? build_id_size : 0; > > > > if (CHECK(err, "get build_id with readelf", > > "err %d errno %d\n", err, errno)) > > @@ -106,8 +106,6 @@ void test_stacktrace_build_id_nmi(void) > > goto cleanup; > > > > do { > > - char build_id[64]; > > - > > err = bpf_map__lookup_elem(skel->maps.stackmap, &key, sizeof(key), > > id_offs, sizeof(id_offs), 0); > > if (CHECK(err, "lookup_elem from stackmap", > > @@ -116,10 +114,7 @@ void test_stacktrace_build_id_nmi(void) > > for (i = 0; i < PERF_MAX_STACK_DEPTH; ++i) > > if (id_offs[i].status == BPF_STACK_BUILD_ID_VALID && > > id_offs[i].offset != 0) { > > - for (j = 0; j < 20; ++j) > > - sprintf(build_id + 2 * j, "%02x", > > - id_offs[i].build_id[j] & 0xff); > > - if (strstr(buf, build_id) != NULL) > > + if (memcmp(buf, id_offs[i].build_id, build_id_size) == 0) > > build_id_matches = 1; > > } > > prev_key = key; > > diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c > > index 6d5e3022c75f..9813d53c4878 100644 > > --- a/tools/testing/selftests/bpf/test_progs.c > > +++ b/tools/testing/selftests/bpf/test_progs.c > > @@ -591,31 +591,6 @@ int compare_stack_ips(int smap_fd, int amap_fd, int stack_trace_len) > > return err; > > } > > > > -int extract_build_id(char *build_id, size_t size) > > -{ > > - FILE *fp; > > - char *line = NULL; > > - size_t len = 0; > > - > > - fp = popen("readelf -n ./urandom_read | grep 'Build ID'", "r"); > > - if (fp == NULL) > > - return -1; > > - > > - if (getline(&line, &len, fp) == -1) > > - goto err; > > - pclose(fp); > > - > > - if (len > size) > > - len = size; > > - memcpy(build_id, line, len); > > - build_id[len] = '\0'; > > - free(line); > > - return 0; > > -err: > > - pclose(fp); > > - return -1; > > -} > > - > > static int finit_module(int fd, const char *param_values, int flags) > > { > > return syscall(__NR_finit_module, fd, param_values, flags); > > diff --git a/tools/testing/selftests/bpf/test_progs.h b/tools/testing/selftests/bpf/test_progs.h > > index 9fbdc57c5b57..3825c2797a4b 100644 > > --- a/tools/testing/selftests/bpf/test_progs.h > > +++ b/tools/testing/selftests/bpf/test_progs.h > > @@ -404,7 +404,6 @@ static inline void *u64_to_ptr(__u64 ptr) > > int bpf_find_map(const char *test, struct bpf_object *obj, const char *name); > > int compare_map_keys(int map1_fd, int map2_fd); > > int compare_stack_ips(int smap_fd, int amap_fd, int stack_trace_len); > > -int extract_build_id(char *build_id, size_t size); > > int kern_sync_rcu(void); > > int trigger_module_test_read(int read_sz); > > int trigger_module_test_write(int write_sz); > > -- > > 2.39.2 > >
diff --git a/tools/testing/selftests/bpf/prog_tests/stacktrace_build_id.c b/tools/testing/selftests/bpf/prog_tests/stacktrace_build_id.c index 9ad09a6c538a..9e4b76ee356f 100644 --- a/tools/testing/selftests/bpf/prog_tests/stacktrace_build_id.c +++ b/tools/testing/selftests/bpf/prog_tests/stacktrace_build_id.c @@ -7,13 +7,12 @@ void test_stacktrace_build_id(void) int control_map_fd, stackid_hmap_fd, stackmap_fd, stack_amap_fd; struct test_stacktrace_build_id *skel; - int err, stack_trace_len; + int err, stack_trace_len, build_id_size; __u32 key, prev_key, val, duration = 0; - char buf[256]; - int i, j; + char buf[BPF_BUILD_ID_SIZE]; struct bpf_stack_build_id id_offs[PERF_MAX_STACK_DEPTH]; int build_id_matches = 0; - int retry = 1; + int i, retry = 1; retry: skel = test_stacktrace_build_id__open_and_load(); @@ -52,9 +51,10 @@ void test_stacktrace_build_id(void) "err %d errno %d\n", err, errno)) goto cleanup; - err = extract_build_id(buf, 256); + build_id_size = read_build_id("./urandom_read", buf); + err = build_id_size < 0 ? build_id_size : 0; - if (CHECK(err, "get build_id with readelf", + if (CHECK(err, "read_build_id", "err %d errno %d\n", err, errno)) goto cleanup; @@ -64,8 +64,6 @@ void test_stacktrace_build_id(void) goto cleanup; do { - char build_id[64]; - err = bpf_map_lookup_elem(stackmap_fd, &key, id_offs); if (CHECK(err, "lookup_elem from stackmap", "err %d, errno %d\n", err, errno)) @@ -73,10 +71,7 @@ void test_stacktrace_build_id(void) for (i = 0; i < PERF_MAX_STACK_DEPTH; ++i) if (id_offs[i].status == BPF_STACK_BUILD_ID_VALID && id_offs[i].offset != 0) { - for (j = 0; j < 20; ++j) - sprintf(build_id + 2 * j, "%02x", - id_offs[i].build_id[j] & 0xff); - if (strstr(buf, build_id) != NULL) + if (memcmp(buf, id_offs[i].build_id, build_id_size) == 0) build_id_matches = 1; } prev_key = key; diff --git a/tools/testing/selftests/bpf/prog_tests/stacktrace_build_id_nmi.c b/tools/testing/selftests/bpf/prog_tests/stacktrace_build_id_nmi.c index f4ea1a215ce4..8d84149ebcc7 100644 --- a/tools/testing/selftests/bpf/prog_tests/stacktrace_build_id_nmi.c +++ b/tools/testing/selftests/bpf/prog_tests/stacktrace_build_id_nmi.c @@ -28,11 +28,10 @@ void test_stacktrace_build_id_nmi(void) .config = PERF_COUNT_HW_CPU_CYCLES, }; __u32 key, prev_key, val, duration = 0; - char buf[256]; - int i, j; + char buf[BPF_BUILD_ID_SIZE]; struct bpf_stack_build_id id_offs[PERF_MAX_STACK_DEPTH]; - int build_id_matches = 0; - int retry = 1; + int build_id_matches = 0, build_id_size; + int i, retry = 1; attr.sample_freq = read_perf_max_sample_freq(); @@ -94,7 +93,8 @@ void test_stacktrace_build_id_nmi(void) "err %d errno %d\n", err, errno)) goto cleanup; - err = extract_build_id(buf, 256); + build_id_size = read_build_id("./urandom_read", buf); + err = build_id_size < 0 ? build_id_size : 0; if (CHECK(err, "get build_id with readelf", "err %d errno %d\n", err, errno)) @@ -106,8 +106,6 @@ void test_stacktrace_build_id_nmi(void) goto cleanup; do { - char build_id[64]; - err = bpf_map__lookup_elem(skel->maps.stackmap, &key, sizeof(key), id_offs, sizeof(id_offs), 0); if (CHECK(err, "lookup_elem from stackmap", @@ -116,10 +114,7 @@ void test_stacktrace_build_id_nmi(void) for (i = 0; i < PERF_MAX_STACK_DEPTH; ++i) if (id_offs[i].status == BPF_STACK_BUILD_ID_VALID && id_offs[i].offset != 0) { - for (j = 0; j < 20; ++j) - sprintf(build_id + 2 * j, "%02x", - id_offs[i].build_id[j] & 0xff); - if (strstr(buf, build_id) != NULL) + if (memcmp(buf, id_offs[i].build_id, build_id_size) == 0) build_id_matches = 1; } prev_key = key; diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c index 6d5e3022c75f..9813d53c4878 100644 --- a/tools/testing/selftests/bpf/test_progs.c +++ b/tools/testing/selftests/bpf/test_progs.c @@ -591,31 +591,6 @@ int compare_stack_ips(int smap_fd, int amap_fd, int stack_trace_len) return err; } -int extract_build_id(char *build_id, size_t size) -{ - FILE *fp; - char *line = NULL; - size_t len = 0; - - fp = popen("readelf -n ./urandom_read | grep 'Build ID'", "r"); - if (fp == NULL) - return -1; - - if (getline(&line, &len, fp) == -1) - goto err; - pclose(fp); - - if (len > size) - len = size; - memcpy(build_id, line, len); - build_id[len] = '\0'; - free(line); - return 0; -err: - pclose(fp); - return -1; -} - static int finit_module(int fd, const char *param_values, int flags) { return syscall(__NR_finit_module, fd, param_values, flags); diff --git a/tools/testing/selftests/bpf/test_progs.h b/tools/testing/selftests/bpf/test_progs.h index 9fbdc57c5b57..3825c2797a4b 100644 --- a/tools/testing/selftests/bpf/test_progs.h +++ b/tools/testing/selftests/bpf/test_progs.h @@ -404,7 +404,6 @@ static inline void *u64_to_ptr(__u64 ptr) int bpf_find_map(const char *test, struct bpf_object *obj, const char *name); int compare_map_keys(int map1_fd, int map2_fd); int compare_stack_ips(int smap_fd, int amap_fd, int stack_trace_len); -int extract_build_id(char *build_id, size_t size); int kern_sync_rcu(void); int trigger_module_test_read(int read_sz); int trigger_module_test_write(int write_sz);
Replacing extract_build_id with read_build_id that parses out build id directly from elf without using readelf tool. Signed-off-by: Jiri Olsa <jolsa@kernel.org> --- .../bpf/prog_tests/stacktrace_build_id.c | 19 ++++++-------- .../bpf/prog_tests/stacktrace_build_id_nmi.c | 17 +++++-------- tools/testing/selftests/bpf/test_progs.c | 25 ------------------- tools/testing/selftests/bpf/test_progs.h | 1 - 4 files changed, 13 insertions(+), 49 deletions(-)