bcc/python: Add support for API 'bpf_attach_perf_event_raw' in BPF py…

[athira-rajeev]

…thon interface

Add python interface for attach_perf_event_raw to bcc.
The bpf_attach_perf_event_raw API provide flexibility to use
advanced features of perf events with BPF. Presently, this
API is available to use in BPF programs via C and C++ interface.
Patch enables support to use in python interface.

Patch also adds testcase under 'tests/python' which uses
the newly added python interface 'attach_perf_event_raw'.

Signed-off-by: Athira Rajeev [email protected]

[yonghong-song]

[buildbot, test this please]

[yonghong-song]

@davemarchevsky could you help take a look?

[davemarchevsky]

what's this import being used for?

[athira-rajeev]

@davemarchevsky Hi Dave,

This import is not needed. Thanks for the catch. I will remove this in next revision.

[davemarchevsky]

Can the ctype struct Perf.perf_event_attr() be used as the first type here?

[athira-rajeev]

@davemarchevsky
Sure, I will make the changes to use perf_event_attr as first type.

Does changes below looks good ?

index 959296e3..d888473b 100644
--- a/src/python/bcc/libbcc.py
+++ b/src/python/bcc/libbcc.py
@@ -147,6 +147,10 @@ lib.bpf_attach_perf_event.restype = ct.c_int
 lib.bpf_attach_perf_event.argtype = [ct.c_int, ct.c_uint, ct.c_uint, ct.c_ulonglong, ct.c_ulonglong,
         ct.c_int, ct.c_int, ct.c_int]
 
+from .perf import Perf
+lib.bpf_attach_perf_event_raw.restype = ct.c_int
+lib.bpf_attach_perf_event_raw.argtype = [Perf.perf_event_attr, ct.c_uint, ct.c_uint, ct.c_uint, ct.c_uint]
+
 lib.bpf_close_perf_event_fd.restype = ct.c_int
 lib.bpf_close_perf_event_fd.argtype = [ct.c_int]

[davemarchevsky]

you may need to do perf_event_attr(), but otherwise looks good

[athira-rajeev]

Thanks for checking Dave @davemarchevsky

I have updated pull request with the changes you suggested and also corrected the type of event attr in testcase to proper one. Please review the updated changes.

[davemarchevsky]

[buildbot, ok to test]

[davemarchevsky]

could you move this import to the top of the file?

[athira-rajeev]

@davemarchevsky Hi Dave,

I had tried putting it to top of file ( right after import of ctypes ) , but there is import dependency on lib with that change.

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/lib/python3.6/site-packages/bcc/__init__.py", line 26, in <module>
    from .libbcc import lib, bcc_symbol, bcc_symbol_option, bcc_stacktrace_build_id, _SYM_CB_TYPE
  File "/usr/lib/python3.6/site-packages/bcc/libbcc.py", line 16, in <module>
    from .perf import Perf
  File "/usr/lib/python3.6/site-packages/bcc/perf.py", line 17, in <module>
    from .utils import get_online_cpus
  File "/usr/lib/python3.6/site-packages/bcc/utils.py", line 20, in <module>
    from .libbcc import lib
ImportError: cannot import name 'lib'

I can put it at top but after we define lib.

index 959296e3..ea19406b 100644
--- a/src/python/bcc/libbcc.py
+++ b/src/python/bcc/libbcc.py
@@ -16,6 +16,7 @@ import ctypes as ct
 
 lib = ct.CDLL("libbcc.so.0", use_errno=True)
 
+from .perf import Perf
 # keep in sync with bcc_common.h
 lib.bpf_module_create_b.restype = ct.c_void_p
 lib.bpf_module_create_b.argtypes = [ct.c_char_p, ct.c_char_p, ct.c_uint,
@@ -147,6 +148,9 @@ lib.bpf_attach_perf_event.restype = ct.c_int
 lib.bpf_attach_perf_event.argtype = [ct.c_int, ct.c_uint, ct.c_uint, ct.c_ulonglong, ct.c_ulonglong,
         ct.c_int, ct.c_int, ct.c_int]
 
+lib.bpf_attach_perf_event_raw.restype = ct.c_int
+lib.bpf_attach_perf_event_raw.argtype = [Perf.perf_event_attr(), ct.c_uint, ct.c_uint, ct.c_uint, ct.c_uint]
+
 lib.bpf_close_perf_event_fd.restype = ct.c_int
 lib.bpf_close_perf_event_fd.argtype = [ct.c_int]

Does that look fine ?

Thanks
Athira

[davemarchevsky]

Ah, I see what you mean. I think it's still preferable to keep imports as close to the beginning of the file as possible, even if this one needs to follow the lib line

[athira-rajeev]

Sure, will move this import to begining of file after lib line

[davemarchevsky]

I tried running test_attach_perf_event.py, noticed that it seemed to work fine except for addr always being 0 on my system (5.2.9 kernel), then spent some time digging into why that might be. Ended up fixing a few other minor issues in the script, so instead of a few PR comments here's the diff between my changed script and the one in this PR:

@@ -29,14 +29,11 @@
 int on_sample_hit(struct bpf_perf_event_data *ctx) {
     struct key_t key = {};
     get_key(&key);
-    u64 addr;
-    struct bpf_perf_event_data_kern *kctx;
-    struct perf_sample_data *data;
+    u64 addr = 0;

-    kctx = (struct bpf_perf_event_data_kern *)ctx;
-    bpf_probe_read(&data, sizeof(struct perf_sample_data*), &(kctx->data));
-    if (data)
-        bpf_probe_read(&addr, sizeof(u64), &(data->addr));
+    if(bpf_probe_read_kernel(&addr, sizeof(addr), &ctx->addr)) {
+        bpf_trace_printk("error in bpf_probe_read_kernel!\\n");
+    }

     bpf_trace_printk("Hit a sample with pid: %ld, comm: %s, addr: 0x%llx\\n", key.pid, key.name, addr);
     return 0;
@@ -47,11 +44,11 @@
         b = BPF(text=bpf_text)
         try:
             event_attr = Perf.perf_event_attr()
-            event_attr.type = 0
+            event_attr.type = Perf.PERF_TYPE_HARDWARE
             event_attr.config = PerfHWConfig.CACHE_MISSES
             event_attr.sample_period = 1000000
-            event_attr.sample_type = 0x8
-            event_attr.exclude_kernel = 1;
+            event_attr.sample_type = 0x8  # PERF_SAMPLE_ADDR
+            event_attr.exclude_kernel = 1
             b.attach_perf_event_raw(attr=event_attr, fn_name="on_sample_hit", pid=-1, cpu=-1)
         except Exception:
             print("Failed to attach to a raw event. Please check the event attr used")

Aside from a few nit-level fixes in the python 'runner', the real change here is to not treat ctx like a struct bpf_perf_event_data_kern and instead read its addr field. Looks like that there are no existing examples of reading the access address in this repo and some confusion (#1756) around how to do so, so this test can serve as a convenient example.

[davemarchevsky]

@yonghong-song tells me that bpf_probe_read_kernel(&addr, sizeof(addr), &ctx->addr) may be incorrect, so please give me a day or two to figure out what's going on here

[athira-rajeev]

@yonghong-song tells me that bpf_probe_read_kernel(&addr, sizeof(addr), &ctx->addr) may be incorrect, so please give me a day or two to figure out what's going on here

Sure, Thanks for the help.

I could read the addr from perf sample via bpf_probe_read in my test environment.
IIUC, we can use bpf_probe_read to access perf_sample_data and read addr since this field gets filled in before perf_prepare_sample() in kernel, ie before BPF callback happens.

[davemarchevsky]

@athira-rajeev which kernel is your test environment running?

[davemarchevsky]

This part of my previous suggestion was definitely incorrect

+    if(bpf_probe_read_kernel(&addr, sizeof(addr), &ctx->addr)) {
+        bpf_trace_printk("error in bpf_probe_read_kernel!\\n");
+    }

The way you're reading addr here is fine although struct bpf_perf_event_data_kern is not guaranteed to be stable. Doing addr = ctx->addr and relying on the verifier to rewrite the access is the correct way to do this, but for some reason I'm having trouble getting that to work with this program in my test env, while other programs which do similar do work. It's likely that this is a problem with my test environment, and all this addr stuff is a significant digression from the main purpose of this PR, so let's proceed with the addr reading you're currently doing.

I think the nit-level changes suggested in my previous long comment are still valid, though.

[athira-rajeev]

@athira-rajeev which kernel is your test environment running?

Hi Dave @davemarchevsky

I used upstream kernel and powerpc environment.

[athira-rajeev]

This part of my previous suggestion was definitely incorrect

+    if(bpf_probe_read_kernel(&addr, sizeof(addr), &ctx->addr)) {
+        bpf_trace_printk("error in bpf_probe_read_kernel!\\n");
+    }

The way you're reading addr here is fine although struct bpf_perf_event_data_kern is not guaranteed to be stable. Doing addr = ctx->addr and relying on the verifier to rewrite the access is the correct way to do this, but for some reason I'm having trouble getting that to work with this program in my test env, while other programs which do similar do work. It's likely that this is a problem with my test environment, and all this addr stuff is a significant digression from the main purpose of this PR, so let's proceed with the addr reading you're currently doing.

I think the nit-level changes suggested in my previous long comment are still valid, though.

Sure, I will make those nit-level changes in next version

[athira-rajeev]

@davemarchevsky Hi Dave,
I have updated pull request with the changes you suggested. Please review the updated changes.

[davemarchevsky]

LGTM. Thanks for the contribution!

I figured out why I wasn't seeing actual addrs. Two issues:

I used upstream kernel and powerpc environment.

powerpc will try to set ->addr on any perf event with PERF_SAMPLE_ADDR enabled, while my x86 machine requires perf_event_attr.precise_ip to be set for the event in this test in order to write a nonzero addr.

I tried to set this additional field on the perf_event in this test and still was not seeing non-zero addrs. This was because the ctypes Structure didn't have precise_ip in its _fields_, just had a flags u64. So although precise_ip was being set on the python object, this had no effect on the struct passed to C.

So this unearthed two quick improvements that I'll make:

• the perf_event_attr ctype should be fleshed out more, and perhaps should complain when a field that won't make it into the struct is set
• I should add a test similar to the one in this PR, but operating on a SW perf event like PERF_COUNT_SW_PAGE_FAULTS, whose addr-setting behavior does not vary across platforms, so there's a good example to point to

[athira-rajeev]

LGTM. Thanks for the contribution!

I figured out why I wasn't seeing actual addrs. Two issues:

I used upstream kernel and powerpc environment.

powerpc will try to set ->addr on any perf event with PERF_SAMPLE_ADDR enabled, while my x86 machine requires perf_event_attr.precise_ip to be set for the event in this test in order to write a nonzero addr.

I tried to set this additional field on the perf_event in this test and still was not seeing non-zero addrs. This was because the ctypes Structure didn't have precise_ip in its _fields_, just had a flags u64. So although precise_ip was being set on the python object, this had no effect on the struct passed to C.

So this unearthed two quick improvements that I'll make:

* the perf_event_attr ctype should be fleshed out more, and perhaps should complain when a field that won't make it into the struct is set

* I should add a test similar to the one in this PR, but operating on a SW perf event like `PERF_COUNT_SW_PAGE_FAULTS`, whose `addr`-setting behavior does not vary across platforms, so there's a good example to point to

@davemarchevsky Thanks Dave for review and merging.

Great we could find these improvements. Please tag me in your next changes and I can contribute with testing them in powerpc platform.

Thanks
Athira