Annotate mmap ranges using PR_SET_VMA #1236

wks · 2024-11-15T07:20:21Z

We demand that every invocation of mmap within mmtk-core to be accompanied with an "annotation" for the purpose of the mmap. On Linux, we will use PR_SET_VMA_ANON_NAME to set the attribute after mmap so that it can be seen in /proc/pid/maps. This will greatly improve the debugging experience.

We demand that every invocation of `mmap` within mmtk-core to be accompanied with an "annotation" for the purpose of the mmap. On Linux, we will use `PR_SET_VMA` to set the annotation after `mmap` so that it can be seen in `/proc/pid/maps`. This will greatly improve the debugging experience.

k-sareen · 2024-11-16T06:15:12Z

Why not just use the space/metadata spec name instead? Also there's a 80-byte limit for the string (including the NUL byte).

However, for Map32, spaces may interleave at chunk granularity in the shared discontiguous memory range.

Why is this a problem? If a chunk is recycled, then we should just rename it.

wks · 2024-11-16T14:19:53Z

Why not just use the space/metadata spec name instead?

For space, I just use its name.
For side metadata, I use both space name and metadata spec name to distinguish between metadata for different spaces.
And there are mmap invocations for other purposes, too, such as SFTMap.

Also there's a 80-byte limit for the string (including the NUL byte).

Good point. Although I don't think any space name + metadata name would exceed this limit, it's safe to perform the check when formatting.

Why is this a problem? If a chunk is recycled, then we should just rename it.

The number of mmap entries is limited at the OS level. So whenever possible, we should merge adjacent entries. If no chunk in the discontiguous space has name (annotated with PR_SET_VMA_ANON_NAME), the OS may merge adjacent entries; but if adjacent entries have different names, they can't be merged.

k-sareen · 2024-11-16T14:43:12Z

And there are mmap invocations for other purposes, too, such as SFTMap.

Yes that is a good point. Though in my branch I just use an Option<&str> because it's not always clear what name to give to an mmap region such as the side metadata implementation for 32-bits and for panic_if_unmapped.

The number of mmap entries is limited at the OS level. So whenever possible, we should merge adjacent entries. If no chunk in the discontiguous space has name (annotated with PR_SET_VMA_ANON_NAME), the OS may merge adjacent entries; but if adjacent entries have different names, they can't be merged.

I'm not sure this is a real concern? I don't imagine we'll ever be hitting the limit even for 32-bit machines. Even Android always has the annotations enabled. I strongly suggest having these on by default. If we ever hit the limit then we can disable it.

wks · 2024-11-25T12:43:18Z

The number of mmap entries is limited at the OS level. So whenever possible, we should merge adjacent entries. If no chunk in the discontiguous space has name (annotated with PR_SET_VMA_ANON_NAME), the OS may merge adjacent entries; but if adjacent entries have different names, they can't be merged.

I'm not sure this is a real concern? I don't imagine we'll ever be hitting the limit even for 32-bit machines. Even Android always has the annotations enabled. I strongly suggest having these on by default. If we ever hit the limit then we can disable it.

Well, perhaps it's not a problem. I tried a few benchmarks. We do mmap at chunk granularity. This means we don't call mmap very often. It is proportional to the heap size, but even memory-hungry benchmarks like h2 run just fine, with only a few hundred invocations of mmap in the entire execution. And there are less than 20 entries in /proc/pid/mmap The following is captured while running the h2 benchmark on OpenJDK with 1GB heap size. They are the only mmaps created by mmtk-core (besides those created by malloc and pthread_create).

40000000-40400000 rw-p 00000000 00:00 0                                  [anon:mmtk:space:nursery]
40400000-40800000 rw-p 00000000 00:00 0                                  [anon:mmtk:space:los]
40800000-41800000 rw-p 00000000 00:00 0                                  [anon:mmtk:space:nursery]
41800000-41c00000 rw-p 00000000 00:00 0                                  [anon:mmtk:space:los]
41c00000-5f400000 rw-p 00000000 00:00 0                                  [anon:mmtk:space:nursery]
5f400000-6f400000 rw-p 00000000 00:00 0                                  [anon:mmtk:space:immix_mature]
6f400000-8f000000 rw-p 00000000 00:00 0                                  [anon:mmtk:space:nursery]
8f000000-90800000 rw-p 00000000 00:00 0                                  [anon:mmtk:space:immix_mature]
100000000-1001e0000 rw-p 00000000 00:00 0 
1001e0000-140000000 ---p 00000000 00:00 0 
e0005000000-e0005800000 rw-p 00000000 00:00 0                            [anon:mmtk:sidemeta:nursery:VMGlobalLogBitSpec]
e0005800000-e0005c00000 rw-p 00000000 00:00 0                            [anon:mmtk:sidemeta:immix_mature:VMGlobalLogBitSpec]
e0005c00000-e0006400000 rw-p 00000000 00:00 0                            [anon:mmtk:sidemeta:nursery:VMGlobalLogBitSpec]
e0006400000-e0006800000 rw-p 00000000 00:00 0                            [anon:mmtk:sidemeta:immix_mature:VMGlobalLogBitSpec]
4e0800400000-4e0800c00000 rw-p 00000000 00:00 0                          [anon:mmtk:sidemeta:immix_mature:IX_LINE_MARK]
4e8800000000-4e8800400000 rw-p 00000000 00:00 0                          [anon:mmtk:sidemeta:immix_mature:IX_BLOCK_DEFRAG]
4e8900000000-4e8900400000 rw-p 00000000 00:00 0                          [anon:mmtk:sidemeta:immix_mature:IX_BLOCK_MARK]
4e8a00000000-4e8a00400000 rw-p 00000000 00:00 0                          [anon:mmtk:sidemeta:immix_mature:CHUNK_MARK]
4eaa82000000-4eaa82400000 rw-p 00000000 00:00 0                          [anon:mmtk:sidemeta:los:VMLocalLOSMarkNurserySpec]
4eac83000000-4eac84800000 rw-p 00000000 00:00 0                          [anon:mmtk:sidemeta:immix_mature:VMLocalMarkBitSpec]

So I think it's OK to leave it enabled by default. The user can still set MMTK_MMAP_ANNOTATION=false to disable it if it ever becomes problematic.

k-sareen · 2024-11-26T00:34:10Z

I think you need to update some of the tests to use the new annotation API as well.

Re: mmap chunks. Like I mentioned in a meeting, we can improve this further by allocating spaces from different ends of the address space

k-sareen · 2024-11-26T04:47:36Z

Err.. Why is sysinfo broken 🤔

wks · 2024-11-26T05:29:30Z

Err.. Why is sysinfo broken 🤔

There is an upstream bug report: GuillaumeGomez/sysinfo#1392

k-sareen · 2024-11-26T07:15:06Z

Should we pin version of sysinfo/libc until this is resolved then?

wks · 2024-11-26T07:34:59Z

Should we pin version of sysinfo/libc until this is resolved then?

I pinned the version of libc in this PR so that we can test and review this PR.

There is already a PR for the sysinfo crate that will fix this problem.

k-sareen

LGTM except some minor stuff in review.

src/mmtk.rs

src/policy/space.rs

src/util/heap/layout/mmapper.rs

src/util/memory.rs

src/util/metadata/side_metadata/global.rs

src/util/memory.rs

qinsoon · 2024-11-27T00:37:50Z

src/util/memory.rs

+/// FIXME: Since it is set via `Options`, it is in theory a decision per MMTk instance. However, we
+/// currently don't have a good design for multiple MMTk instances, so we use static variable for
+/// now.
+pub(crate) static MMAP_ANNOTATION: AtomicBool = AtomicBool::new(true);


This looks bad. Though there is a FIXME above, we should avoid introducing this at first place.

Both Plan and Space have references to the options. You can let plan/space to check options and create a MmapAnnotation. If the runtime option is not enabled, we can use Option<MmapAnnotation> or MmapAnnotation::Omit/None.

Yes. That looks bad.

Although it is possible to let plans and spaces check options, and optionally pass MmapAnnotation::Omit or a None for Option<MmapAnnotation>, it is not ideal.

One important part of my intention is that the annotation cannot be omitted. I'd like the function signatures to enforce that an annotation must be provided for every call to mmap. If we give the caller the option to pass None or Omit, developers will do it if they think it is not worth annotating. Instead, the decision of whether to actually annotate the mmap should be postponed until mmap is actually called and is successful, and should be controlled by the user via an option rather than at call sites.

And checking at every call site is repetitive and error prone. Developers may forget to do the check or erroneously omit the annotation.

Ideally, functions related to memory mapping should be wrapped into a struct, maybe named MmapSupport or CommonMapper. It will be created per MMTK instance, like Mmapper and VMMap (which are currently both global singletons), and referenced by plans, spaces, SFT, etc., whatever needs to use mmap. The mmap_annotation: bool should be an instance variable of CommonMapper, and map_fixed should be an instance method of MmapSupport. That may involve much refactoring, including moving dzmmap, dzmmap_noreplace and mmap_noreserve into MmapSupport. For now, creating a global boolean variable seems to be the easiest solution.

In the short term, we may introduce MmapSupport as a global singleton, like Mmapper and VMMap. In the long term, we can make them specific to the MMTK instance.

One important part of my intention is that the annotation cannot be omitted. I'd like the function signatures to enforce that an annotation must be provided for every call to mmap.

We could enforce the users to supply MmapAnnotation, and turn it into Option<MmapAnnotation> before we send it to mmap functions. Plus the problem of having a global var outweighs the benefits of 'enforcing annotations'.

Alternatively, we can have an instance for memory which holds the boolean value from the option. I think they are both worth trying before adopting the current implementation. We have arg structs for creating plans and spaces. Creating a Memory instance and letting plan/space to hold it shouldn't be too hard.

mmtk-core/src/plan/global.rs

Line 372 in 8640ab8

pub struct CreateGeneralPlanArgs<'a, VM: VMBinding> {

It will be created per MMTK instance, like Mmapper and VMMap (which are currently both global singletons), and referenced by plans, spaces, SFT, etc., whatever needs to use mmap.

I think it is still arguable whether Mmapper and VMMap should be global or per instance. We do have a hacky vm_layout() that should be per instance, and is global at the moment. But we should avoid introducing more globals like this. The more we have, the more difficult and the more resistance we will face when we try to support multiple instances.

Alternatively, we can have an instance for memory which holds the boolean value from the option. I think they are both worth trying before adopting the current implementation. We have arg structs for creating plans and spaces. Creating a Memory instance and letting plan/space to hold it shouldn't be too hard.

mmtk-core/src/plan/global.rs

Line 372 in 8640ab8

pub struct CreateGeneralPlanArgs<'a, VM: VMBinding> {

Yes. This is my intended way to do it. But this may involve too many changes and I think it is better to do the refactoring separately.

The introduced instance (let's call it MmapSupport for now) should contain dzmap, dzmap_noreplace, mmap_noreserve and mmap_fixed. I grepped the code and found that the following places will be affected.

MapState: MapState::transition_to_* will call those functions directly, as if the MapState can transition its own state without the help of Mmapper or the MmapSupport instance. This needs to be refactored so that MapState will become a state holder, and Mmapper implementations (FragmentedMapper and ByteMapMmapper) can call the mmap functions in MmapSupport to do the transition.

LockFreeImmortalSpace: It skipped the Mmapper and directly calls dzmmap_noreplace. I think this is a bug because Mmapper should be aware of the mapping of spaces.

RawMemoryFreeList: It calls dzmmap_noreplace to map its memory. Not sure if it should go through Mmapper, but probably it should because a RawMemoryFreeList is part of a space using it.

In the end, we may have all mmap invocations going through Mmapper, and an Mmapper holds a reference to MmapSupport.

We could enforce the users to supply MmapAnnotation, and turn it into Option<MmapAnnotation> before we send it to mmap functions.

It is good to enforce the users to supply MmapAnnotation. The difficulty is drawing a border between the "user" and the "mmap functions". And another difficulty is that we lost access to Options before reaching "mmap functions".

Take the call chain SideMetadataContext::map_metadata_internal -> try_mmap_contiguous_metadata_space -> MMAPPER.ensure_mapped for example.

In SideMetadataContext::map_metadata_internal, we have enough information (space name and side metadata name) to construct the annotation. We construct MmapAnnotation here. But since SideMetadataContext does not reference anySpace or Plan, it cannot access Options.

try_mmap_contiguous_metadata_space is a top-level function. It has access to neither space name nor Option.

MMAPER.ensure_mapped is a function that is more related to mmap. It may be refactored to hold a boolean variable MMAP_ANNOTATION, or refer to MmapSupport I mentioned above.

So the availability of Options or the MMAP_ANNOTATION variable does not match the distinction between user and mmap. I think it's easier to refactor the code and just let "mmap functions" decide whether to annotate.

Plus the problem of having a global var outweighs the benefits of 'enforcing annotations'.

Having global var is bad, but I think it is worse to let the users check Options::mmap_annotation before constructing the Option<MmapAnnotation> because it is repetitive and error-prone. Having a global variable is the easiest workaround before we refactor Mmapper, MapState, etc.

If you think having a global variable is unacceptible, I can remove Options::mmap_annotation and make the annotation controlled by a Cargo feature instead, or leave it always on. I think it is OK to leave it always on because there will be strictly fewer prctl than mmap, and we don't call mmap very often, either.

If you think having a global variable is unacceptible, I can remove Options::mmap_annotation and make the annotation controlled by a Cargo feature instead, or leave it always on.

Right. We dont really need the option anyway. We can always annotate the mmaps.

The reason that I am against having a global var is that we know it's wrong and this kind of small issues will accumulate over time and eventually make it hard to get rid of them. We should try to avoid introducing them in first place.

I removed the option, and added a Cargo feature no_mmap_annotation just in case anyone needs to disable it for any reason.

This global var can be removed now. I am suprised that we did not get any warning from the compiler for this, this seems to be an unused variable.

This reverts commit 491a196.

qinsoon

Generally looks good. The global var MMAP_ANNOTATION can be removed.

src/policy/lockfreeimmortalspace.rs

qinsoon · 2024-11-28T22:16:58Z

src/policy/marksweepspace/malloc_ms/metadata.rs

-        "failed to mmap meta memory"
-    );
+    CHUNK_METADATA
+        .try_map_metadata_space(start, size, space_name)


This is good refactoring with clear improvement. The old code put the actual action performed in the assert! which is bad.

qinsoon · 2024-11-28T22:19:59Z

src/util/memory.rs

+/// FIXME: Since it is set via `Options`, it is in theory a decision per MMTk instance. However, we
+/// currently don't have a good design for multiple MMTk instances, so we use static variable for
+/// now.
+pub(crate) static MMAP_ANNOTATION: AtomicBool = AtomicBool::new(true);


This global var can be removed now. I am suprised that we did not get any warning from the compiler for this, this seems to be an unused variable.

qinsoon

Looks good

wks · 2024-11-29T07:14:37Z

V8 failed for HTTP 404. I think it might be a transient failure if the Ubuntu server is updating itself and happens to be in an inconsistent state when we ran the test. See: https://github.com/mmtk/mmtk-core/actions/runs/12078239098/job/33686682978?pr=1236

JikesRVM failed for the classic "instruction ... not in RVM space" error. Interestingly, the dumped maps file showed that our annotation is working. See: https://github.com/mmtk/mmtk-core/actions/runs/12078239098/job/33686683158?pr=1236

I'll rerun those tests.

#1242 fixed most similar issues in the repo, but #1236 introduced `MmapAnnotation` and introduced a new warning.

wks added 9 commits November 25, 2024 14:39

Guard the annotation behind an option.

a453e32

Make annotation linux-only

2972474

Update comments

a418700

Fix helpers_32

94cd260

Fix vmspace

3dfad91

Fix space names

12ce6ae

Fix error handling

851444c

Update comment

b196709

Rename MmapAnno to MmapAnnotation

de848c4

wks marked this pull request as ready for review November 25, 2024 12:20

wks added the PR-extended-testing Run extended tests for the pull request label Nov 25, 2024

wks added 2 commits November 26, 2024 12:11

Fix mock tests.

4a5e256

Fix documentation

5344a0b

Workaround sysinfo build on MacOS

491a196

k-sareen reviewed Nov 26, 2024

View reviewed changes

qinsoon reviewed Nov 27, 2024

View reviewed changes

wks added 4 commits November 27, 2024 09:39

Revert "Workaround sysinfo build on MacOS"

ff31481

This reverts commit 491a196.

Use Space::get_name consistently

4089d83

Include android, too.

bbfdec0

Use Relaxed order

9d860d3

wks added 6 commits November 27, 2024 09:50

Update the doc for map_metadata_internal

18f3f63

Comment the "all" metadata name.

c5bc4f9

Doc for strategy and anno

e1071af

Control annotation with Cargo feature instead.

13c10b5

A missing doc comment

df35f42

API migration guide

b1e43d5

wks mentioned this pull request Nov 28, 2024

Refactor: Should we do all mmap via Mmapper? #1239

Open

qinsoon reviewed Nov 28, 2024

View reviewed changes

qinsoon mentioned this pull request Nov 29, 2024

Add misc debugging helpers and feature to poison dead memory #1241

Open

Remove unused static variable

b3d8672

qinsoon approved these changes Nov 29, 2024

View reviewed changes

wks added this pull request to the merge queue Nov 29, 2024

Merged via the queue into mmtk:master with commit cd2fe83 Nov 29, 2024
29 of 34 checks passed

wks deleted the feature/mmap-anno branch November 29, 2024 10:18

qinsoon mentioned this pull request Dec 1, 2024

Fix warnings for lifetime in MmapAnnotation impl #1244

Merged

github-merge-queue bot pushed a commit that referenced this pull request Dec 2, 2024

Fix warnings for lifetime in MmapAnnotation impl (#1244)

3d7bc11

#1242 fixed most similar issues in the repo, but #1236 introduced `MmapAnnotation` and introduced a new warning.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Annotate mmap ranges using PR_SET_VMA #1236

Annotate mmap ranges using PR_SET_VMA #1236

wks commented Nov 15, 2024 •

edited

Loading

k-sareen commented Nov 16, 2024 •

edited

Loading

wks commented Nov 16, 2024

k-sareen commented Nov 16, 2024 •

edited

Loading

wks commented Nov 25, 2024

k-sareen commented Nov 26, 2024

k-sareen commented Nov 26, 2024

wks commented Nov 26, 2024

k-sareen commented Nov 26, 2024

wks commented Nov 26, 2024

k-sareen left a comment

qinsoon Nov 27, 2024

wks Nov 27, 2024

qinsoon Nov 27, 2024

wks Nov 27, 2024

qinsoon Nov 27, 2024

wks Nov 27, 2024

qinsoon Nov 28, 2024

qinsoon left a comment

qinsoon Nov 28, 2024

qinsoon Nov 28, 2024

qinsoon left a comment

wks commented Nov 29, 2024

Annotate mmap ranges using PR_SET_VMA #1236

Annotate mmap ranges using PR_SET_VMA #1236

Conversation

wks commented Nov 15, 2024 • edited Loading

k-sareen commented Nov 16, 2024 • edited Loading

wks commented Nov 16, 2024

k-sareen commented Nov 16, 2024 • edited Loading

wks commented Nov 25, 2024

k-sareen commented Nov 26, 2024

k-sareen commented Nov 26, 2024

wks commented Nov 26, 2024

k-sareen commented Nov 26, 2024

wks commented Nov 26, 2024

k-sareen left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qinsoon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qinsoon left a comment

Choose a reason for hiding this comment

wks commented Nov 29, 2024

wks commented Nov 15, 2024 •

edited

Loading

k-sareen commented Nov 16, 2024 •

edited

Loading

k-sareen commented Nov 16, 2024 •

edited

Loading