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

createdump: only dump committed memory#79853
hoyosjs merged 3 commits into
dotnet:mainfrom
criteo-forks:createdump_only_dump_committed_memory

Conversation

@ezsilmar

@ezsilmarezsilmar commented Dec 20, 2022

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

@ghostghost 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:-

@build-analysisbuild-analysisBot mentioned this pull request Dec 20, 2022
Closed
hoyosjs
hoyosjs approved these changes Dec 21, 2022
View reviewed changes

@hoyosjshoyosjs 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 threadsrc/coreclr/debug/createdump/crashinfo.cpp Outdated
Comment threadsrc/coreclr/debug/createdump/crashinfo.cpp Outdated
Comment threadsrc/coreclr/debug/createdump/crashinfo.cpp Outdated
janvorli
janvorli approved these changes Dec 21, 2022
View reviewed changes

@janvorlijanvorli 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

ezsilmar commented Dec 21, 2022

Copy link
Copy Markdown
ContributorAuthor

The same ComInterfaceGenerator.Unit.Tests is failing on the CI, I think it's not related: https://helixre107v0xdeko0k025g8.blob.core.windows.net/dotnet-runtime-refs-pull-79853-merge-812d7bc7f4264ef99c/ComInterfaceGenerator.Unit.Tests/1/console.50e18f2c.log?helixlogtype=result

@ezsilmar
ezsilmarforce-pushed the createdump_only_dump_committed_memory branch from 3acb407 to 3c36826CompareDecember 21, 2022 14:01
@ezsilmar

ezsilmar commented Dec 21, 2022

Copy link
Copy Markdown
ContributorAuthor

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

@ezsilmar
createdump: only dump committed memory
ac585a6
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
do not try to read pagemap on macos
7f46e2f
@ezsilmar
Address review comments
07d8aff
@ezsilmar
ezsilmarforce-pushed the createdump_only_dump_committed_memory branch from 3c36826 to 07d8affCompareDecember 21, 2022 14:06
@build-analysisbuild-analysisBot mentioned this pull request Dec 21, 2022
Closed
@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:mainDec 22, 2022
@hoyosjs

hoyosjs commented Dec 22, 2022

Copy link
Copy Markdown
Member

Thanks @ezsilmar!

@afilatov-st

afilatov-st commented Dec 22, 2022

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
@ezsilmar
createdump: only dump committed memory (dotnet#79853)
ea1c6bc
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•
edited
Loading

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

github-actionsBot commented Dec 27, 2022

Copy link
Copy Markdown
Contributor

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

@github-actionsgithub-actionsBot mentioned this pull request Dec 27, 2022
Closed
@hoyosjshoyosjs mentioned this pull request Dec 27, 2022
Closed
@ghostghost locked as resolved and limited conversation to collaborators Jan 26, 2023
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@janvorlijanvorlijanvorli approved these changes
@hoyosjshoyosjshoyosjs approved these changes
+1 more reviewer
@mikem8361mikem8361mikem8361 approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area-Diagnostics-coreclrcommunity-contributionIndicates that the PR has been added by a community member

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

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

5 participants

@ezsilmar@hoyosjs@afilatov-st@mikem8361@janvorli