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

[release/6.0] CreateDump: Only add pages with committed memory #80005

Closed

Conversation

hoyosjs
Copy link
Member

@hoyosjs hoyosjs commented Dec 27, 2022

Backport of #79853 to release/7.0, issue #71472

Creating a dump against some kernel implementations is committing empty pages with the page probing technique we currently use during memory enumeration. This change uses the pagemap API's in the /proc system if available - both regarding existence and permissions - along with the maps api to decide what pages contain relevant information. This results in smaller dumps and no increase in resident memory usage when collecting a dump.

Customer Impact

Customers have reported memory doubling in processes they dump. This is fatal environments such as K8s and cgroups hosting, where the process of collecting a dump often results of a OOM kill. This makes crash and ad-hoc diagnostics harder than needs be.

Testing

TBD

Risk

Low - fallback to prior code paths is enabled. Also, it's possible to explicitly disable the new behavior by setting the env var DbgDisablePagemapUse.

Creating a dump against some kernel implementations is committing empty pages with the page probing technique we currently use during memory enumeration. This change uses the `pagemap` API's in the `/proc` system if available - both regarding existence and permissions - along with the `maps` api to decide what pages contain relevant information. This results in smaller dumps and no increase in resident memory usage when collecting a dump.

Co-authored-by: Eugene Zhirov <e.zhirov@criteo.com>
@hoyosjs hoyosjs added the Servicing-consider Issue for next servicing release review label Dec 27, 2022
@hoyosjs hoyosjs added this to the 6.0.x milestone Dec 27, 2022
@hoyosjs hoyosjs requested a review from mikem8361 December 27, 2022 23:37
@hoyosjs hoyosjs self-assigned this Dec 27, 2022
@ghost
Copy link

ghost commented Dec 27, 2022

Tagging subscribers to this area: @tommcdon
See info in area-owners.md if you want to be subscribed.

Issue Details

Backport of #79853 to release/7.0, issue #71472

Creating a dump against some kernel implementations is committing empty pages with the page probing technique we currently use during memory enumeration. This change uses the pagemap API's in the /proc system if available - both regarding existence and permissions - along with the maps api to decide what pages contain relevant information. This results in smaller dumps and no increase in resident memory usage when collecting a dump.

Customer Impact

Customers have reported memory doubling in processes they dump. This is fatal environments such as K8s and cgroups hosting, where the process of collecting a dump often results of a OOM kill. This makes crash and ad-hoc diagnostics harder than needs be.

Testing

TBD

Risk

Low - fallback to prior code paths is enabled.

Author: hoyosjs
Assignees: hoyosjs
Labels:

Servicing-consider, area-Diagnostics-coreclr

Milestone: 6.0.x

@mikem8361
Copy link
Member

We may want a opt-in/out env variable to control using the page map code. Tom and I briefly talked about this before vacation.

/cc: @tommcdon

@hoyosjs
Copy link
Member Author

hoyosjs commented Dec 28, 2022

Happy to add that as long as it is opt out.

@jeffschwMSFT jeffschwMSFT removed the Servicing-consider Issue for next servicing release review label Jan 3, 2023
@mikem8361 mikem8361 self-assigned this Jan 3, 2023
@jeffschwMSFT jeffschwMSFT added the Servicing-consider Issue for next servicing release review label Jan 3, 2023
Copy link
Member

@jeffschwMSFT jeffschwMSFT left a comment

Choose a reason for hiding this comment

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

approved. we will take for consideration in 6.0.x

@hoyosjs hoyosjs added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Jan 5, 2023
@jeffschwMSFT jeffschwMSFT removed the Servicing-consider Issue for next servicing release review label Jan 10, 2023
@hoyosjs hoyosjs marked this pull request as draft January 12, 2023 23:27
@ghost ghost closed this Mar 12, 2023
@ghost
Copy link

ghost commented Mar 12, 2023

Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it.

@ghost ghost locked as resolved and limited conversation to collaborators Apr 12, 2023
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Diagnostics-coreclr NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants