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

Add support for fast memory monitoring using smaps_rollup #234

Merged
merged 4 commits into from
Jun 3, 2024

Conversation

amete
Copy link
Collaborator

@amete amete commented Aug 3, 2023

This PR adds the so-called fast memory monitoring option, which uses the smaps_rollup file instead of the standard smaps file, as discussed in #219.

There are some limitation though. Most notably the Size metric is missing in the pre-summed file, where we get the virtual memory measurements, and the fact that smaps_rollup is not supported by some older kernels, e.g., cc7. Therefore, we probably need to do a little bit of testing before using this new mode for anything serious.

Closes #219

@amete amete added the enhancement New feature or request label Aug 3, 2023
@amete amete added this to the v3.1 milestone Aug 3, 2023
@amete amete self-assigned this Aug 3, 2023
Copy link
Member

@graeme-a-stewart graeme-a-stewart left a comment

Choose a reason for hiding this comment

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

There's one minor problem with the get_longopts that's trivial.

My larger concern is that when you switch this option on the reported vmem is 0. That's just wrong and I don't like using 0 as a magic value that means sorry, we couldn't measure this.

My proposal is that we'd rather drop that value from the reported statistics, which would be less confusing. Plus, that definitely needs documentation!

package/src/prmon.cpp Outdated Show resolved Hide resolved
@amete
Copy link
Collaborator Author

amete commented May 3, 2024

Wow, I didn't realize this slipped through the cracks for so long @graeme-a-stewart 😄 Thanks a lot for the review. I tweaked the implementation in that if a particular metric is not available, we print a warning message and drop it from the list of monitored metrics. I also fixed the typo in the options. How does this look from a functional point of view? If it looks OK, I can put together a few words in the README for the new functionality and we can probably push this through. Please let me know, many thanks.

Copy link
Member

@graeme-a-stewart graeme-a-stewart left a comment

Choose a reason for hiding this comment

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

Hi @amete - this is looking good now. I only have one code suggestion (downgrade the warning to info). And the README will need to be updated.

package/src/memmon.cpp Outdated Show resolved Hide resolved
@amete
Copy link
Collaborator Author

amete commented May 31, 2024

Thanks a lot @graeme-a-stewart. I just pushed a new version. Can you let me know how this looks, please?

Copy link
Member

@graeme-a-stewart graeme-a-stewart left a comment

Choose a reason for hiding this comment

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

Looks good - thanks @amete

@graeme-a-stewart graeme-a-stewart merged commit 86b4b13 into main Jun 3, 2024
7 checks passed
@graeme-a-stewart graeme-a-stewart deleted the main-rollup branch June 3, 2024 06:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for smaps_rollup
2 participants