annotate program tag #1332

4ast · 2017-09-02T00:43:22Z

during debug of production systems it's difficult to trace back
the kernel reported 'bpf_prog_3196C6D60BBB8549' symbols to the source code
of the program, hence teach bcc to store the main function source
in the /var/tmp/bcc/bpf_prog_3196C6D60BBB8549/ directory.

This program tag is stable. Every time the script is called the tag
will be the same unless source code of the program changes.
During active development of bcc scripts the /var/tmp/bcc/ dir can
get a bunch of stale tags.
The users have to trim that dir manually.

Python scripts can be modified to use this feature too, but probably
need to be gated by the flag. For c++ api I think it makes sense
to store the source code always, since the cost is minimal and
c++ api is used by long running services.

Example:
$ ./examples/cpp/LLCStat
$ ls -l /var/tmp/bcc/bpf_prog_3196C6D60BBB8549/
total 16
-rw-r--r--. 1 root root 226 Sep 1 17:30 on_cache_miss.c
-rw-r--r--. 1 root root 487 Sep 1 17:30 on_cache_miss.rewritten.c
-rw-r--r--. 1 root root 224 Sep 1 17:30 on_cache_ref.c
-rw-r--r--. 1 root root 484 Sep 1 17:30 on_cache_ref.rewritten.c
$ cat /var/tmp/bcc/bpf_prog_3196C6D60BBB8549/on_cache_miss.c
int on_cache_miss(struct bpf_perf_event_data *ctx) {
struct event_t key = {};
get_key(&key);

u64 zero = 0, *val;
val = miss_count.lookup_or_init(&key, &zero);

...

Signed-off-by: Alexei Starovoitov [email protected]

4ast · 2017-09-02T00:43:49Z

cc @yonghong-song @palmtenor @drzaeus77 thoughts?

drzaeus77 · 2017-09-02T00:49:27Z

Should the files survive a reboot? /var/tmp vs /var/run? Probably should be cmake configurable.

4ast · 2017-09-02T00:54:01Z

should definitely survive the reboot. both /var/run and /var/tmp on my system have this property. don't have strong opinion which one is better. /var/tmp has more clear meaning that it's temporary.
cmake flag is good idea. will do.

drzaeus77

Seems like these can be two separate commits, with the tag computation coming first and the rewritten source file stuff second.

drzaeus77 · 2017-09-02T00:50:53Z

src/cc/bpf_module.cc

+
+ ::snprintf(buf, sizeof(buf), "/var/tmp/bcc/bpf_prog_%llX/%s.c",
+ tag1, name.data());
+ int fd = open(buf, O_CREAT | O_WRONLY | O_TRUNC, 0644);


Could use FileDesc class here.

drzaeus77 · 2017-09-02T00:52:30Z

src/cc/frontends/b/loader.cc

@@ -33,7 +33,7 @@ BLoader::~BLoader() {
 }

 int BLoader::parse(llvm::Module *mod, const string &filename, const string &proto_filename,
- TableStorage &ts, const string &id) {
+ TableStorage &ts, const std::string &id) {


Why? It is inconsistent with surrounding lines, and unnecessary in this particular .cc file.

drzaeus77 · 2017-09-02T00:56:05Z

src/cc/frontends/clang/loader.h

+ std::map<std::string, SourceCode> funcs_;
+ public:
+ FuncSource() {}
+ const char * get_src(const std::string& name);


Would prefer to return a const string & so that caller has option to use other string methods if they want, or data() otherwise. This is throwing away type information for no reason.

Also, the convention is to name the functions as src() and src_rewritten(), omitting the get_ prefix.

returning static string s = ""; return s; looked uglier to me than return "";
will drop get_

drzaeus77 · 2017-09-02T01:04:10Z

src/cc/libbpf.h

@@ -89,6 +89,10 @@ int bpf_close_perf_event_fd(int fd);

 int bpf_obj_pin(int fd, const char *pathname);
 int bpf_obj_get(const char *pathname);
+int bpf_obj_get_info_by_fd(int prog_fd, void *info, int *info_len);


Does _by_fd need to be here? It is inconsistent with other bpf_xx functions, the fd is generally an implicit parameter.

drzaeus77 · 2017-09-02T01:07:56Z

src/cc/libbpf.c

+ .salg_type = "hash",
+ .salg_name = "sha1",
+ };
+ int shafd = socket(AF_ALG, SOCK_SEQPACKET, 0);


Huh, never knew about this one. Alternatively, there are some header-only implementations of sha1, but I assume you chose this approach for a reason?

which header only? was only aware of this one, since it's so simple.

palmtenor · 2017-09-02T03:39:02Z

The Rewritten text would be super useful when debugging!

yonghong-song

A high-level question. The current scheme is to write each function (before/after rewriter) into two files. Does it make sense to base on compilation unit (each source file is a compilation unit)? This way, macro's, functions to be inlined later, table definition, will be all in the same file, easy to see relationships.

This may simplify your implementation as well?

Later on, I do plan to implement a debug option to permit user to print out source code annotated byte code (implementation will be similar to llvm-objdump -D). So with verifier error, they can go back to see which source line is corresponding to and hopefully can find the error easily. This is source file based as well.

4ast · 2017-09-06T01:05:05Z

@yonghong-song
most bpf programs are written with meaningful function body and not just single call into always_inline.
Also single compilation typically has many bpf programs. If bcc keeps the whole file for all prog_tags, how the user will know which one is running?
also file won't have all the source either. kernel headers, bcc_helpers are external to the file.
Ideally we could preserve .c code after inlining, but that's not possible.
So original func src and rewritten one seems to be as a good start for typical bcc usage.
The upcoming CTF work and further extensions to BPF_OBJ_GET_INFO_BY_FD
will give us all the info about maps, so keeping full file just to see the maps is imo
unnecessary. The rewritten .c code has bpf_pseudo(FD_NUM) calls, so it's
easy for humans to see which map fd to look at for map details.
We can make bcc store map info in /var/bcc too, but imo it's not necessary
since obj_get_info can return that as well we just need to build the tools.
Storing annotated bytecode (as llvm-objdump) associated with prog_tag
also would be great!

yonghong-song · 2017-09-06T04:42:13Z

@4ast Right, I forgot to pay attention to prog tag is for bpf prog. Esp if you want to load one prog multiple times, prog tag should differentiate it. My compilation unit idea does not fit here.

Currently, the files are dumped after loading the functions into the kernel.
The files will be preserved when functions are unloaded or even reboot.
This is a good first step towards exposing kernel bpf states in user spaces.
Expecting more in the future to reflect the actual runtime bpf state on the host!

yonghong-song · 2017-09-06T17:06:01Z

src/cc/libbpf.c

+ }
+ struct bpf_insn prog[prog_len / 8];
+ bool map_ld_seen = false;
+ for (int i = 0; i < prog_len / 8; i++) {


looks like old version of gcc (4.8.5) does not like "for (int i = 0; ...". You need additional flags to make gcc happy. So maybe it is a good idea to have " int i; for (i = 0; ...)".

yonghong-song

Overall approach is good. A few additional comments beyond the inlined ones:
(1). the commit message probably outdated.
$ ls -l /var/tmp/bcc/bpf_prog_3196C6D60BBB8549/
total 16
-rw-r--r--. 1 root root 226 Sep 1 17:30 on_cache_miss.c
-rw-r--r--. 1 root root 487 Sep 1 17:30 on_cache_miss.rewritten.c
-rw-r--r--. 1 root root 224 Sep 1 17:30 on_cache_ref.c
-rw-r--r--. 1 root root 484 Sep 1 17:30 on_cache_ref.rewritten.c
Actually, for each function, there is a program tag. So the above four files should be under two different directories.
(2). How user knows which bpf_prog_#tag directory to look? This one probably needs another tool to enumerate all the prog info's on the host. One can manually enumerate as well through /proc//fdinfo/.
(3). Currently, /var/tmp/bcc will just contain a bunch of bpf_prog_ directories. It would be hard to find the one corresponding to an attachment point.
So either the prog enumeration tool should print out attachment point or we could add attachment point to the directory name. (I know you can go one more dir level to find the function names).

yonghong-song · 2017-09-06T19:56:02Z

src/cc/libbpf.c

@@ -219,6 +220,115 @@ static void bpf_print_hints(char *log)
 }
 #define ROUND_UP(x, n) (((x) + (n) - 1u) & ~((n) - 1u))

+int bpf_obj_get_info_by_fd(int prog_fd, void *info, int *info_len)


This function is not used in this patch. I guess this API previously want to get prog_tag and now it directly accesses /proc//fdinfo/ to get this. Since we do not have a well defined structure for "info" yet,
maybe we can leave this API for now.

there are bpf_prog_info and bpf_map_info for info. The api takes void* since it will return whatever FD type is. I can split it into separate patch.

yonghong-song · 2017-09-06T19:59:41Z

src/cc/libbpf.c

+ }
+ unsigned long long tag = 0;
+ sscanf(p + 1, "%llx", &tag);
+ *ptag = __builtin_bswap64(tag);


We will have a little problem for this.
We do a swap for the tag and later on use swapped tag as part of the file name.
This will cause a mismatch between /proc/self/fdinfo//prog_tag and file name,
e.g.,
prog_tag before swap is 0x2, then
directory name will be bpf_prog_0200000000000000.
Which is a little bit awkward.

i don't follow. scanf + swap + printf %llx will do the same bpf_prog_xxx, no?

Okay, just gave a concrete example. Tried one program with two kprobe attachments.
[root@localhost bpf]# cat /proc/20991/fdinfo/8
pos: 0
flags: 02000002
mnt_id: 12
prog_type: 2
prog_jited: 0
prog_tag: 1a8ee7e0aed58b39
memlock: 4096
[root@localhost bpf]# cat /proc/20991/fdinfo/4
pos: 0
flags: 02000002
mnt_id: 12
prog_type: 2
prog_jited: 0
prog_tag: c6fbcd6ef2ad039f
memlock: 4096

[root@localhost bpf]# ls -l /var/tmp/bcc
total 0
drwxr-xr-x. 2 root root 64 Aug 31 12:22 bpf_prog_398BD5AEE0E78E1A
drwxr-xr-x. 2 root root 60 Aug 31 12:06 bpf_prog_9F03ADF26ECDFBC6
[root@localhost bpf]#

oops. I guess I added that swap in the wrong place then. Will move it to bpf_prog_compute_tag().
Do you think there is an issue with scanf() or concern was for bswap only?

sscanf should be okay. The concern is only for bswap.

4ast · 2017-09-06T20:41:22Z

(1). the commit message probably outdated.
$ ls -l /var/tmp/bcc/bpf_prog_3196C6D60BBB8549/
total 16
-rw-r--r--. 1 root root 226 Sep 1 17:30 on_cache_miss.c
-rw-r--r--. 1 root root 487 Sep 1 17:30 on_cache_miss.rewritten.c
-rw-r--r--. 1 root root 224 Sep 1 17:30 on_cache_ref.c
-rw-r--r--. 1 root root 484 Sep 1 17:30 on_cache_ref.rewritten.c
Actually, for each function, there is a program tag. So the above four files should be under two different directories.

the commit is accurate. I picked that example to demonstrate that
two .c files with different names hash to the same prog_tag because
they have exactly the same bpf bytecode

(2). How user knows which bpf_prog_#tag directory to look? This one probably needs another tool to enumerate all the prog info's on the host. One can manually enumerate as well through /proc//fdinfo/.

yes. we need another tool that does prog_get_next_id() + get_info_by_fd().
It needs to be standalone tool. Ideally we'll use libbpf part of bcc
without llvm. It should probably be in pure C code, so we can eventually
put it into kernel tree or some place else.
cc @iamkafai

(3). Currently, /var/tmp/bcc will just contain a bunch of bpf_prog_ directories. It would be hard to find the one corresponding to an attachment point.
So either the prog enumeration tool should print out attachment point or we could add attachment point to the directory name. (I know you can go one more dir level to find the function names).

attachment info will go stale, so if we store such info in dir, we probably
need to clean it up after execution, but not .c files.
I like tag->.c annotation, since it's stable. Even when debugging is happening
a week later. Like if we have 'perf report' from any server we can login
to one of them and look for /var/../bpf_prog_xxx/
That .c code will be exactly what was running back then.
So I'd prefer to keep /var/../bpf_prog_xxx/ for program code related info only.
Like original .c, rewritten, annotated bytecode, llvm ir.

I think separate tool to go from attachment point to prog_tag is better,
since it's for debugging on the specific box while things are live.

bpf_obj_get_info() to retreive prog_tag from the kernel based on prog_fd (kernel 4.13+) bpf_prog_compute_tag() to compute prog_tag from a set of bpf_insns (kernel independent) bpf_prog_get_tag() to retrieve prog_tag from /proc/pid/fdinfo/fd (kernel 4.10+) Signed-off-by: Alexei Starovoitov <[email protected]>

during debug of production systems it's difficult to trace back the kernel reported 'bpf_prog_4985bb0bd6c69631' symbols to the source code of the program, hence teach bcc to store the main function source in the /var/tmp/bcc/bpf_prog_4985bb0bd6c69631/ directory. This program tag is stable. Every time the script is called the tag will be the same unless source code of the program changes. During active development of bcc scripts the /var/tmp/bcc/ dir can get a bunch of stale tags. The users have to trim that dir manually. Python scripts can be modified to use this feature too, but probably need to be gated by the flag. For c++ api I think it makes sense to store the source code always, since the cost is minimal and c++ api is used by long running services. Example: $ ./examples/cpp/LLCStat $ ls -l /var/tmp/bcc/bpf_prog_4985bb0bd6c69631/ total 16 -rw-r--r--. 1 root root 226 Sep 1 17:30 on_cache_miss.c -rw-r--r--. 1 root root 487 Sep 1 17:30 on_cache_miss.rewritten.c -rw-r--r--. 1 root root 224 Sep 1 17:30 on_cache_ref.c -rw-r--r--. 1 root root 484 Sep 1 17:30 on_cache_ref.rewritten.c Note that there are two .c files there, since two different bpf programs have exactly the same bytecode hence same prog_tag. $ cat /var/tmp/bcc/bpf_prog_4985bb0bd6c69631/on_cache_miss.c int on_cache_miss(struct bpf_perf_event_data *ctx) { struct event_t key = {}; get_key(&key); u64 zero = 0, *val; val = miss_count.lookup_or_init(&key, &zero); ... Signed-off-by: Alexei Starovoitov <[email protected]>

4ast · 2017-09-07T22:15:27Z

cc @yonghong-song @drzaeus77

brendangregg · 2017-09-07T23:57:28Z

Will one of these cats please score a point? I've been watching them for hours.

brendangregg · 2017-09-07T23:58:24Z

I tested it and it works. We're going to need this for python as well.

4ast requested review from drzaeus77 and yonghong-song as code owners September 2, 2017 00:43

drzaeus77 reviewed Sep 2, 2017

View reviewed changes

yonghong-song reviewed Sep 2, 2017

View reviewed changes

yonghong-song reviewed Sep 6, 2017

View reviewed changes

4ast added 2 commits September 6, 2017 19:53

4ast force-pushed the ast_dev branch from c0bf6bc to 4f47e3b Compare September 7, 2017 03:00

4ast requested review from brendangregg and goldshtn as code owners September 7, 2017 03:00

drzaeus77 approved these changes Sep 8, 2017

View reviewed changes

yonghong-song merged commit d56fff0 into master Sep 8, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

annotate program tag #1332

annotate program tag #1332

4ast commented Sep 2, 2017

4ast commented Sep 2, 2017

drzaeus77 commented Sep 2, 2017

4ast commented Sep 2, 2017

drzaeus77 left a comment

drzaeus77 Sep 2, 2017

drzaeus77 Sep 2, 2017

drzaeus77 Sep 2, 2017

4ast Sep 2, 2017

drzaeus77 Sep 2, 2017

drzaeus77 Sep 2, 2017

4ast Sep 2, 2017

palmtenor commented Sep 2, 2017

yonghong-song left a comment

4ast commented Sep 6, 2017

yonghong-song commented Sep 6, 2017

yonghong-song Sep 6, 2017

yonghong-song left a comment

yonghong-song Sep 6, 2017

4ast Sep 6, 2017

yonghong-song Sep 6, 2017

4ast Sep 6, 2017

yonghong-song Sep 6, 2017

4ast Sep 6, 2017

yonghong-song Sep 6, 2017

4ast commented Sep 6, 2017

4ast commented Sep 7, 2017

brendangregg commented Sep 7, 2017

brendangregg commented Sep 7, 2017

annotate program tag #1332

annotate program tag #1332

Conversation

4ast commented Sep 2, 2017

4ast commented Sep 2, 2017

drzaeus77 commented Sep 2, 2017

4ast commented Sep 2, 2017

drzaeus77 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

palmtenor commented Sep 2, 2017

yonghong-song left a comment

Choose a reason for hiding this comment

4ast commented Sep 6, 2017

yonghong-song commented Sep 6, 2017

Choose a reason for hiding this comment

yonghong-song left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

4ast commented Sep 6, 2017

4ast commented Sep 7, 2017

brendangregg commented Sep 7, 2017

brendangregg commented Sep 7, 2017