Skip to content

createdump: only dump committed memory - #79853

Merged
hoyosjs merged 3 commits into
dotnet:mainfrom
criteo-forks:createdump_only_dump_committed_memory
Dec 22, 2022
Merged

hoyosjs merged 3 commits into
dotnet:mainfrom
criteo-forks:createdump_only_dump_committed_memory

Conversation

@ezsilmar

Copy link
Copy Markdown
Contributor

Dumping memory regions as they are listed in /proc/pid/maps results in increase of RAM usage of the target application on some Linux kernels.

This change uses /proc/pid/pagemap to check if the page is committed before adding it to the regions list. As the file is not available on kernels 4.0 and 4.1 without elevated permissions there's a fallback to previous behavior.

Fixes #71472

@ghost ghost added area-Diagnostics-coreclr community-contribution Indicates that the PR has been added by a community member labels Dec 20, 2022
@ghost

ghost commented Dec 20, 2022

Copy link
Copy Markdown

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

Issue Details

Dumping memory regions as they are listed in /proc/pid/maps results in increase of RAM usage of the target application on some Linux kernels.

This change uses /proc/pid/pagemap to check if the page is committed before adding it to the regions list. As the file is not available on kernels 4.0 and 4.1 without elevated permissions there's a fallback to previous behavior.

Fixes #71472

Author: ezsilmar
Assignees: -
Labels:

area-Diagnostics-coreclr

Milestone: -

@hoyosjs hoyosjs left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Minor comments but this is looking great. Thanks @ezsilmar! I'll try to test this in a few important environments to check we are not regressing support for any important scenario.

Comment thread src/coreclr/debug/createdump/crashinfo.cpp Outdated
Comment thread src/coreclr/debug/createdump/crashinfo.cpp Outdated
Comment thread src/coreclr/debug/createdump/crashinfo.cpp Outdated
@hoyosjs
hoyosjs requested a review from janvorli December 21, 2022 03:29

@janvorli janvorli left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, thank you!

@ezsilmar

Copy link
Copy Markdown
Contributor Author

@ezsilmar
ezsilmar force-pushed the createdump_only_dump_committed_memory branch from 3acb407 to 3c36826 Compare December 21, 2022 14:01
@ezsilmar

Copy link
Copy Markdown
Contributor Author

Oh I messed up the branch trying to update it to the tip of the main... I'll try to fix it

Dumping memory regions as they are listed in /proc/pid/maps
results in increase of RAM usage of the target application
on some Linux kernels.

This change uses /proc/pid/pagemap to check if the page is committed
before adding it to the regions list. As the file is not available on
kernels 4.0 and 4.1 without elevated permissions there's a fallback to
previous behavior.
@ezsilmar
ezsilmar force-pushed the createdump_only_dump_committed_memory branch from 3c36826 to 07d8aff Compare December 21, 2022 14:06
@hoyosjs

hoyosjs commented Dec 22, 2022

Copy link
Copy Markdown
Member

Error is known and tracked in build analysis

@hoyosjs
hoyosjs merged commit f85d515 into dotnet:main Dec 22, 2022
@hoyosjs

hoyosjs commented Dec 22, 2022

Copy link
Copy Markdown
Member

Thanks @ezsilmar!

@afilatov-st

Copy link
Copy Markdown

Is it possible to get the fixed createdump binary or compile it locally so we can use it with .NET 6?

ezsilmar added a commit to criteo-forks/runtime that referenced this pull request Dec 22, 2022
Dumping memory regions as they are listed in /proc/pid/maps
results in increase of RAM usage of the target application
on some Linux kernels.

This change uses /proc/pid/pagemap to check if the page is committed
before adding it to the regions list. As the file is not available on
kernels 4.0 and 4.1 without elevated permissions there's a fallback to
previous behavior.
Conflicts:
	src/coreclr/debug/createdump/crashinfo.cpp
	src/coreclr/debug/createdump/crashinfo.h
@hoyosjs

hoyosjs commented Dec 26, 2022 •

Copy link
Copy Markdown
Member

createdump uses libmscordaccore. There's been quite a few changes from 8.0 to 6.0. This change would need to be ported to 6.0 - I'll need to do some testing, but generally I've heard interest to take it into servicing. I'll prepare the patch, but it might be a while for it to go into the proper released product.

@hoyosjs

hoyosjs commented Dec 27, 2022

Copy link
Copy Markdown
Member

/backport to release/7.0

@github-actions

Copy link
Copy Markdown
Contributor

Started backporting to release/7.0: https://github.com/dotnet/runtime/actions/runs/3784244575

@ghost ghost locked as resolved and limited conversation to collaborators Jan 26, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-Diagnostics-coreclr community-contribution Indicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

dotnet-dump makes process to double its used memory and fails

5 participants