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

meminfo: Add meminfo corelens module #5

Merged
merged 1 commit into from
Nov 27, 2023

Conversation

jianfenw
Copy link
Member

This commit implements a meminfo corelens module that enables users to dump detailed statistics of the memory management subsystem. The output is similar to 'cat /proc/meminfo', which includes many aspects of the mm subsystem.

This module supports UEK 5, 6, and 7 and for both x86-64 and aarch64. It is tested for all these above settings. For each case, this meminfo module's output is compared against the output of cat /proc/meminfo. Results match closely with only small differences.

@oracle-contributor-agreement
Copy link

Thank you for your pull request and welcome to our community! To contribute, please sign the Oracle Contributor Agreement (OCA).
The following contributors of this PR have not signed the OCA:

To sign the OCA, please create an Oracle account and sign the OCA in Oracle's Contributor Agreement Application.

When signing the OCA, please provide your GitHub username. After signing the OCA and getting an OCA approval from Oracle, this PR will be automatically updated.

If you are an Oracle employee, please make sure that you are a member of the main Oracle GitHub organization, and your membership in this organization is public.

@oracle-contributor-agreement oracle-contributor-agreement bot added the OCA Required At least one contributor does not have an approved Oracle Contributor Agreement. label Oct 24, 2023
@brenns10
Copy link
Member

Hey @jianfenw , please double check the internal docs regarding Github profiles. I see that the "oracle-samples" organization membership is public. Did you make sure to add your Oracle email address to your account?

@jianfenw
Copy link
Member Author

Thanks, Stephen. The OCA check failed because I didn't make my membership in oracle public when I submitted this PR. I made my memberships in oracle and oracle-samples public afterwards. However, the OCA check didn't get updated automatically. I may have to update the commit to trigger a new check later.

@oracle-contributor-agreement oracle-contributor-agreement bot added OCA Verified All contributors have signed the Oracle Contributor Agreement. and removed OCA Required At least one contributor does not have an approved Oracle Contributor Agreement. labels Oct 25, 2023
@jianfenw jianfenw force-pushed the jianfenw-meminfo branch 11 times, most recently from 616eb64 to 2a42897 Compare October 27, 2023 18:03
@brenns10 brenns10 self-requested a review October 27, 2023 18:09
Copy link
Member

@brenns10 brenns10 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a pretty excellent job! You managed to fit a lot of logic into this. Most of my review comments have to do with the use of global variables, but the logic itself seems sound.

doc/api.rst Outdated Show resolved Hide resolved
drgn_tools/meminfo.py Outdated Show resolved Hide resolved
drgn_tools/meminfo.py Outdated Show resolved Hide resolved
drgn_tools/meminfo.py Outdated Show resolved Hide resolved
drgn_tools/meminfo.py Outdated Show resolved Hide resolved
drgn_tools/meminfo.py Outdated Show resolved Hide resolved
drgn_tools/meminfo.py Outdated Show resolved Hide resolved
drgn_tools/meminfo.py Outdated Show resolved Hide resolved
drgn_tools/meminfo.py Outdated Show resolved Hide resolved
drgn_tools/util.py Outdated Show resolved Hide resolved
@jianfenw jianfenw force-pushed the jianfenw-meminfo branch 3 times, most recently from ff2305d to 5abe054 Compare November 2, 2023 22:04
@jianfenw
Copy link
Member Author

jianfenw commented Nov 2, 2023

I've resolved comments and also included uek4 support. This branch has passed all CI tests.
Please review again. Thank you.

Copy link
Member

@brenns10 brenns10 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking good, but still needs some work around caching. This needs to work correctly on live systems, and give always up-to-date statistics each time you run it. That means you need to limit the use of caching. I've provided several suggestions inline.

drgn_tools/meminfo.py Outdated Show resolved Hide resolved
drgn_tools/meminfo.py Outdated Show resolved Hide resolved
drgn_tools/meminfo.py Outdated Show resolved Hide resolved
drgn_tools/meminfo.py Outdated Show resolved Hide resolved
@jianfenw jianfenw force-pushed the jianfenw-meminfo branch 4 times, most recently from 35696fd to dba3892 Compare November 14, 2023 01:02
@jianfenw
Copy link
Member Author

Thank you Stephen. I have updated the commit according to your comments. Appreciate all your wonderful suggestions.

Copy link
Member

@brenns10 brenns10 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have just three little nits here, overall this is good. The most difficult one here is with respect to VMALLOC_END on aarch64, and I'd honestly suggest you just omit the vmalloc info on aarch64, and omit the vmalloc statistic when you go to print everything out.

I checked the internal CI and the failures all have to do with the currently quite flaky in-flight I/O test. I may need to disable that test or find some way to make it a bit better. In any case, all the vmcores passed.

Overall it's looking good and I expect to merge it after these quick changes. Thank you!

drgn_tools/meminfo.py Outdated Show resolved Hide resolved
drgn_tools/meminfo.py Outdated Show resolved Hide resolved
drgn_tools/meminfo.py Outdated Show resolved Hide resolved
This commit implements a meminfo corelens module that enables users to
dump detailed statistics of the memory management subsystem. The output
is similar to 'cat /proc/meminfo', which includes many aspects of the
mm subsystem.

This module supports UEK 4, 5, 6, and 7 and for both x86-64 and aarch64.
It is tested for all these above settings. For each case, this meminfo
module's output is compared against the output of `cat /proc/meminfo`.
Results match closely with only small differences.

Signed-off-by: Jianfeng Wang <[email protected]>
@jianfenw
Copy link
Member Author

Updated this commit accordingly! Thank you, Stephen.

Copy link
Member

@brenns10 brenns10 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for iterating on this, it looks great! I checked the tests, and the only failures on the vmcores were the ones on the in-flight I/O test, which are fixed in #28. So it's good to merge!

@brenns10 brenns10 merged commit 09c5bce into oracle-samples:main Nov 27, 2023
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
OCA Verified All contributors have signed the Oracle Contributor Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants