usdt: Add support for kernel usdt semaphore activation #1567

danobi · 2020-10-21T02:11:37Z

This PR adds support for the kernel's uprobe ref count API. If you
pass the kernel an offset to the usdt semaphore while attaching
a uprobe, the kernel will manage the lifetime of the semaphore.

This is better because:

if bpftrace crashes the semaphore will be deactivated
the kernel does a much better job implementing
--usdt-file-activation than we can in userspace (faster and less
racy)

This is nice because it makes usdt's with semaphore act like a regular
usdt. No need to pass the PID to activate the semaphore.

Checklist

Language changes are updated in docs/reference_guide.md
User-visible and non-trivial changes updated in CHANGELOG.md
The new behaviour is covered by tests

danobi · 2020-10-21T02:20:36Z

Dependent on iovisor/bcc#3135 .

fbs · 2020-10-21T09:17:41Z

docs/reference_guide.md

@@ -1268,7 +1268,18 @@ hi
 ^C
 ```

-bpftrace also supports USDT semaphores. You may activate semaphores by passing in `-p $PID` or
+bpftrace also supports USDT semaphores. If both your environment and bpftrace


Is this a (new) kernel feature or has it been in there for ages? Might be good to list the required version if this requires a modern kernel

danobi · 2020-10-21T14:20:49Z

It came out in 4.20 kernel which came out late 2018. I think it should be fine to leave out.

…

On Wed, Oct 21, 2020, at 2:17 AM, bas smit wrote: ***@***.**** commented on this pull request. In docs/reference_guide.md <#1567 (comment)>: > @@ -1268,7 +1268,18 @@ hi ^C ``` -bpftrace also supports USDT semaphores. You may activate semaphores by passing in `-p $PID` or +bpftrace also supports USDT semaphores. If both your environment and bpftrace Is this a (new) kernel feature or has it been in there for ages? Might be good to list the required version if this requires a modern kernel — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1567 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAGJKJYDI4J42FBEVEYM5RDSL2RMNANCNFSM4SZEBKMQ>.

-- Daniel Xu accounts@dxuuu.xyz

javierhonduco · 2020-10-27T13:09:30Z

cc @dalehamel

dalehamel · 2020-10-27T14:22:55Z

Thanks @javierhonduco i've been keeping an eye on this and iovisor/bcc#3135

danobi · 2020-10-28T18:26:00Z

bcc PR is merged, this is ready for review now

fbs · 2020-11-03T10:44:17Z

Looks good :). @dalehamel do you have time for a review, im not that familiar with usdts

dalehamel · 2020-11-03T14:40:36Z

@fbs i'll give this a closer look this week

dalehamel

Love it. I wonder if we can update github actions for CI so that we run on several versions of ubuntu, as I think that ubuntu 16.04 is still on 4.15 kernel that doesn't have this API, but 20.04 and 18.04 are on 5.4 kernels that do have it.

It would be great to be able to test both code paths (kernel has ref counter offset, kernel doesn't) in CI, but everything looks right to me.

fbs · 2020-11-05T09:07:55Z

@danobi needs a rebase

Will be used in a later commit to see if we can use the kernel's usdt semaphore support.

The function was getting huge, with a bunch of #ifdef's sprinkled around. It was pretty hard to reason about, let alone add more #ifdef branches. This is (hopefully) a non-functional change.

If available, let the kernel manage the semaphore count for the usdt probe. This is better because the kernel can activate the semaphore on all processes that have the target mapped much faster than the current --usdt-file-activation implementation (which walks /proc/*/maps) and greps for strings. It's also better b/c if bpftrace crashes the kernel can reliable decrement the semaphore.

Now you can gate a runtime test on a feature NOT being present. eg: REQUIRES_FEATURE !btf

danobi · 2020-11-05T17:46:50Z

Love it. I wonder if we can update github actions for CI so that we run on several versions of ubuntu, as I think that ubuntu 16.04 is still on 4.15 kernel that doesn't have this API, but 20.04 and 18.04 are on 5.4 kernels that do have it.

@dalehamel I'll file a separate issue for VM tests in CI

danobi · 2020-11-06T00:36:23Z

Don't merge this yet -- I'm investigating the build failure. This is looking to be a really strange bug. Either in bcc or in the kernel.

danobi · 2020-11-06T02:50:30Z

Ok, so it took an entire day of printf debugging the kernel, but I have a suspicion.

I think embedded build's toolchain is having usdt_test_semaphore load its executable image into 2 distinct VMAs while my host's native toolchain is doing 1 VMA. Then to be safe (guessing), b/c the insn exists in both VMAs, the kernel installs a breakpoint in both VMAs (also guessing). Regardless, what the kernel is doing isn't wrong. If it wants to bump the semaphore by 2 instead of 1, so be it. As long as it subtracts by 2 during cleanup (which it does).

I'll update the tests to allow >1 semaphore count bump.

The kernel can do it much faster and reliably. Note that the kernel may increment the semaphore by more than 1. This happens when the executable image is mapped to >1 VMAs. The runtime tests are updated to accept a semaphore value > 0.

If we run two usdt_semaphore_test binaries and both are running a busy loop full speed, then all the events generated by the first binary can overflow the perf buffer and starve out events from the second binary.

dalehamel · 2020-11-06T05:49:07Z

I think embedded build's toolchain is having usdt_test_semaphore load its executable image into 2 distinct VMAs while my host's native toolchain is doing 1 VMA.

That's really bizarre if it depends on the toolchain... maybe it is the BCC version though? I think the embedded builds were still on 0.12 last I checked, but there is a build that does "latest" also, the "edge" build. Is that one also behaving oddly?

Then to be safe (guessing), b/c the insn exists in both VMAs, the kernel installs a breakpoint in both VMAs (also guessing). Regardless, what the kernel is doing isn't wrong. If it wants to bump the semaphore by 2 instead of 1, so be it. As long as it subtracts by 2 during cleanup (which it does).

I think I've seen this exact issue before, but I forget what the conclusion was...

I've found using dd and seeking to the offset in the elf notes for the different addresses of a probe, while the probes are still attached, is a good way to confirm this behavior. IIRC there is also an outstanding bug in bpftrace where we weren't attaching to all USDT probe addresses if there were multiple, but I think that bcc will attach to all of them correctly if you use the kernel API, but I don't think the embedded builds except for the one using the latest bcc would.

danobi · 2020-11-06T17:28:15Z

That's really bizarre if it depends on the toolchain... maybe it is the BCC version though?

I don't think so. I singled stepped the code using GDB and it really was the syscall (perf_event_open) incrementing by 2. I dumped the perf_event_open() args using strace and everything matched.

I could also reproduce the issue with a native build of bpftrace against the embedded built test prog. So I really do think it's the test prog doing some funny stuff here.

IIRC there is also an outstanding bug in bpftrace where we weren't attaching to all USDT probe addresses if there were multiple

I think I fixed that in 17588d2 . Lemme know if it's still buggy and I can take a look

danobi added the do-not-merge Changes are not ready to be merged into master yet label Oct 21, 2020

fbs reviewed Oct 21, 2020

View reviewed changes

danobi removed the do-not-merge Changes are not ready to be merged into master yet label Oct 28, 2020

danobi mentioned this pull request Nov 3, 2020

Embedded build broke #1584

Closed

dalehamel approved these changes Nov 4, 2020

View reviewed changes

fbs approved these changes Nov 5, 2020

View reviewed changes

danobi added 5 commits November 5, 2020 09:45

features: Add feature test for uprobe refcount

f783e5d

NFC: Pass BPFfeature to AttachedProbe

19d68d6

Will be used in a later commit to see if we can use the kernel's usdt semaphore support.

NFC: Split up attach_usdt() into smaller functions

dab29a9

The function was getting huge, with a bunch of #ifdef's sprinkled around. It was pretty hard to reason about, let alone add more #ifdef branches. This is (hopefully) a non-functional change.

runtime-tests: Add negative feature requirement

9ef57b1

Now you can gate a runtime test on a feature NOT being present. eg: REQUIRES_FEATURE !btf

danobi force-pushed the uprobe_refcnt branch from c69ae0e to 38d86e0 Compare November 5, 2020 17:45

danobi added the do-not-merge Changes are not ready to be merged into master yet label Nov 6, 2020

danobi added 4 commits November 5, 2020 19:16

runtime-tests: Print bpftrace --info before running

a5e516c

Fix --usdt-file-activation flakiness

c1f8be5

If we run two usdt_semaphore_test binaries and both are running a busy loop full speed, then all the events generated by the first binary can overflow the perf buffer and starve out events from the second binary.

Update docs and changelog

30a0019

danobi force-pushed the uprobe_refcnt branch from a95f57a to 30a0019 Compare November 6, 2020 03:18

danobi removed the do-not-merge Changes are not ready to be merged into master yet label Nov 6, 2020

danobi merged commit c2fda13 into bpftrace:master Nov 10, 2020

danobi mentioned this pull request Jan 5, 2021

Add usdt file activation namespace runtime test #1331

Closed

2 tasks

danobi mentioned this pull request Mar 16, 2021

docs: Add a manual / language reference / man page #1711

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

usdt: Add support for kernel usdt semaphore activation #1567

usdt: Add support for kernel usdt semaphore activation #1567

danobi commented Oct 21, 2020 •

edited

Loading

danobi commented Oct 21, 2020

fbs Oct 21, 2020

danobi commented Oct 21, 2020 via email

javierhonduco commented Oct 27, 2020

dalehamel commented Oct 27, 2020

danobi commented Oct 28, 2020

fbs commented Nov 3, 2020

dalehamel commented Nov 3, 2020

dalehamel left a comment

fbs commented Nov 5, 2020

danobi commented Nov 5, 2020

danobi commented Nov 6, 2020

danobi commented Nov 6, 2020 •

edited

Loading

dalehamel commented Nov 6, 2020

danobi commented Nov 6, 2020

usdt: Add support for kernel usdt semaphore activation #1567

usdt: Add support for kernel usdt semaphore activation #1567

Conversation

danobi commented Oct 21, 2020 • edited Loading

Checklist

danobi commented Oct 21, 2020

fbs Oct 21, 2020

Choose a reason for hiding this comment

danobi commented Oct 21, 2020 via email

javierhonduco commented Oct 27, 2020

dalehamel commented Oct 27, 2020

danobi commented Oct 28, 2020

fbs commented Nov 3, 2020

dalehamel commented Nov 3, 2020

dalehamel left a comment

Choose a reason for hiding this comment

fbs commented Nov 5, 2020

danobi commented Nov 5, 2020

danobi commented Nov 6, 2020

danobi commented Nov 6, 2020 • edited Loading

dalehamel commented Nov 6, 2020

danobi commented Nov 6, 2020

danobi commented Oct 21, 2020 •

edited

Loading

danobi commented Nov 6, 2020 •

edited

Loading