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

prof: use /proc/<pid>/task/<pid>/maps on Linux #227

Closed
wants to merge 1 commit into
base: dev
from

Conversation

Projects
None yet
2 participants
@dhobsd
Copy link

dhobsd commented Apr 29, 2015

In software with many thousands of maps (due to high thread counts), reading
reading from /proc/<pid>/maps on Linux requires n^2 scans of the thread group
list just to annotate thread stacks. This gets very slow very quickly.

Since this information is not useful at all for heap profiling, and reading
from /proc/<pid>/task/<pid>/maps does not attempt to annotate maps as thread
stacks, we should simply read from this latter file when we dump maps.

An example from an actual production machine:

dho@xxx:~$ sudo /usr/bin/time cat /proc/177063/maps > /dev/null
0.00user 82.99system 1:33.53elapsed 88%CPU (0avgtext+0avgdata 2608maxresident)k
0inputs+0outputs (0major+204minor)pagefaults 0swaps

dho@xxx:~$ sudo /usr/bin/time cat /proc/177063/task/177063/maps > /dev/null
0.00user 0.12system 0:00.12elapsed 96%CPU (0avgtext+0avgdata 2592maxresident)k
0inputs+0outputs (0major+204minor)pagefaults 0swaps
@jasone

This comment has been minimized.

Copy link
Member

jasone commented Apr 29, 2015

Is /proc/<pid>/task/<pid>/maps a long-standing procfs feature? If not, we need to protect the added code with a feature test. Otherwise your patch can simply replace the /proc/<pid>/maps case.

@dhobsd

This comment has been minimized.

Copy link

dhobsd commented Apr 29, 2015

proc(5) says it's been around since since 2.6.0-test6. Stack annotation was introduced in 3.4 though, so perhaps I should test around that. If you need 2.4 compatibility, that's probably the best route.

@dhobsd dhobsd force-pushed the dhobsd:dho/task-map branch 2 times, most recently from 4d09f8d to aeb06cd Apr 29, 2015

@dhobsd

This comment has been minimized.

Copy link

dhobsd commented Apr 29, 2015

Ok, I've added an --enable-task-maps option, off by default. I could add some awk -F. sourcery around uname -r to do version detection if you'd prefer going the route of just defaulting to use this file on Linux kernel versions > 3.4, but this seemed more intrusive to configure.ac (and possibly more annoying for dealing with non-Linux systems wrt autodetection doing the right thing because introducing more global state to autoconf).

@jasone

This comment has been minimized.

Copy link
Member

jasone commented Apr 30, 2015

After considering for a while the best default for the task-maps option, I realized that we could instead just sequentially try opening /proc/<pid>/task/<pid>/maps (which will always work on modern systems), then /proc/<pid>/maps if that fails. Can you think of any problems with that approach?

prof: use /proc/<pid>/task/<pid>/maps on Linux
In software with many thousands of maps (due to high thread counts), reading
reading from /proc/<pid>/maps on Linux requires n^2 scans of the thread group
list just to annotate thread stacks. This gets very slow very quickly.

Since this information is not useful at all for heap profiling, and reading
from /proc/<pid>/task/<pid>/maps does not attempt to annotate maps as thread
stacks, we should simply read from this latter file when we dump maps.

This file was added to procfs in Linux `2.6.0-test6`, though the stack
annotation didn't make it in until `3.4.0`. To support older systems, if we
are unable to open this file, we fall back to opening `/proc/<pid>/maps` as
the old behavior.

An example from an actual production machine:

dho@xxx:~$ sudo /usr/bin/time cat /proc/177063/maps > /dev/null
0.00user 82.99system 1:33.53elapsed 88%CPU (0avgtext+0avgdata 2608maxresident)k
0inputs+0outputs (0major+204minor)pagefaults 0swaps

dho@xxx:~$ sudo /usr/bin/time cat /proc/177063/task/177063/maps > /dev/null
0.00user 0.12system 0:00.12elapsed 96%CPU (0avgtext+0avgdata 2592maxresident)k
0inputs+0outputs (0major+204minor)pagefaults 0swaps

@dhobsd dhobsd force-pushed the dhobsd:dho/task-map branch from aeb06cd to 25586a6 Apr 30, 2015

@dhobsd

This comment has been minimized.

Copy link

dhobsd commented Apr 30, 2015

The switch case is a little tricky, but I'm prettty sure it's correct and the tests pass. I don't have a Linux 2.4 system to test the fallback code on. Technical correctness / verifiability are the only concerns I really have with this version of the solution. It's certainly a more intrusive change, but this was the best way I could think of to keep things reasonably clean and avoid duplicating a bunch of the code.

@jasone

This comment has been minimized.

Copy link
Member

jasone commented May 1, 2015

I have a simpler equivalent patch I'm going to push in a moment. Thanks for your efforts on this issue, and please keep submitting patches as you discover issues. I don't make a habit of rewriting people's submissions, so please don't let this instance deter you. =)

@jasone jasone closed this in 8e33c21 May 1, 2015

@dhobsd dhobsd deleted the dhobsd:dho/task-map branch May 1, 2015

@dhobsd

This comment has been minimized.

Copy link

dhobsd commented May 1, 2015

Thanks!

@dhobsd

This comment has been minimized.

Copy link

dhobsd commented May 2, 2015

(Now that I have some time -- for clarity, wasn't sure whether just dropping a block in the middle was cool -- hopefully I'll have some further input to development and style will cease being a factor. Thanks again for including this!)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment