-
Notifications
You must be signed in to change notification settings - Fork 4k
Reduce CPU overhead of per-event tools by allowing more buffering #1033
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
Comments
A problem of a built-in sleep to kprobe_poll() would be Ctrl-C behavior. Maybe it's fine if it just lets it quit early, but the calling program would need to know Ctrl-C was hit so it can begin the exit code path. |
In my opinion, the best way is to implement it in kernel and to configure through ioctl(), like ttys and sockets do :-)
Most likely no. I believe, much better is an example of this good practice in the documentation.
I prefer something about 30 ms.
I don't like the current handling of Ctrl-C at all. Probably it is a good idea to redevelop or to refactor it. |
I'd allow a 30 ms sleep through an argument:
It's about finding the balance: too short and the tool wakes up too often and consumes CPU. Too long and the tool may overflow its buffer. I'm guessing a 100 ms default for all tools would work, then we tune some as needed. |
Not sure whether @brendangregg is this what you want? |
I'm wondering about the status of this. So everything is using b.perf_buffer_poll() now? That has a timeout argument, but I don't think that's what we want. We want it to sleep and not wake up for a duration, to allow things to buffer. So it should have a sleep= argument as well. The bigger question is whether we make it the default, eg, 50ms for 20 wakeups per second. Fewer wakeups is good, but we don't want to overflow the buffer. |
Do I understand it correctly that the ideal behavior would roughly be to “wake up” when the buffer gets reasonably filled rather than when it's just non-empty? |
@pavlix that will be ideal. But the default value is very hard to choose. We do not want to overflow the buffer. |
Has this change of a sleep value to perf_buffer_poll been included? This would be very useful for perf events. |
@hsane2001 No. The timeout value for |
@yonghong-song - There seems to be a wide variation of the buffer's fill rate between Linux perf in counting mode or profiling (record) mode, with the latter easily leading to lost samples. I assumed the intention was to get some indication to wakeup when the buffer is full so we could drain it in user space and restart collection. |
|
The perf ring buffer in bcc should be reworked to make sense, to find a balance between buffering, dropping event,s and consuming CPU. We don't want tools burning too much CPU (as described above) or dropping events (e.g., #4219). We also would prefer to fix this in bcc rather than have people add custom fixes in each tool, although that may be unavoidable in some special cases. For example, #4219 (adding "-b"/"--buffer-pages" to opensnoop) doesn't feel like a special case so hopefully that should be done via bcc for all tools instead (and hopefully at some point we can remove -b from opesnoop). |
Not a big problem yet, but at some point we should look into this...
The normal perf_output-consuming loop we've been using is:
This wakes up as quickly as it can, and can consume noticeable CPU. I had to tune it for my cpuunclaimed tool:
This ticket is to explore doing this for more tools. Should b.kprobe_poll() have a built-in sleep, that can be tuned? Eg, 100ms by default?
This is more practical now we have #997, and can increase the buffer size.
The text was updated successfully, but these errors were encountered: