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

PageAccounting::clear_reserved() called after reset() in GenImmix #734

Closed
wks opened this issue Jan 10, 2023 · 3 comments · Fixed by #755
Closed

PageAccounting::clear_reserved() called after reset() in GenImmix #734

wks opened this issue Jan 10, 2023 · 3 comments · Fixed by #755
Labels
A-heap Area: Heap (including Mmapper, VMMap) C-bug Category: Bug

Comments

@wks
Copy link
Collaborator

wks commented Jan 10, 2023

GenImmix is very likely to crash. I can reproduce the crash using the following command:

RUST_BACKTRACE=1 MMTK_PLAN=GenImmix /path/to/openjdk/build/linux-x86_64-normal-server-slowdebug/jdk/bin/java -XX:+UseThirdPartyHeap -Xm{s,x}50M -jar dacapo-9.12-MR1-bach.jar lusearch -t 1

It may crash with both debug and release build alike.

What happened is, the counter PageAccounting::reserved underflowed (attempted to subtract from 0).

At the end of a GC, PageAccounting::reset() is called because CopySpace::release() calls self.pr.reset(). reserved is cleared to 0.

Then GC finishes. A mutator wakes up, and calls pr.clear_request(pages_reserved) with a non-zero page_reserved. This will call PageAccounting::clear_reserved(pages) with a non-zero argument, and it will call self.reserved.fetch_sub when the current value of reserved is zero.

The pr.clear_request statement is in src/policy/space.rs line 72:

// src/policy/space.rs:66
        trace!("Polling ..");

        if should_poll && self.get_gc_trigger().poll(false, Some(self.as_space())) {
            debug!("Collection required");
            assert!(allow_gc, "GC is not allowed here: collection is not initialized (did you call initialize_collection()?).");
            VM::VMCollection::block_for_gc(VMMutatorThread(tls)); // We have checked that this is mutator
            pr.clear_request(pages_reserved); // clear the pages after GC. We need those reserved pages so we can compute new heap size properly.
            unsafe { Address::zero() }
        } else {
            debug!("Collection not required");

A stack trace when this happened is shown below. Note that the "attempt to subtract with overflow" happens because I added a logging statement:

 // src/util/heap/accounting.rs
     pub fn clear_reserved(&self, pages: usize) {
         let result = self.reserved.fetch_sub(pages, Ordering::Relaxed); // `fetch_sub` never crashes when overflowing.
+        eprintln!("[clear_reserved] {} -> {}", result, result - pages);  // `result-pages` will overflow in debug mode.
     }

The stack trace:

thread '<unnamed>' panicked at 'attempt to subtract with overflow', /home/wks/projects/mmtk-github/mmtk-core/src/util/heap/accounting.rs:42:56
stack backtrace:
   0: rust_begin_unwind
             at /rustc/9d1b2106e23b1abd32fce1f17267604a5102f57a/library/std/src/panicking.rs:498:5
   1: core::panicking::panic_fmt
             at /rustc/9d1b2106e23b1abd32fce1f17267604a5102f57a/library/core/src/panicking.rs:116:14
   2: core::panicking::panic
             at /rustc/9d1b2106e23b1abd32fce1f17267604a5102f57a/library/core/src/panicking.rs:48:5
   3: mmtk::util::heap::accounting::PageAccounting::clear_reserved
             at /home/wks/projects/mmtk-github/mmtk-core/src/util/heap/accounting.rs:42:56
   4: mmtk::util::heap::pageresource::PageResource::clear_request
             at /home/wks/projects/mmtk-github/mmtk-core/src/util/heap/pageresource.rs:35:9
   5: mmtk::policy::space::Space::acquire
             at /home/wks/projects/mmtk-github/mmtk-core/src/policy/space.rs:72:13
   6: mmtk::util::alloc::bumpallocator::BumpAllocator<VM>::acquire_block
             at /home/wks/projects/mmtk-github/mmtk-core/src/util/alloc/bumpallocator.rs:163:30
   7: <mmtk::util::alloc::bumpallocator::BumpAllocator<VM> as mmtk::util::alloc::allocator::Allocator<VM>>::alloc_slow_once
             at /home/wks/projects/mmtk-github/mmtk-core/src/util/alloc/bumpallocator.rs:88:9
   8: mmtk::util::alloc::allocator::Allocator::alloc_slow_inline
             at /home/wks/projects/mmtk-github/mmtk-core/src/util/alloc/allocator.rs:228:17
   9: mmtk::util::alloc::allocator::Allocator::alloc_slow
             at /home/wks/projects/mmtk-github/mmtk-core/src/util/alloc/allocator.rs:182:9
  10: <mmtk::util::alloc::bumpallocator::BumpAllocator<VM> as mmtk::util::alloc::allocator::Allocator<VM>>::alloc
             at /home/wks/projects/mmtk-github/mmtk-core/src/util/alloc/bumpallocator.rs:71:13
  11: <mmtk::plan::mutator_context::Mutator<VM> as mmtk::plan::mutator_context::MutatorContext<VM>>::alloc
             at /home/wks/projects/mmtk-github/mmtk-core/src/plan/mutator_context.rs:92:9
  12: mmtk::memory_manager::alloc
             at /home/wks/projects/mmtk-github/mmtk-core/src/memory_manager.rs:158:5
  13: alloc
             at /home/wks/projects/mmtk-github/mmtk-openjdk/mmtk/src/api.rs:135:5
  14: _ZN18MMTkMutatorContext5allocEm9Allocator
             at /home/wks/projects/mmtk-github/openjdk/../mmtk-openjdk/openjdk/mmtkMutator.cpp:29:36
  15: _ZN8MMTkHeap12mem_allocateEmPb
             at /home/wks/projects/mmtk-github/openjdk/../mmtk-openjdk/openjdk/mmtkHeap.cpp:453:68
  16: _ZNK12MemAllocator21allocate_outside_tlabERNS_10AllocationE
             at /home/wks/projects/mmtk-github/openjdk/src/hotspot/share/gc/shared/memAllocator.cpp:272:38
  17: _ZNK12MemAllocator12mem_allocateERNS_10AllocationE
             at /home/wks/projects/mmtk-github/openjdk/src/hotspot/share/gc/shared/memAllocator.cpp:370:31
  18: _ZNK12MemAllocator8allocateEv
             at /home/wks/projects/mmtk-github/openjdk/src/hotspot/share/gc/shared/memAllocator.cpp:377:33
  19: _ZN13CollectedHeap14array_allocateEP5KlassiibP6Thread
             at /home/wks/projects/mmtk-github/openjdk/src/hotspot/share/gc/shared/collectedHeap.cpp:457:28
  20: _ZN14TypeArrayKlass15allocate_commonEibP6Thread
             at /home/wks/projects/mmtk-github/openjdk/src/hotspot/share/oops/typeArrayKlass.cpp:106:60
  21: _ZN14TypeArrayKlass8allocateEiP6Thread
             at /home/wks/projects/mmtk-github/openjdk/src/hotspot/share/oops/typeArrayKlass.hpp:71:68
  22: _ZN10oopFactory13new_typeArrayE9BasicTypeiP6Thread
             at /home/wks/projects/mmtk-github/openjdk/src/hotspot/share/memory/oopFactory.cpp:58:52
  23: _ZN11OptoRuntime11new_array_CEP5KlassiP10JavaThread
             at /home/wks/projects/mmtk-github/openjdk/src/hotspot/share/opto/runtime.cpp:248:39
  24: <unknown>
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
fatal runtime error: failed to initiate panic, error 5
^Cfish: Job 1, 'RUST_BACKTRACE=1 MMTK_PLAN=GenI…' terminated by signal SIGABRT (Abort)
@qinsoon qinsoon added C-bug Category: Bug A-heap Area: Heap (including Mmapper, VMMap) labels Jan 11, 2023
@qinsoon
Copy link
Member

qinsoon commented Jan 11, 2023

This is introduced in #712. Maybe we can have a separate field in PageAccounting for pending allocation, and that does not get cleared during GC.

@qinsoon
Copy link
Member

qinsoon commented Feb 6, 2023

@k-sareen You said that GenImmix crashes when running fop. Is the crash related with this issue?

@k-sareen
Copy link
Collaborator

k-sareen commented Feb 6, 2023

No I don't think so. Think it was something in the binding. But I didn't run in debug build anyway. Let me see if I can reproduce it.

qinsoon added a commit that referenced this issue Feb 9, 2023
This PR reverts a change about where to call `pr.clear_request()` in
`Space.acquire()` in #712. The
change caused issues like #734.
This PR reverts the change. We now clear the reserved pages before GC
(the same as before, and the same as Java MMTk), and additionally we
inform the GC trigger about pending allocation pages. This closes
#734.
qinsoon added a commit to qinsoon/mmtk-core that referenced this issue Feb 21, 2023
This PR reverts a change about where to call `pr.clear_request()` in
`Space.acquire()` in mmtk#712. The
change caused issues like mmtk#734.
This PR reverts the change. We now clear the reserved pages before GC
(the same as before, and the same as Java MMTk), and additionally we
inform the GC trigger about pending allocation pages. This closes
mmtk#734.
wenyuzhao pushed a commit to wenyuzhao/mmtk-core that referenced this issue Mar 20, 2023
This PR reverts a change about where to call `pr.clear_request()` in
`Space.acquire()` in mmtk#712. The
change caused issues like mmtk#734.
This PR reverts the change. We now clear the reserved pages before GC
(the same as before, and the same as Java MMTk), and additionally we
inform the GC trigger about pending allocation pages. This closes
mmtk#734.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-heap Area: Heap (including Mmapper, VMMap) C-bug Category: Bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants