Date:	Thu, 29 Oct 2015 19:37:44 -0300
From:	Arnaldo Carvalho de Melo <acme@...nel.org>
To:	Wang Nan <wangnan0@...wei.com>
Cc:	ast@...mgrid.com, brendan.d.gregg@...il.com,
	a.p.zijlstra@...llo.nl, daniel@...earbox.net, dsahern@...il.com,
	hekuang@...wei.com, jolsa@...nel.org, lizefan@...wei.com,
	masami.hiramatsu.pt@...achi.com, namhyung@...nel.org,
	paulus@...ba.org, linux-kernel@...r.kernel.org, pi3orama@....com,
	xiakaixu@...wei.com
Subject: Re: [PATCH 10/31] perf test: Enforce LLVM test for BPF test

Em Wed, Oct 14, 2015 at 12:41:21PM +0000, Wang Nan escreveu:
> This patch replaces the original toy BPF program with previous introduced
> bpf-script-example.c. Dynamically embedded it into 'llvm-src.c'.
> 
> The newly introduced BPF program attaches a BPF program at
> 'sys_epoll_pwait()', and collect half samples from it. perf itself never

"collect half samples from it"? Can you rephrase this?

> use that syscall, so further test can verify their result with it.
> 
> Since BPF program require LINUX_VERSION_CODE of runtime kernel, this
> patch computes that code from uname.
> 
> Since the resuling BPF object is useful for further testcases, this patch
> introduces 'prepare' and 'cleanup' method to tests, and makes test__llvm()
> create a MAP_SHARED memory array to hold the resulting object.

I think you're doing lots of things in just one patch, please split it,
for instance, that ->prepare()/->cleanup() should stand on its own, with
a proper changelog, etc.

The LINUX_VERSION_CODE part should probably also be on its own patch,
working much like __FILE__ or __func__, i.e. gcc predefines those, the
bpf code that calls clang to build the provided .c file should do the
same for LINUX_VERSION_CODE, so that bpf scriptlets can rely on it being
defined.

I.e. that compose_source() function, but not just for the 'perf test'
entry, for any bpf scriptlet.
 
> Signed-off-by: He Kuang <hekuang@...wei.com>
> Signed-off-by: Wang Nan <wangnan0@...wei.com>
> Cc: Arnaldo Carvalho de Melo <acme@...hat.com>
> Cc: Alexei Starovoitov <ast@...mgrid.com>
> Cc: Brendan Gregg <brendan.d.gregg@...il.com>
> Cc: Daniel Borkmann <daniel@...earbox.net>
> Cc: David Ahern <dsahern@...il.com>
> Cc: Jiri Olsa <jolsa@...nel.org>
> Cc: Kaixu Xia <xiakaixu@...wei.com>
> Cc: Masami Hiramatsu <masami.hiramatsu.pt@...achi.com>
> Cc: Namhyung Kim <namhyung@...nel.org>
> Cc: Peter Zijlstra <a.p.zijlstra@...llo.nl>
> Cc: Zefan Li <lizefan@...wei.com>
> Cc: pi3orama@....com
> Link: http://lkml.kernel.org/n/ebpf-6yw9eg0ej3l4jnqhinngkw86@git.kernel.org
> ---
>  tools/perf/tests/Build          |   9 +++-
>  tools/perf/tests/builtin-test.c |   6 +++
>  tools/perf/tests/llvm.c         | 104 +++++++++++++++++++++++++++++++++++-----
>  tools/perf/tests/llvm.h         |  14 ++++++
>  tools/perf/tests/tests.h        |   4 ++
>  5 files changed, 123 insertions(+), 14 deletions(-)
>  create mode 100644 tools/perf/tests/llvm.h
> 
> diff --git a/tools/perf/tests/Build b/tools/perf/tests/Build
> index 50de225..4afc8c8 100644
> --- a/tools/perf/tests/Build
> +++ b/tools/perf/tests/Build
> @@ -31,9 +31,16 @@ perf-y += sample-parsing.o
>  perf-y += parse-no-sample-id-all.o
>  perf-y += kmod-path.o
>  perf-y += thread-map.o
> -perf-y += llvm.o
> +perf-y += llvm.o llvm-src.o
>  perf-y += topology.o
>  
> +$(OUTPUT)tests/llvm-src.c: tests/bpf-script-example.c
> +	$(call rule_mkdir)
> +	$(Q)echo '#include <tests/llvm.h>' > $@
> +	$(Q)echo 'const char test_llvm__bpf_prog[] =' >> $@
> +	$(Q)sed -e 's/"/\\"/g' -e 's/\(.*\)/"\1\\n"/g' $< >> $@
> +	$(Q)echo ';' >> $@
> +
>  ifeq ($(ARCH),$(filter $(ARCH),x86 arm arm64))
>  perf-$(CONFIG_DWARF_UNWIND) += dwarf-unwind.o
>  endif
> diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
> index 66f72d3..e812a0c 100644
> --- a/tools/perf/tests/builtin-test.c
> +++ b/tools/perf/tests/builtin-test.c
> @@ -160,6 +160,8 @@ static struct test generic_tests[] = {
>  	{
>  		.desc = "Test LLVM searching and compiling",
>  		.func = test__llvm,
> +		.prepare = test__llvm_prepare,
> +		.cleanup = test__llvm_cleanup,
>  	},
>  	{
>  		.desc = "Test topology in session",
> @@ -261,7 +263,11 @@ static int __cmd_test(int argc, const char *argv[], struct intlist *skiplist)
>  		}
>  
>  		pr_debug("\n--- start ---\n");
> +		if (t->prepare)
> +			t->prepare();
>  		err = run_test(t);
> +		if (t->cleanup)
> +			t->cleanup();
>  		pr_debug("---- end ----\n%s:", t->desc);
>  
>  		switch (err) {
> diff --git a/tools/perf/tests/llvm.c b/tools/perf/tests/llvm.c
> index 52d5597..236bf39 100644
> --- a/tools/perf/tests/llvm.c
> +++ b/tools/perf/tests/llvm.c
> @@ -1,9 +1,13 @@
>  #include <stdio.h>
> +#include <sys/utsname.h>
>  #include <bpf/libbpf.h>
>  #include <util/llvm-utils.h>
>  #include <util/cache.h>
> +#include <util/util.h>
> +#include <sys/mman.h>
>  #include "tests.h"
>  #include "debug.h"
> +#include "llvm.h"
>  
>  static int perf_config_cb(const char *var, const char *val,
>  			  void *arg __maybe_unused)
> @@ -11,16 +15,6 @@ static int perf_config_cb(const char *var, const char *val,
>  	return perf_default_config(var, val, arg);
>  }
>  
> -/*
> - * Randomly give it a "version" section since we don't really load it
> - * into kernel
> - */
> -static const char test_bpf_prog[] =
> -	"__attribute__((section(\"do_fork\"), used)) "
> -	"int fork(void *ctx) {return 0;} "
> -	"char _license[] __attribute__((section(\"license\"), used)) = \"GPL\";"
> -	"int _version __attribute__((section(\"version\"), used)) = 0x40100;";
> -
>  #ifdef HAVE_LIBBPF_SUPPORT
>  static int test__bpf_parsing(void *obj_buf, size_t obj_buf_sz)
>  {
> @@ -41,12 +35,44 @@ static int test__bpf_parsing(void *obj_buf __maybe_unused,
>  }
>  #endif
>  
> +static char *
> +compose_source(void)
> +{
> +	struct utsname utsname;
> +	int version, patchlevel, sublevel, err;
> +	unsigned long version_code;
> +	char *code;
> +
> +	if (uname(&utsname))
> +		return NULL;
> +
> +	err = sscanf(utsname.release, "%d.%d.%d",
> +		     &version, &patchlevel, &sublevel);
> +	if (err != 3) {
> +		fprintf(stderr, " (Can't get kernel version from uname '%s')",
> +			utsname.release);
> +		return NULL;
> +	}
> +
> +	version_code = (version << 16) + (patchlevel << 8) + sublevel;
> +	err = asprintf(&code, "#define LINUX_VERSION_CODE 0x%08lx;\n%s",
> +		       version_code, test_llvm__bpf_prog);
> +	if (err < 0)
> +		return NULL;
> +
> +	return code;
> +}
> +
> +#define SHARED_BUF_INIT_SIZE	(1 << 20)
> +struct test_llvm__bpf_result *p_test_llvm__bpf_result;
> +
>  int test__llvm(void)
>  {
>  	char *tmpl_new, *clang_opt_new;
>  	void *obj_buf;
>  	size_t obj_buf_sz;
>  	int err, old_verbose;
> +	char *source;
>  
>  	perf_config(perf_config_cb, NULL);
>  
> @@ -73,10 +99,22 @@ int test__llvm(void)
>  	if (!llvm_param.clang_opt)
>  		llvm_param.clang_opt = strdup("");
>  
> -	err = asprintf(&tmpl_new, "echo '%s' | %s", test_bpf_prog,
> -		       llvm_param.clang_bpf_cmd_template);
> -	if (err < 0)
> +	source = compose_source();
> +	if (!source) {
> +		pr_err("Failed to compose source code\n");
> +		return -1;
> +	}
> +
> +	/* Quote __EOF__ so strings in source won't be expanded by shell */
> +	err = asprintf(&tmpl_new, "cat << '__EOF__' | %s\n%s\n__EOF__\n",
> +		       llvm_param.clang_bpf_cmd_template, source);
> +	free(source);
> +	source = NULL;
> +	if (err < 0) {
> +		pr_err("Failed to alloc new template\n");
>  		return -1;
> +	}
> +
>  	err = asprintf(&clang_opt_new, "-xc %s", llvm_param.clang_opt);
>  	if (err < 0)
>  		return -1;
> @@ -93,6 +131,46 @@ int test__llvm(void)
>  	}
>  
>  	err = test__bpf_parsing(obj_buf, obj_buf_sz);
> +	if (!err && p_test_llvm__bpf_result) {
> +		if (obj_buf_sz > SHARED_BUF_INIT_SIZE) {
> +			pr_err("Resulting object too large\n");
> +		} else {
> +			p_test_llvm__bpf_result->size = obj_buf_sz;
> +			memcpy(p_test_llvm__bpf_result->object,
> +			       obj_buf, obj_buf_sz);
> +		}
> +	}
>  	free(obj_buf);
>  	return err;
>  }
> +
> +void test__llvm_prepare(void)
> +{
> +	p_test_llvm__bpf_result = mmap(NULL, SHARED_BUF_INIT_SIZE,
> +				       PROT_READ | PROT_WRITE,
> +				       MAP_SHARED | MAP_ANONYMOUS, -1, 0);
> +	if (!p_test_llvm__bpf_result)
> +		return;
> +	memset((void *)p_test_llvm__bpf_result, '\0', SHARED_BUF_INIT_SIZE);
> +}
> +
> +void test__llvm_cleanup(void)
> +{
> +	unsigned long boundary, buf_end;
> +
> +	if (!p_test_llvm__bpf_result)
> +		return;
> +	if (p_test_llvm__bpf_result->size == 0) {
> +		munmap((void *)p_test_llvm__bpf_result, SHARED_BUF_INIT_SIZE);
> +		p_test_llvm__bpf_result = NULL;
> +		return;
> +	}
> +
> +	buf_end = (unsigned long)p_test_llvm__bpf_result + SHARED_BUF_INIT_SIZE;
> +
> +	boundary = (unsigned long)(p_test_llvm__bpf_result);
> +	boundary += p_test_llvm__bpf_result->size;
> +	boundary = (boundary + (page_size - 1)) &
> +			(~((unsigned long)page_size - 1));
> +	munmap((void *)boundary, buf_end - boundary);
> +}
> diff --git a/tools/perf/tests/llvm.h b/tools/perf/tests/llvm.h
> new file mode 100644
> index 0000000..1e89e46
> --- /dev/null
> +++ b/tools/perf/tests/llvm.h
> @@ -0,0 +1,14 @@
> +#ifndef PERF_TEST_LLVM_H
> +#define PERF_TEST_LLVM_H
> +
> +#include <stddef.h> /* for size_t */
> +
> +struct test_llvm__bpf_result {
> +	size_t size;
> +	char object[];
> +};
> +
> +extern struct test_llvm__bpf_result *p_test_llvm__bpf_result;
> +extern const char test_llvm__bpf_prog[];
> +
> +#endif
> diff --git a/tools/perf/tests/tests.h b/tools/perf/tests/tests.h
> index c804869..a848802 100644
> --- a/tools/perf/tests/tests.h
> +++ b/tools/perf/tests/tests.h
> @@ -27,6 +27,8 @@ enum {
>  struct test {
>  	const char *desc;
>  	int (*func)(void);
> +	void (*prepare)(void);
> +	void (*cleanup)(void);
>  };
>  
>  /* Tests */
> @@ -66,6 +68,8 @@ int test__fdarray__add(void);
>  int test__kmod_path__parse(void);
>  int test__thread_map(void);
>  int test__llvm(void);
> +void test__llvm_prepare(void);
> +void test__llvm_cleanup(void);
>  int test_session_topology(void);
>  
>  #if defined(__arm__) || defined(__aarch64__)
> -- 
> 1.8.3.4
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists