From 7583a4e24cfebf9bd08883a8efd3d2852568799f Mon Sep 17 00:00:00 2001 From: Alexei Starovoitov Date: Wed, 3 Feb 2016 21:25:43 -0800 Subject: [PATCH 1/6] use __builtin_memcpy() instead of unrolled loop also remove tab and unused import Signed-off-by: Alexei Starovoitov --- tools/offwaketime.py | 37 ++++--------------------------------- 1 file changed, 4 insertions(+), 33 deletions(-) diff --git a/tools/offwaketime.py b/tools/offwaketime.py index 745d5b49b87a..13c8c98ca757 100755 --- a/tools/offwaketime.py +++ b/tools/offwaketime.py @@ -17,11 +17,11 @@ # Copyright 2016 Netflix, Inc. # Licensed under the Apache License, Version 2.0 (the "License") # -# 20-Jan-2016 Brendan Gregg Created this. +# 20-Jan-2016 Brendan Gregg Created this. from __future__ import print_function from bcc import BPF -from time import sleep, strftime +from time import sleep import argparse import signal @@ -184,37 +184,8 @@ def signal_ignore(signal, frame): out: woke = wokeby.lookup(&pid); if (woke) { - // unrolled loop (MAXWDEPTH): - depth = 0; - key.wret[depth] = woke->ret[depth]; depth++; - key.wret[depth] = woke->ret[depth]; depth++; - key.wret[depth] = woke->ret[depth]; depth++; - key.wret[depth] = woke->ret[depth]; depth++; - key.wret[depth] = woke->ret[depth]; depth++; - key.wret[depth] = woke->ret[depth]; depth++; - key.wret[depth] = woke->ret[depth]; depth++; - key.wret[depth] = woke->ret[depth]; depth++; - key.wret[depth] = woke->ret[depth]; depth++; - key.wret[depth] = woke->ret[depth]; - - // unrolled loop (TASK_COMM_LEN): - depth = 0; - key.waker[depth] = woke->name[depth]; depth++; - key.waker[depth] = woke->name[depth]; depth++; - key.waker[depth] = woke->name[depth]; depth++; - key.waker[depth] = woke->name[depth]; depth++; - key.waker[depth] = woke->name[depth]; depth++; - key.waker[depth] = woke->name[depth]; depth++; - key.waker[depth] = woke->name[depth]; depth++; - key.waker[depth] = woke->name[depth]; depth++; - key.waker[depth] = woke->name[depth]; depth++; - key.waker[depth] = woke->name[depth]; depth++; - key.waker[depth] = woke->name[depth]; depth++; - key.waker[depth] = woke->name[depth]; depth++; - key.waker[depth] = woke->name[depth]; depth++; - key.waker[depth] = woke->name[depth]; depth++; - key.waker[depth] = woke->name[depth]; depth++; - key.waker[depth] = woke->name[depth]; + __builtin_memcpy(&key.wret, woke->ret, sizeof(key.wret)); + __builtin_memcpy(&key.waker, woke->name, TASK_COMM_LEN); wokeby.delete(&pid); } From 597b0e36942e9c926a1c3ea6fd0da2e2c6acdbc1 Mon Sep 17 00:00:00 2001 From: Brenden Blanco Date: Tue, 2 Feb 2016 16:07:56 -0800 Subject: [PATCH 2/6] Reorder P4 struct key initializers and blocks The basic_routing.p4 program was failing verification due to missed map key initializers in some paths. Put the goto label at the head of the block and add a " = {}" for each key declaration inside the block. Signed-off-by: Brenden Blanco --- src/cc/frontends/p4/compiler/ebpfTable.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/cc/frontends/p4/compiler/ebpfTable.py b/src/cc/frontends/p4/compiler/ebpfTable.py index 8de97c68dbd9..70ab7a6bfcb0 100644 --- a/src/cc/frontends/p4/compiler/ebpfTable.py +++ b/src/cc/frontends/p4/compiler/ebpfTable.py @@ -275,6 +275,11 @@ def serializeCode(self, serializer, program, nextNode): keyname = "key" valueName = "value" + serializer.newline() + serializer.emitIndent() + serializer.appendFormat("{0}:", program.getLabel(self)) + serializer.newline() + serializer.emitIndent() serializer.blockStart() @@ -283,7 +288,7 @@ def serializeCode(self, serializer, program, nextNode): serializer.newline() serializer.emitIndent() - serializer.appendFormat("struct {0} {1};", self.keyTypeName, keyname) + serializer.appendFormat("struct {0} {1} = {{}};", self.keyTypeName, keyname) serializer.newline() serializer.emitIndent() @@ -291,11 +296,6 @@ def serializeCode(self, serializer, program, nextNode): "struct {0} *{1};", self.valueTypeName, valueName) serializer.newline() - serializer.newline() - serializer.emitIndent() - serializer.appendFormat("{0}:", program.getLabel(self)) - serializer.newline() - self.key.serializeConstruction(serializer, keyname, program) serializer.emitIndent() From 87d2f69a591b29c254f6ce1974a8347d746bfe9f Mon Sep 17 00:00:00 2001 From: Brendan Gregg Date: Fri, 5 Feb 2016 13:36:06 -0800 Subject: [PATCH 3/6] contributing scripts guide --- CONTRIBUTING-SCRIPTS.md | 40 ++++++++++++++++++++++++++++++++++++++++ README.md | 2 ++ 2 files changed, 42 insertions(+) create mode 100644 CONTRIBUTING-SCRIPTS.md diff --git a/CONTRIBUTING-SCRIPTS.md b/CONTRIBUTING-SCRIPTS.md new file mode 100644 index 000000000000..02cae5832de1 --- /dev/null +++ b/CONTRIBUTING-SCRIPTS.md @@ -0,0 +1,40 @@ +# Contributing bcc/eBPF scripts + +If you want to contribute scripts to bcc, or improve your own bcc programs, great! Please read this first. + +_(Written by Brendan Gregg.)_ + +## Type of script + +bcc has 2 types of scripts, in different directories: + +- **/examples**: intended as short examples of bcc & eBPF code. You should focus on keeping it short, neat, and documented (code comments). A submission can just the example code. +- **/tools**: intended as production safe performance and troubleshooting tools. You should focus on it being useful, tested, low overhead, documented (incl. all caveats), and easy to use. A submission should involve 4 changes: the tool, a man page, an example file, and an addition to README.md. Follow [my lead](https://github.com/brendangregg/bcc/commit/9fa156273b395cfc5505f0fff5d6b7b1396f7daa), and see the checklist below. These will be run in mission critical environments as root, so if spending hours testing isn't for you, please submit your idea as an issue instead, or chat with us on irc. + +More detail for each below. + +## Examples + +These are grouped into subdirectories (networking, tracing). Your example can either be a Python program with embedded C (eg, tracing/strlen_count.py), or separate Python and C files (eg, tracing/bitehist.*}). + +As said earlier: keep it short, neat, and documented (code comments). + +## Tools + +A checklist for bcc tool development: + +1. **Research the topic landscape**. Learn the existing tools and metrics (incl. from /proc). Determine what real world problems exist and need solving. We have too many tools and metrics as it is, we don't need more "I guess that's useful" tools, we need more "ah-hah! I couldn't do this before!" tools. Consider asking other developers about your idea. Many of us can be found in IRC, in the #iovisor channel on irc.oftc.net. There's also the mailing list (see the README.md), and github for issues. +1. **Create a known workload for testing**. This might involving writing a 10 line C program, using a microbenchmark, or just improvising at the shell. If you don't know how to create a workload, learn! Figuring this out will provide invaluable context and details that you may have otherwise overlooked. Sometimes it's easy, and I'm able to just use dd(1) from /dev/urandom or a disk device to /dev/null. It lets me set the I/O size, count, and provides throughput statistics for cross-checking checking my tool output. But other times I need a micro-benchhark, or some C. +1. **Write the tool to solve the problem and no more**. Unix philosophy: do one thing and do it well. netstat doesn't have an option to dump packets, tcpdump-style. They are two different tools. +1. **Check your tool correctly measures your known workload**. If possible, run a prime number of events (eg, 23) and check that the numbers match. Try other workload variations. +1. **Use other observability tools to perform a cross-check or sanity check**. Eg, imagine you write a PCI bus tool that shows current throughput is 28 Gbytes/sec. How could you sanity test that? Well, what PCI devices are there? Disks and network cards? Measure their throughput (iostat, nicstat, sar), and check if is in the ballpark of 28 Gbytes/sec (which would include PCI frame overheads). Ideally, your numbers match. +1. **Measure the overhead of the tool**. If you are running a microbenchmark, how much slower is it with the tool running. Is more CPU consumed? Try to determine the worst case: run the microbenchmark so that CPU headroom is exhausted, and then run the bcc tool. Can overhead be lowered? +1. **Test again, and stress test**. You want to discover and fix all the bad things before others hit them. +1. **Consider command line options**. Should it have -p for filtering on a PID? -T for timestamps? -i for interval? See other tools for examples, and copy the style: the usage message should list example usage at the end. Remember to keep the tool doing one thing and doing it well. Also, if there's one option that seems to be the common case, perhaps it should just be the first argument and not need a switch (no -X). A special case of this is *stat tools, like iostat/vmstat/etc, where the convention is [interval [count]]. +1. **Use pep8 to check Python style**: pep8 --show-source --ignore=E123,E125,E126,E127,E128,E302 filename . Note that it misses some things, like consistent usage, so you'll still need to check your double check your script. +1. **Write an _example.txt file**. Copy the style in tools/biolatency_example.txt: start with an intro sentence, then have examples, and finish with the USAGE message. Explain everything: the first example should explain what we are seeing, even if this seems obvious. For some people it won't be obvious. Also explain why we are running the tool: what problems it's solving. It can take a long time (hours) to come up with good examples, but it's worth it. These will get copied around (eg, presentations, articles). +1. **Read your example.txt file**. Does this sound too niche or convoluted? Are you spending too much time explaining caveats? These can be hints that perhaps you should fix your tool, or abandon it! Perhaps it better belongs as an /example, and not a tool. I've abandoned many tools at this stage. +1. **Write a man page**. These are under man/man8. See the other examples. Include a section on overhead, and pull no punches. It's better for end users to know about high overhead beforehand, than to discover it the hard way. Also explain caveats. Don't assume those will be obvious to tool users. +1. **Read your man page**. nroff -man filename. Like before, this exercise is like saying something out loud. Does it sound too niche or convoluted? Again, hints that you might need to go back and fix things, or abandon it. +1. **Add an entry to README.md**. +1. If you made it this far, pull request! diff --git a/README.md b/README.md index ac6d249f0af1..9d1e69035cda 100644 --- a/README.md +++ b/README.md @@ -255,6 +255,7 @@ for k, v in b["stats"].items(): See [INSTALL.md](INSTALL.md) for installation steps on your platform. ## Contributing + Already pumped up to commit some code? Here are some resources to join the discussions in the [IOVisor](https://www.iovisor.org/) community and see what you want to work on. @@ -263,3 +264,4 @@ what you want to work on. * _IRC:_ #iovisor at irc.oftc.net * _IRC Logs:_ https://scrollback.io/iovisor/all * _BCC Issue Tracker:_ [Github Issues](https://github.com/iovisor/bcc/issues) +* _A guide for contributing scripts:_ [CONTRIBUTING-SCRIPTS.md](CONTRIBUTING-SCRIPTS.md) From 0179923e82c681289fce08fd26193ab84764e328 Mon Sep 17 00:00:00 2001 From: Brendan Gregg Date: Fri, 5 Feb 2016 14:40:34 -0800 Subject: [PATCH 4/6] different man formats --- CONTRIBUTING-SCRIPTS.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/CONTRIBUTING-SCRIPTS.md b/CONTRIBUTING-SCRIPTS.md index 02cae5832de1..5284c5b17133 100644 --- a/CONTRIBUTING-SCRIPTS.md +++ b/CONTRIBUTING-SCRIPTS.md @@ -34,7 +34,7 @@ A checklist for bcc tool development: 1. **Use pep8 to check Python style**: pep8 --show-source --ignore=E123,E125,E126,E127,E128,E302 filename . Note that it misses some things, like consistent usage, so you'll still need to check your double check your script. 1. **Write an _example.txt file**. Copy the style in tools/biolatency_example.txt: start with an intro sentence, then have examples, and finish with the USAGE message. Explain everything: the first example should explain what we are seeing, even if this seems obvious. For some people it won't be obvious. Also explain why we are running the tool: what problems it's solving. It can take a long time (hours) to come up with good examples, but it's worth it. These will get copied around (eg, presentations, articles). 1. **Read your example.txt file**. Does this sound too niche or convoluted? Are you spending too much time explaining caveats? These can be hints that perhaps you should fix your tool, or abandon it! Perhaps it better belongs as an /example, and not a tool. I've abandoned many tools at this stage. -1. **Write a man page**. These are under man/man8. See the other examples. Include a section on overhead, and pull no punches. It's better for end users to know about high overhead beforehand, than to discover it the hard way. Also explain caveats. Don't assume those will be obvious to tool users. -1. **Read your man page**. nroff -man filename. Like before, this exercise is like saying something out loud. Does it sound too niche or convoluted? Again, hints that you might need to go back and fix things, or abandon it. +1. **Write a man page**. Either ROFF (.8), markdown (.md), or plain text (.txt): so long as it documents the important sections, particularly columns (fields) and caveats. These go under man/man8. See the other examples. Include a section on overhead, and pull no punches. It's better for end users to know about high overhead beforehand, than to discover it the hard way. Also explain caveats. Don't assume those will be obvious to tool users. +1. **Read your man page**. For ROFF: nroff -man filename. Like before, this exercise is like saying something out loud. Does it sound too niche or convoluted? Again, hints that you might need to go back and fix things, or abandon it. 1. **Add an entry to README.md**. 1. If you made it this far, pull request! From 856b1777b08ee426fe80964c9748e89b827d77e8 Mon Sep 17 00:00:00 2001 From: Brenden Blanco Date: Fri, 5 Feb 2016 14:49:10 -0800 Subject: [PATCH 5/6] Fix segfault in ~BPFModule on syntax error ~BPFModule was segfaulting because tables_ was an empty pointer. The pointer is valid only for valid compilations. Add a test as well. Signed-off-by: Brenden Blanco --- src/cc/bpf_module.cc | 8 +++++--- tests/cc/test_clang.py | 4 ++++ 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/src/cc/bpf_module.cc b/src/cc/bpf_module.cc index 682890917a8c..7e1a409d6e97 100644 --- a/src/cc/bpf_module.cc +++ b/src/cc/bpf_module.cc @@ -114,9 +114,11 @@ BPFModule::~BPFModule() { engine_.reset(); rw_engine_.reset(); ctx_.reset(); - for (auto table : *tables_) { - if (table.is_shared) - SharedTables::instance()->remove_fd(table.name); + if (tables_) { + for (auto table : *tables_) { + if (table.is_shared) + SharedTables::instance()->remove_fd(table.name); + } } } diff --git a/tests/cc/test_clang.py b/tests/cc/test_clang.py index 5811c2274943..87f2bd4e9154 100755 --- a/tests/cc/test_clang.py +++ b/tests/cc/test_clang.py @@ -299,5 +299,9 @@ def test_exported_maps(self): b1 = BPF(text="""BPF_TABLE_PUBLIC("hash", int, int, table1, 10);""") b2 = BPF(text="""BPF_TABLE("extern", int, int, table1, 10);""") + def test_syntax_error(self): + with self.assertRaises(Exception): + b = BPF(text="""int failure(void *ctx) { if (); return 0; }""") + if __name__ == "__main__": main() From d924d3eef6c26eb696a1d5b482622b1d2192fa97 Mon Sep 17 00:00:00 2001 From: Brendan Gregg Date: Fri, 5 Feb 2016 15:37:56 -0800 Subject: [PATCH 6/6] documentation typos --- CONTRIBUTING-SCRIPTS.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/CONTRIBUTING-SCRIPTS.md b/CONTRIBUTING-SCRIPTS.md index 5284c5b17133..7e52b730467d 100644 --- a/CONTRIBUTING-SCRIPTS.md +++ b/CONTRIBUTING-SCRIPTS.md @@ -8,7 +8,7 @@ _(Written by Brendan Gregg.)_ bcc has 2 types of scripts, in different directories: -- **/examples**: intended as short examples of bcc & eBPF code. You should focus on keeping it short, neat, and documented (code comments). A submission can just the example code. +- **/examples**: intended as short examples of bcc & eBPF code. You should focus on keeping it short, neat, and documented (code comments). A submission can just be the example code. - **/tools**: intended as production safe performance and troubleshooting tools. You should focus on it being useful, tested, low overhead, documented (incl. all caveats), and easy to use. A submission should involve 4 changes: the tool, a man page, an example file, and an addition to README.md. Follow [my lead](https://github.com/brendangregg/bcc/commit/9fa156273b395cfc5505f0fff5d6b7b1396f7daa), and see the checklist below. These will be run in mission critical environments as root, so if spending hours testing isn't for you, please submit your idea as an issue instead, or chat with us on irc. More detail for each below. @@ -31,7 +31,7 @@ A checklist for bcc tool development: 1. **Measure the overhead of the tool**. If you are running a microbenchmark, how much slower is it with the tool running. Is more CPU consumed? Try to determine the worst case: run the microbenchmark so that CPU headroom is exhausted, and then run the bcc tool. Can overhead be lowered? 1. **Test again, and stress test**. You want to discover and fix all the bad things before others hit them. 1. **Consider command line options**. Should it have -p for filtering on a PID? -T for timestamps? -i for interval? See other tools for examples, and copy the style: the usage message should list example usage at the end. Remember to keep the tool doing one thing and doing it well. Also, if there's one option that seems to be the common case, perhaps it should just be the first argument and not need a switch (no -X). A special case of this is *stat tools, like iostat/vmstat/etc, where the convention is [interval [count]]. -1. **Use pep8 to check Python style**: pep8 --show-source --ignore=E123,E125,E126,E127,E128,E302 filename . Note that it misses some things, like consistent usage, so you'll still need to check your double check your script. +1. **Use pep8 to check Python style**: pep8 --show-source --ignore=E123,E125,E126,E127,E128,E302 filename . Note that it misses some things, like consistent usage, so you'll still need to double check your script. 1. **Write an _example.txt file**. Copy the style in tools/biolatency_example.txt: start with an intro sentence, then have examples, and finish with the USAGE message. Explain everything: the first example should explain what we are seeing, even if this seems obvious. For some people it won't be obvious. Also explain why we are running the tool: what problems it's solving. It can take a long time (hours) to come up with good examples, but it's worth it. These will get copied around (eg, presentations, articles). 1. **Read your example.txt file**. Does this sound too niche or convoluted? Are you spending too much time explaining caveats? These can be hints that perhaps you should fix your tool, or abandon it! Perhaps it better belongs as an /example, and not a tool. I've abandoned many tools at this stage. 1. **Write a man page**. Either ROFF (.8), markdown (.md), or plain text (.txt): so long as it documents the important sections, particularly columns (fields) and caveats. These go under man/man8. See the other examples. Include a section on overhead, and pull no punches. It's better for end users to know about high overhead beforehand, than to discover it the hard way. Also explain caveats. Don't assume those will be obvious to tool users.