Re: [PATCH v3 bpf-next 3/3] selftests/bpf: add bpf_perf_prog_read_branches() selftest

["Daniel Xu" <dxu@dxuuu.xyz>]

On Thu Jan 23, 2020 at 11:20 PM, Martin Lau wrote:
> On Thu, Jan 23, 2020 at 01:23:12PM -0800, Daniel Xu wrote:
> > Signed-off-by: Daniel Xu <dxu@dxuuu.xyz>
> Please put some details to avoid empty commit message.
> Same for patch 2.

Ok.
>
> 
> > ---
> >  .../selftests/bpf/prog_tests/perf_branches.c  | 106 ++++++++++++++++++
> >  .../selftests/bpf/progs/test_perf_branches.c  |  39 +++++++
> >  2 files changed, 145 insertions(+)
> >  create mode 100644 tools/testing/selftests/bpf/prog_tests/perf_branches.c
> >  create mode 100644 tools/testing/selftests/bpf/progs/test_perf_branches.c
> > 
> > diff --git a/tools/testing/selftests/bpf/prog_tests/perf_branches.c b/tools/testing/selftests/bpf/prog_tests/perf_branches.c
> > new file mode 100644
> > index 000000000000..f8d7356a6507
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/prog_tests/perf_branches.c
> > @@ -0,0 +1,106 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +#define _GNU_SOURCE
> > +#include <pthread.h>
> > +#include <sched.h>
> > +#include <sys/socket.h>
> > +#include <test_progs.h>
> > +#include "bpf/libbpf_internal.h"
> > +
> > +static void on_sample(void *ctx, int cpu, void *data, __u32 size)
> > +{
> > +	int pbe_size = sizeof(struct perf_branch_entry);
> > +	int ret = *(int *)data, duration = 0;
> > +
> > +	// It's hard to validate the contents of the branch entries b/c it
> > +	// would require some kind of disassembler and also encoding the
> > +	// valid jump instructions for supported architectures. So just check
> > +	// the easy stuff for now.
> /* ... */ comment style

Whoops, sorry.

>
> 
> > +	CHECK(ret < 0, "read_branches", "err %d\n", ret);
> > +	CHECK(ret % pbe_size != 0, "read_branches",
> > +	      "bytes written=%d not multiple of struct size=%d\n",
> > +	      ret, pbe_size);
> > +
> > +	*(int *)ctx = 1;
> > +}
> > +
> > +void test_perf_branches(void)
> > +{
> > +	int err, prog_fd, i, pfd = -1, duration = 0, ok = 0;
> > +	const char *file = "./test_perf_branches.o";
> > +	const char *prog_name = "perf_event";
> > +	struct perf_buffer_opts pb_opts = {};
> > +	struct perf_event_attr attr = {};
> > +	struct bpf_map *perf_buf_map;
> > +	struct bpf_program *prog;
> > +	struct bpf_object *obj;
> > +	struct perf_buffer *pb;
> > +	struct bpf_link *link;
> > +	volatile int j = 0;
> > +	cpu_set_t cpu_set;
> > +
> > +	/* load program */
> > +	err = bpf_prog_load(file, BPF_PROG_TYPE_PERF_EVENT, &obj, &prog_fd);
> > +	if (CHECK(err, "obj_load", "err %d errno %d\n", err, errno)) {
> > +		obj = NULL;
> > +		goto out_close;
> > +	}
> > +
> > +	prog = bpf_object__find_program_by_title(obj, prog_name);
> > +	if (CHECK(!prog, "find_probe", "prog '%s' not found\n", prog_name))
> > +		goto out_close;
> > +
> > +	/* load map */
> > +	perf_buf_map = bpf_object__find_map_by_name(obj, "perf_buf_map");
> > +	if (CHECK(!perf_buf_map, "find_perf_buf_map", "not found\n"))
> > +		goto out_close;
> Using skel may be able to cut some lines.

Ok, will take a look.

>
> 
> > +
> > +	/* create perf event */
> > +	attr.size = sizeof(attr);
> > +	attr.type = PERF_TYPE_HARDWARE;
> > +	attr.config = PERF_COUNT_HW_CPU_CYCLES;
> > +	attr.freq = 1;
> > +	attr.sample_freq = 4000;
> > +	attr.sample_type = PERF_SAMPLE_BRANCH_STACK;
> > +	attr.branch_sample_type = PERF_SAMPLE_BRANCH_USER | PERF_SAMPLE_BRANCH_ANY;
> > +	pfd = syscall(__NR_perf_event_open, &attr, -1, 0, -1, PERF_FLAG_FD_CLOEXEC);
> > +	if (CHECK(pfd < 0, "perf_event_open", "err %d\n", pfd))
> > +		goto out_close;
> > +
> > +	/* attach perf_event */
> > +	link = bpf_program__attach_perf_event(prog, pfd);
> > +	if (CHECK(IS_ERR(link), "attach_perf_event", "err %ld\n", PTR_ERR(link)))
> > +		goto out_close_perf;
> > +
> > +	/* set up perf buffer */
> > +	pb_opts.sample_cb = on_sample;
> > +	pb_opts.ctx = &ok;
> > +	pb = perf_buffer__new(bpf_map__fd(perf_buf_map), 1, &pb_opts);
> > +	if (CHECK(IS_ERR(pb), "perf_buf__new", "err %ld\n", PTR_ERR(pb)))
> > +		goto out_detach;
> > +
> > +	/* generate some branches on cpu 0 */
> > +	CPU_ZERO(&cpu_set);
> > +	CPU_SET(0, &cpu_set);
> > +	err = pthread_setaffinity_np(pthread_self(), sizeof(cpu_set), &cpu_set);
> > +	if (err && CHECK(err, "set_affinity", "cpu #0, err %d\n", err))
> 'err &&' seems unnecessary.

Will remove.

>
> 
> > +		goto out_free_pb;
> > +	for (i = 0; i < 1000000; ++i)
> May be some comments on 1000000?
>
> 
> > +		++j;
> > +
> > +	/* read perf buffer */
> > +	err = perf_buffer__poll(pb, 500);
> > +	if (CHECK(err < 0, "perf_buffer__poll", "err %d\n", err))
> > +		goto out_free_pb;
> > +
> > +	if (CHECK(!ok, "ok", "not ok\n"))
> > +		goto out_free_pb;
> > +
> > +out_free_pb:
> > +	perf_buffer__free(pb);
> > +out_detach:
> > +	bpf_link__destroy(link);
> > +out_close_perf:
> > +	close(pfd);
> > +out_close:
> > +	bpf_object__close(obj);
> > +}
> > diff --git a/tools/testing/selftests/bpf/progs/test_perf_branches.c b/tools/testing/selftests/bpf/progs/test_perf_branches.c
> > new file mode 100644
> > index 000000000000..d818079c7778
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/progs/test_perf_branches.c
> > @@ -0,0 +1,39 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +// Copyright (c) 2019 Facebook
> > +
> > +#include <linux/ptrace.h>
> > +#include <linux/bpf.h>
> > +#include <bpf/bpf_helpers.h>
> > +#include "bpf_trace_helpers.h"
> > +
> > +struct {
> > +	__uint(type, BPF_MAP_TYPE_PERF_EVENT_ARRAY);
> > +	__uint(key_size, sizeof(int));
> > +	__uint(value_size, sizeof(int));
> > +} perf_buf_map SEC(".maps");
> > +
> > +struct fake_perf_branch_entry {
> > +	__u64 _a;
> > +	__u64 _b;
> > +	__u64 _c;
> > +};
> > +
> > +SEC("perf_event")
> > +int perf_branches(void *ctx)
> > +{
> > +	int ret;
> > +	struct fake_perf_branch_entry entries[4];
> Try to keep the reverse xmas tree.
>
> 
> > +
> > +	ret = bpf_perf_prog_read_branches(ctx,
> > +					  entries,
> > +					  sizeof(entries));
> > +	/* ignore spurious events */
> > +	if (!ret)
> Check for -ve also?

Assuming that means negative, no. Sometimes there aren't any branch
events stored. That's ok and we want to ignore that. If there's an error
(negative), we should pass that up to the selftest in userspace and fail
the test.

>
> 
> > +		return 1;
> > +
> > +	bpf_perf_event_output(ctx, &perf_buf_map, BPF_F_CURRENT_CPU,
> > +			      &ret, sizeof(ret));
> > +	return 0;
> > +}
> > +
> > +char _license[] SEC("license") = "GPL";
> > -- 
> > 2.21.1
> > 
>
> 
>
>