Thanks Tobin. I will fold these changes in.
> On May 3, 2018, at 12:19 AM, Tobin C. Harding <to...@apporbit.com> wrote:
>
> On Wed, May 02, 2018 at 04:20:30PM -0700, Song Liu wrote:
>> This new test captures stackmap with build_id with hardware event
>> PERF_COUNT_HW_CPU_CYCLES.
>>
>> Because we only support one ips-to-build_id lookup per cpu in NMI
>> context, stack_amap will not be able to do the lookup in this test.
>
> stack_map ?
This one is stack_amap. There are two maps in the test.
Song
>
>> Therefore, we didn't do compare_stack_ips(), as it will alwasy fail.
>>
>> urandom_read.c is extended to run configurable cycles so that it can be
>> caught by the perf event.
>>
>> Signed-off-by: Song Liu <songliubrav...@fb.com>
>> ---
>> tools/testing/selftests/bpf/test_progs.c | 137
>> +++++++++++++++++++++++++++++
>> tools/testing/selftests/bpf/urandom_read.c | 10 ++-
>> 2 files changed, 145 insertions(+), 2 deletions(-)
>>
>> diff --git a/tools/testing/selftests/bpf/test_progs.c
>> b/tools/testing/selftests/bpf/test_progs.c
>> index aa336f0..00bb08c 100644
>> --- a/tools/testing/selftests/bpf/test_progs.c
>> +++ b/tools/testing/selftests/bpf/test_progs.c
>> @@ -1272,6 +1272,142 @@ static void test_stacktrace_build_id(void)
>> return;
>> }
>>
>> +static void test_stacktrace_build_id_nmi(void)
>> +{
>> + int control_map_fd, stackid_hmap_fd, stackmap_fd, stack_amap_fd;
>> + const char *file = "./test_stacktrace_build_id.o";
>> + int err, pmu_fd, prog_fd;
>> + struct perf_event_attr attr = {
>> + .sample_freq = 5000,
>> + .freq = 1,
>> + .type = PERF_TYPE_HARDWARE,
>> + .config = PERF_COUNT_HW_CPU_CYCLES,
>> + };
>> + __u32 key, previous_key, val, duration = 0;
>> + struct bpf_object *obj;
>> + char buf[256];
>> + int i, j;
>> + struct bpf_stack_build_id id_offs[PERF_MAX_STACK_DEPTH];
>> + int build_id_matches = 0;
>> +
>> + err = bpf_prog_load(file, BPF_PROG_TYPE_PERF_EVENT, &obj, &prog_fd);
>> + if (CHECK(err, "prog_load", "err %d errno %d\n", err, errno))
>> + goto out;
>
> perhaps:
> return;
>
>> + pmu_fd = syscall(__NR_perf_event_open, &attr, -1 /* pid */,
>> + 0 /* cpu 0 */, -1 /* group id */,
>> + 0 /* flags */);
>> + if (CHECK(pmu_fd < 0, "perf_event_open",
>> + "err %d errno %d. Does the test host support
>> PERF_COUNT_HW_CPU_CYCLES?\n",
>> + pmu_fd, errno))
>> + goto close_prog;
>> +
>> + err = ioctl(pmu_fd, PERF_EVENT_IOC_ENABLE, 0);
>> + if (CHECK(err, "perf_event_ioc_enable", "err %d errno %d\n",
>> + err, errno))
>> + goto close_pmu;
>> +
>> + err = ioctl(pmu_fd, PERF_EVENT_IOC_SET_BPF, prog_fd);
>> + if (CHECK(err, "perf_event_ioc_set_bpf", "err %d errno %d\n",
>> + err, errno))
>> + goto disable_pmu;
>> +
>> + /* find map fds */
>> + control_map_fd = bpf_find_map(__func__, obj, "control_map");
>> + if (CHECK(control_map_fd < 0, "bpf_find_map control_map",
>> + "err %d errno %d\n", err, errno))
>> + goto disable_pmu;
>> +
>> + stackid_hmap_fd = bpf_find_map(__func__, obj, "stackid_hmap");
>> + if (CHECK(stackid_hmap_fd < 0, "bpf_find_map stackid_hmap",
>> + "err %d errno %d\n", err, errno))
>> + goto disable_pmu;
>> +
>> + stackmap_fd = bpf_find_map(__func__, obj, "stackmap");
>> + if (CHECK(stackmap_fd < 0, "bpf_find_map stackmap", "err %d errno %d\n",
>> + err, errno))
>> + goto disable_pmu;
>> +
>> + stack_amap_fd = bpf_find_map(__func__, obj, "stack_amap");
>> + if (CHECK(stack_amap_fd < 0, "bpf_find_map stack_amap",
>> + "err %d errno %d\n", err, errno))
>> + goto disable_pmu;
>> +
>> + assert(system("dd if=/dev/urandom of=/dev/zero count=4 2> /dev/null")
>> + == 0);
>> + assert(system("taskset 0x1 ./urandom_read 100000") == 0);
>> + /* disable stack trace collection */
>> + key = 0;
>> + val = 1;
>> + bpf_map_update_elem(control_map_fd, &key, &val, 0);
>> +
>> + /* for every element in stackid_hmap, we can find a corresponding one
>> + * in stackmap, and vise versa.
>> + */
>> + err = compare_map_keys(stackid_hmap_fd, stackmap_fd);
>> + if (CHECK(err, "compare_map_keys stackid_hmap vs. stackmap",
>> + "err %d errno %d\n", err, errno))
>> + goto disable_pmu;
>> +
>> + err = compare_map_keys(stackmap_fd, stackid_hmap_fd);
>> + if (CHECK(err, "compare_map_keys stackmap vs. stackid_hmap",
>> + "err %d errno %d\n", err, errno))
>> + goto disable_pmu;
>> +
>> + err = extract_build_id(buf, 256);
>> +
>> + if (CHECK(err, "get build_id with readelf",
>> + "err %d errno %d\n", err, errno))
>> + goto disable_pmu;
>> +
>> + err = bpf_map_get_next_key(stackmap_fd, NULL, &key);
>> + if (CHECK(err, "get_next_key from stackmap",
>> + "err %d, errno %d\n", err, errno))
>> + goto disable_pmu;
>> +
>> + 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))
>> + goto disable_pmu;
>> + 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)
>> + build_id_matches = 1;
>> + }
>> + previous_key = key;
>> + } while (bpf_map_get_next_key(stackmap_fd, &previous_key, &key) == 0);
>> +
>> + if (CHECK(build_id_matches < 1, "build id match",
>> + "Didn't find expected build ID from the map\n"))
>> + goto disable_pmu;
>> +
>> + /*
>> + * We intentionally skip compare_stack_ips(). This is because we
>> + * only support one in_nmi() ips-to-build_id translation per cpu
>> + * at any time, thus stack_amap here will always fallback to
>> + * BPF_STACK_BUILD_ID_IP;
>> + */
>> +
>> +disable_pmu:
>> + ioctl(pmu_fd, PERF_EVENT_IOC_DISABLE);
>> +
>> +close_pmu:
>> + close(pmu_fd);
>> +
>> +close_prog:
>> + bpf_object__close(obj);
>> +
>> +out:
>> + return;
>> +}
>
> No real need for label 'out' right? We can just return directly and
> remove the last three lines of this function.
>
> Hope this helps,
> Tobin.
