Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

auto attach BPF functions to kprobes #191

Closed
brendangregg opened this issue Sep 8, 2015 · 12 comments
Closed

auto attach BPF functions to kprobes #191

brendangregg opened this issue Sep 8, 2015 · 12 comments

Comments

@brendangregg
Copy link
Member

I think this is worth suggesting... I keep declaring C function names that only exist for an attach_kprobe() mapping, and wonder if this could be simplified.

Eg, here's the existing hello_world:

b = BPF(text='void hello(void *ctx) { bpf_trace_printk("Hello, World!\\n"); }')
b.attach_kprobe(event="sys_clone", fn_name="hello")
b.trace_print()

imagine:

b = BPF(text='kprobe:sys_clone(void *ctx) { bpf_trace_printk("Hello, World!\\n"); }')
b.trace_print()

I think this will work well with tracepoints, when their support is added; eg, imagine tracing the "sched:sched_process_fork" tracepoint using:

b = BPF(text='sched:sched_process_fork(void *ctx) { bpf_trace_printk("Hello, World!\\n"); }')
b.trace_print()
@brendangregg
Copy link
Member Author

I should note that attach_kprobe() could stay around. This would be an optional shortcut.

@drzaeus77
Copy link
Collaborator

I am hesitant to instrument the C syntax in the way mentioned (sched:sched_process_fork). Though we mangle a little bit the programs that are created, they are still all valid C syntax. If/when we go down the route of a custom tracing language, then this can be introduced.

That said, there is an optimization in the api to be done without changing the supported C syntax. What do you think about this compromise:

BPF(text='int sys_clone(void *ctx) { bpf_trace_printk("Hello, World!\\n"); }').attach_kprobe("sys_clone").trace_print()

The change to support a single function BPF module is a pretty small diff:

diff --git a/src/python/bcc/__init__.py b/src/python/bcc/__init__.py
index 36ef17e..013da1a 100644
--- a/src/python/bcc/__init__.py
+++ b/src/python/bcc/__init__.py
@@ -468,6 +468,13 @@ class BPF(object):
                         cpu=cpu, group_fd=group_fd)
             return

+        # If fn_name is omitted, auto-detect the function in singular case
+        if not fn_name:
+            if len(self.funcs) == 1:
+                fn_name = self.funcs.items()[0][0]
+            else:
+                raise Exception("Missing parameter fn_name not provided")
+
         fn = self.load_func(fn_name, BPF.KPROBE)
         ev_name = "p_" + event.replace("+", "_")
         desc = "p:kprobes/%s %s" % (ev_name, event)

@brendangregg
Copy link
Member Author

Ok, it looks like an improvement. Although could the attach_kprobe() be dropped as well?

@drzaeus77
Copy link
Collaborator

Sure, the precondition will be that the C function name is matching the event.

BPF(text='void sys_clone(void *ctx) { bpf_trace_printk("Hello, World!\\n"); }').trace_print()

@drzaeus77
Copy link
Collaborator

Let me know how #194 looks, which it seems Alexei has already merged :)

@drzaeus77
Copy link
Collaborator

Ok, let me know how this approach seems to you.

@drzaeus77
Copy link
Collaborator

All changes in, please close if it looks good to you.

@brendangregg
Copy link
Member Author

This works well for disksnoop.c, but the next script I tried was bitehist.c, which doesn't use any trace_*() functions, and the kprobe doesn't attach.

@drzaeus77
Copy link
Collaborator

For this my thinking was that it is a fair ask to have the user call attach_kprobe directly as before. Otherwise, b._trace_autoload() should work just fine too.

@brendangregg
Copy link
Member Author

I think funccount is a good example of wanting to run attach_kprobe(), as it's a custom activity. But the bitehist.c usage is simple, like disksnoop.c, and it seems unintuitive to need to switch how it's attached because of how it's later on consumed.

funccount switches how it's attached because the kprobe__ shortcut is insufficient. That one makes sense.

@drzaeus77
Copy link
Collaborator

Fair enough. Let me move _trace_autoload into the BPF constructor.

@brendangregg
Copy link
Member Author

thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants