createdump can read memory with process_vm_readv on Linux #420

nxtn · 2019-12-01T15:51:25Z

See benchmark results in microsoft/clrmd#351

nxtn · 2019-12-01T16:24:47Z

This system call was added in Linux 3.2, which requires at least RHEL 7.

jkotas · 2019-12-01T17:54:23Z

We should drop support for RHEL6 for .NET 5. Opened #423

0xd4d · 2019-12-01T20:38:59Z

Will probably also fix dotnet/diagnostics#548

lpereira

While I like the changes, there are some comments regarding portability and (possible) performance enhancements.

lpereira · 2019-12-02T18:55:53Z

src/coreclr/src/debug/createdump/datatarget.cpp

-    ssize_t read = pread64(m_fd, buffer, size, (off64_t)(ULONG_PTR)address);
+    iovec local{ buffer, size };
+    iovec remote{ (void*)(ULONG_PTR)address, size };
+    ssize_t read = process_vm_readv(m_pid, &local, 1, &remote, 1, 0);


ssize_t is a signed, 64-bit integer; ULONG32 is an unsigned, 32-bit integer. It's unlikely that read will be larger than size as that's the quantity passed as a parameter, however assigning read to *done will at least generate a warning.

(Using 32-bit integers to denote sizes is something we should fix throughout the runtime. It should use size_t throughout.)

Also, are there any places where it would be beneficial to pass an iovec array rather than just 1 element at a time to process_vm_readv()? (There could be a fallback method to go through each one and read the memory on OSes that do not offer this particular syscall or a similar feature.)

It might be a good idea to #ifdef this for Linux only, or check if the syscall exist during build-time. For instance, the FreeBSD procfs supports /proc/<PID>/mem but does not have this syscall, so this breaks createdump on FreeBSD. On systems without process_vm_readv() but with a compatible procfs, you might benefit from using readv() instead of pread() and have similar speedups if the suggestion above is possible.

There may even be Linux distros that don't support process_vm_readv so a build-time check (like all the other HAVE_xxxx defines) should be added falling back to the proc//mem code.

Distro Version Kernel

RHEL 7.0 3.10

CentOS 7.0 3.10

Oracle Linux 7.0 3.10

Fedora 29 4.18

Debian 9 4.9

Ubuntu 16.04 4.4

Linux Mint 18 4.4

openSUSE 15 4.12

SLES 12 SP2 4.4

Alpline Linux 3.8 4.14

There may even be Linux distros that don't support process_vm_readv

All supported Linux distros meet the minimum version requirement, so I assume it only needs to check for Linux.

For instance, the FreeBSD procfs supports /proc/<PID>/mem

The createdump doc says /proc doesn't work on FreeBSD. Can you confirm it? Thanks!

Also, are there any places where it would be beneficial to pass an iovec array rather than just 1 element at a time to process_vm_readv()?

There are some parallel read operations, but they are invoked on ICLRDataTarget instead of DumpDataTarget. It's not easy to extend that interface.

I wrote that createdump doc 2 or 3 years ago and at the time, it didn't look like FreeBSD supported /proc//mem but now I'm not sure.

hoyosjs · 2019-12-04T18:10:06Z

src/coreclr/src/debug/createdump/createdump.h

@@ -45,6 +45,10 @@ typedef int T_CONTEXT;
 #include <sys/user.h>
 #include <sys/wait.h>
 #include <sys/procfs.h>
+#ifdef __linux__
+#include <sys/uio.h>
+#define HAVE_READV


Even though we are sure of such a feature in most modern kernels, why not test for it in

runtime/src/coreclr/src/pal/src/configure.cmake

Line 103 in 875a54a

check_function_exists(sigreturn HAVE_SIGRETURN)

That way we have this consistent, users can build for other platforms with no hard-coded elements, and makes bring-up platforms easier.

hoyosjs · 2019-12-06T20:58:30Z

The changes look fine to me, thanks for the change :) Right now doing thorough testing is a little harder with the migration - we are not producing official drops we can consume in the broader test infrastructure. Once I get around to that I'll test this and let you know how it goes.

Conversations have been resolved.

hoyosjs · 2020-01-28T18:40:35Z

/azp run

azure-pipelines · 2020-01-28T18:40:40Z

You have several pipelines (over 10) configured to build pull requests in this repository. Specify which pipelines you would like to run by using /azp run [pipelines] command. You can specify multiple pipelines using a comma separated list.

hoyosjs · 2020-01-28T18:40:54Z

/azp list

azure-pipelines · 2020-01-28T18:40:59Z

CI/CD Pipelines for this repository: coreclr-gc-longrunning coreclr-gc-simulator runtime-coreclr outerloop runtime-coreclr crossgen2 runtime-libraries outerloop runtime-libraries outerloop-windows runtime-libraries outerloop-linux runtime-libraries outerloop-osx runtime runtime-libraries enterprise-linux runtime-libraries stress-http runtime-libraries stress-ssl runtime-live-build runtime-coreclr crossgen2 outerloop coreclr-release-outerloop-nightly sync-runtime-to-mono

hoyosjs · 2020-01-28T18:41:28Z

/azp run runtime

ghost · 2020-01-28T18:41:48Z

Hello @hoyosjs!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

azure-pipelines · 2020-01-28T18:41:51Z

Azure Pipelines successfully started running 1 pipeline(s).

hoyosjs · 2020-01-28T19:18:32Z

src/coreclr/src/debug/createdump/datatarget.cpp

+    iovec local{ buffer, size };
+    iovec remote{ (void*)(ULONG_PTR)address, size };
+    ssize_t read = process_vm_readv(m_pid, &local, 1, &remote, 1, 0);
+#else
    assert(m_fd != -1);


@mikem8361 Any reason these are not runtime-like asserts?

hoyosjs

Only comment that sustains from @lpereira comments:

Check if size is bigger than IOV_MAX.
Check if read is safely castable. We can't easily use size_t on a COM boundary, but we can check for overflows as needed.

mikem8361 · 2020-01-28T20:49:49Z

Createdump uses the C++ assert macro everywhere so it should be ok. I really tried to keep createdump fairly pure C++ except for a couple of PAL references. @john Salem<mailto:josalem@microsoft.com> (I think) found they didn’t seem to display any output in the runtime when hit.

jkotas added the area-Diagnostics-coreclr label Dec 1, 2019

nxtn force-pushed the createdump branch from aeb16ad to b01ccb0 Compare December 1, 2019 17:38

jkotas mentioned this pull request Dec 1, 2019

Drop support for RHEL6 for .NET 5+ #423

Closed

nxtn force-pushed the createdump branch from b01ccb0 to 31bd1d7 Compare December 2, 2019 09:24

lpereira previously requested changes Dec 2, 2019

View reviewed changes

hoyosjs requested review from mikem8361 and hoyosjs December 2, 2019 19:39

nxtn changed the title ~~createdump can read memory with process_vm_readv~~ createdump can read memory with process_vm_readv on Linux Dec 3, 2019

nxtn force-pushed the createdump branch from 31bd1d7 to 5ce94b6 Compare December 3, 2019 04:08

hoyosjs reviewed Dec 4, 2019

View reviewed changes

mikem8361 approved these changes Dec 5, 2019

View reviewed changes

nxtn added 2 commits December 6, 2019 03:21

Use process_vm_readv when function exists

3abc94d

Cast result to ULONG32

917cc6f

nxtn force-pushed the createdump branch from cc5e73f to 917cc6f Compare December 5, 2019 19:23

hoyosjs added the auto-merge label Jan 28, 2020

hoyosjs self-requested a review January 28, 2020 18:41

hoyosjs reviewed Jan 28, 2020

View reviewed changes

hoyosjs approved these changes Jan 28, 2020

View reviewed changes

hoyosjs merged commit 531feac into dotnet:master Jan 28, 2020

nxtn deleted the createdump branch January 29, 2020 02:51

This was referenced Feb 5, 2020

Actually enable IOV features in createdump #31782

Merged

Remove WSL check in Linux dumper dotnet/diagnostics#801

Closed

ghost locked as resolved and limited conversation to collaborators Dec 11, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

createdump can read memory with process_vm_readv on Linux #420

createdump can read memory with process_vm_readv on Linux #420

nxtn commented Dec 1, 2019

nxtn commented Dec 1, 2019

jkotas commented Dec 1, 2019 •

edited

0xd4d commented Dec 1, 2019

lpereira left a comment

lpereira Dec 2, 2019

lpereira Dec 2, 2019 •

edited

mikem8361 Dec 2, 2019

nxtn Dec 3, 2019

nxtn Dec 3, 2019

nxtn Dec 3, 2019

mikem8361 Dec 3, 2019

hoyosjs Dec 4, 2019

hoyosjs commented Dec 6, 2019

hoyosjs commented Jan 28, 2020

azure-pipelines bot commented Jan 28, 2020

hoyosjs commented Jan 28, 2020

azure-pipelines bot commented Jan 28, 2020

hoyosjs commented Jan 28, 2020

ghost commented Jan 28, 2020

azure-pipelines bot commented Jan 28, 2020

hoyosjs Jan 28, 2020

hoyosjs left a comment

mikem8361 commented Jan 28, 2020 via email

Distro	Version	Kernel
RHEL	7.0	3.10
CentOS	7.0	3.10
Oracle Linux	7.0	3.10
Fedora	29	4.18
Debian	9	4.9
Ubuntu	16.04	4.4
Linux Mint	18	4.4
openSUSE	15	4.12
SLES	12 SP2	4.4
Alpline Linux	3.8	4.14

createdump can read memory with process_vm_readv on Linux #420

createdump can read memory with process_vm_readv on Linux #420

Conversation

nxtn commented Dec 1, 2019

nxtn commented Dec 1, 2019

jkotas commented Dec 1, 2019 • edited

0xd4d commented Dec 1, 2019

lpereira left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lpereira Dec 2, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hoyosjs commented Dec 6, 2019

hoyosjs commented Jan 28, 2020

azure-pipelines bot commented Jan 28, 2020

hoyosjs commented Jan 28, 2020

azure-pipelines bot commented Jan 28, 2020

hoyosjs commented Jan 28, 2020

ghost commented Jan 28, 2020

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

azure-pipelines bot commented Jan 28, 2020

Choose a reason for hiding this comment

hoyosjs left a comment

Choose a reason for hiding this comment

mikem8361 commented Jan 28, 2020 via email

jkotas commented Dec 1, 2019 •

edited

lpereira Dec 2, 2019 •

edited

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.