8297389: resexhausted003 fails with assert(!thread->owns_locks()) failed: must release all locks when leaving VM #11316

TobiHartmann · 2022-11-23T11:56:45Z

Method::build_profiling_method_data acquires the MethodData_lock when initializing Method::_method_data to prevent multiple allocations by different threads. The problem is that when metaspace allocation fails and JvmtiExport::should_post_resource_exhausted() is set, we assert during the ThreadToNativeFromVM transition in JVMTI code.

Since concurrent initialization is a rare event, I suggest to get rid of the lock and perform the initialization with a cmpxchg, similar to how method counters are initialized:

jdk/src/hotspot/share/oops/method.cpp

Lines 644 to 646 in f4b5065

    
           if (!mh->init_method_counters(counters)) { 
        
             MetadataFactory::free_metadata(mh->method_holder()->class_loader_data(), counters); 
        
           }

Since current code in Method::set_method_data uses a Atomic::release_store, I added a OrderAccess::release().

Thanks,
Tobias

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8297389: resexhausted003 fails with assert(!thread->owns_locks()) failed: must release all locks when leaving VM

Reviewers

David Holmes (@dholmes-ora - Reviewer)
Richard Reingruber (@reinrich - Reviewer) ⚠️ Review applies to 7e94c2b2
Dean Long (@dean-long - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/11316/head:pull/11316
$ git checkout pull/11316

Update a local copy of the PR:
$ git checkout pull/11316
$ git pull https://git.openjdk.org/jdk pull/11316/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 11316

View PR using the GUI difftool:
$ git pr show -t 11316

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/11316.diff

…iled: must release all locks when leaving VM

TobiHartmann · 2022-11-23T11:56:52Z

/label hotspot

bridgekeeper · 2022-11-23T11:57:36Z

👋 Welcome back thartmann! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2022-11-23T12:00:27Z

@TobiHartmann
The hotspot label was successfully added.

mlbridge · 2022-11-23T12:03:37Z

Webrevs

dholmes-ora

If MethodData::allocate cannot be called whilst holding a lock then perhaps we can assert that in there?

I think there is a broader problem here that metaspace allocation can occur in numerous places and any failure could post the resource-exhausted event and potentially lead to problems like this.

This fix side-steps one problematic call-site, but going lock-free has its own concerns.

dholmes-ora · 2022-11-23T21:32:02Z

src/hotspot/share/oops/method.cpp

+  // The store into method must be released. On platforms without
+  // total store order (TSO) the reference may become visible before
+  // the initialization of data otherwise.
+  OrderAccess::release();


A release here is not necessary as Atomic::replace_if_null will by default have memory_order_conservative which maintains the full bi-directional fence of the internal cmpxchg.

dholmes-ora · 2022-11-23T21:36:55Z

src/hotspot/share/oops/method.cpp

-      return;   // return the exception (which is cleared)
-    }
+  ClassLoaderData* loader_data = method->method_holder()->class_loader_data();
+  MethodData* method_data = MethodData::allocate(loader_data, method, THREAD);


So the downside of lock-free here is that we have to pre-allocate and then later free. How expensive is that? How likely are we to get multiple threads attempting this at the same time? We might trigger a resource-exhausted event unnecessarily due to the temporary use of metaspace.

TobiHartmann · 2022-11-28T12:06:03Z

Thanks for the review, David!

I added an assert to MethodData::allocate and fixed the ciReplay code which only ever gets executed by a single thread by removing the lock. I also removed the OrderAccess::release().

Regarding the overhead of going lock-free: I temporarily added code that counts the number of times that multiple threads attempt initialization and asserts if it's more that 50x. This triggers in 25 out of 203.772 runs (tests from tier1 to tier3). The average size of these allocations is around 88 words (704 bytes). I therefore think it's fine to avoid a lock in this case. Also, we already use a lock-free mechanism for method::build_method_counters.

What do you think?

reinrich

Thanks for taking care of the issue. The fix looks ok to me. Seems like you've removed all uses of MethodData_lock so you could remove the declaration and definition too.

Thanks, Richard.

openjdk · 2022-11-28T14:17:35Z

@TobiHartmann This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8297389: resexhausted003 fails with assert(!thread->owns_locks()) failed: must release all locks when leaving VM

Reviewed-by: dholmes, rrich, dlong

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 151 new commits pushed to the master branch:

34fcd83: 8297821: jdk/jshell/Test8294583.java fails on some platforms
ce048e7: 8295006: Colored text is not shown on disabled checkbox and radio button with GTK LAF for bug4314194.
756dd5b: 8294942: Compiler implementation for Record Patterns (Second Preview)
c612512: 8296660: Swing HTML table with omitted closing tags misparsed
56048f9: 8297852: Backout 8252584 for the time being
f49acd5: 8297697: RISC-V: Add support for SATP mode detection
3b513a4: 8297802: display of @SPEC tags should mimic that of @see tags
4485d4e: 8297728: Cache invocation type rather than invoker in NamedFunction
e9d501e: 8297519: Improve expressions and modernise code in PKCS
1722f9c: 8290231: java/foreign/malloc/TestMixedMallocFree.java crashed in JDK19 ATR run
... and 141 more: https://git.openjdk.org/jdk/compare/57f5cfdeb52b160e58968fb177b4432b3e079607...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

TobiHartmann · 2022-11-28T14:28:44Z

Thanks for the review, Richard. Good catch, I removed the MethodData_lock.

dean-long · 2022-11-29T01:01:02Z

I added an assert to MethodData::allocate and fixed the ciReplay code which only ever gets executed by a single thread by removing the lock. I also removed the OrderAccess::release().

I'm not sure if it's guaranteed that replay runs single-threaded. I haven't looked into the details, but I think ReplayInline can run in any compiler thread after startup, and then of course there is JDK-8254110.

dholmes-ora · 2022-11-29T04:08:54Z

I therefore think it's fine to avoid a lock in this case. Also, we already use a lock-free mechanism for Method::build_method_counters.

Method::build_method_counters doesn't appear to be lock-free, in the sense there are no atomic operations; rather concurrency just doesn't seem to be a concern with that code. ??

But as long as the overhead is low and interference unlikely, then we can see how this works out in practice. Thanks.

TobiHartmann · 2022-11-29T07:34:08Z

I'm not sure if it's guaranteed that replay runs single-threaded. I haven't looked into the details, but I think ReplayInline can run in any compiler thread after startup, and then of course there is JDK-8254110.

But ReplayInline does not load ciMethodData, i.e., does not call process_ciMethodData, right?

The only way this code can get executed is through JNI_CreateJavaVM_inner -> ciReplay::replay -> ciReplay::replay_impl -> process-> process_command -> process_ciMethodData and that only ever happens single-threaded.

Of course, if we are ever going to implement JDK-8254110, we need to revisit that code, but in that case the assert that I added will trigger.

What do you think?

Method::build_method_counters doesn't appear to be lock-free, in the sense there are no atomic operations; rather concurrency just doesn't seem to be a concern with that code. ??

It uses the exact same mechanism that I now added to Method::build_profiling_method_data, including atomic operations to initialize _method_counters:

jdk/src/hotspot/share/oops/method.cpp

Lines 651 to 654 in f4b5065

    
           bool Method::init_method_counters(MethodCounters* counters) { 
        
             // Try to install a pointer to MethodCounters, return true on success. 
        
             return Atomic::replace_if_null(&_method_counters, counters); 
        
           }

And I verified that multiple threads attempt to initialize the counters by adding verification code to:

jdk/src/hotspot/share/oops/method.cpp

Lines 644 to 646 in f4b5065

    
           if (!mh->init_method_counters(counters)) { 
        
             MetadataFactory::free_metadata(mh->method_holder()->class_loader_data(), counters); 
        
           }

dholmes-ora · 2022-11-30T06:28:08Z

It uses the exact same mechanism that I now added to Method::build_profiling_method_data, including atomic operations to initialize _method_counters

Sorry I somehow missed the init_method_counters logic. :(

dean-long · 2022-11-30T08:54:04Z

Of course, if we are ever going to implement JDK-8254110, we need to revisit that code, but in that case the assert that I added will trigger.

What do you think?

OK, please add a TODO in JDK-8254110 explaining this so it doesn't get lost.

TobiHartmann · 2022-11-30T10:09:25Z

OK, please add a TODO in JDK-8254110 explaining this so it doesn't get lost.

Thanks, done.

TobiHartmann · 2022-11-30T13:48:41Z

@dholmes-ora, @dean-long are you okay with the latest version?

dholmes-ora

Changes seem okay but I'm not that familiar with the actual code here.

Thanks.

TobiHartmann · 2022-12-01T06:10:10Z

Thanks, David!

TobiHartmann · 2022-12-01T08:48:40Z

Thanks, Dean!

TobiHartmann · 2022-12-01T08:48:46Z

/integrate

openjdk · 2022-12-01T08:50:03Z

Going to push as commit 9f24a6f.
Since your change was applied there have been 152 commits pushed to the master branch:

fa0c599: 8297750: Unnecessary Vector usage in IIORegistry
34fcd83: 8297821: jdk/jshell/Test8294583.java fails on some platforms
ce048e7: 8295006: Colored text is not shown on disabled checkbox and radio button with GTK LAF for bug4314194.
756dd5b: 8294942: Compiler implementation for Record Patterns (Second Preview)
c612512: 8296660: Swing HTML table with omitted closing tags misparsed
56048f9: 8297852: Backout 8252584 for the time being
f49acd5: 8297697: RISC-V: Add support for SATP mode detection
3b513a4: 8297802: display of @SPEC tags should mimic that of @see tags
4485d4e: 8297728: Cache invocation type rather than invoker in NamedFunction
e9d501e: 8297519: Improve expressions and modernise code in PKCS
... and 142 more: https://git.openjdk.org/jdk/compare/57f5cfdeb52b160e58968fb177b4432b3e079607...master

Your commit was automatically rebased without conflicts.

openjdk · 2022-12-01T08:50:24Z

@TobiHartmann Pushed as commit 9f24a6f.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

8297389: resexhausted003 fails with assert(\!thread->owns_locks()) fa…

8c23446

…iled: must release all locks when leaving VM

openjdk bot added rfr Pull request is ready for review hotspot hotspot-dev@openjdk.org labels Nov 23, 2022

dholmes-ora suggested changes Nov 23, 2022

View reviewed changes

Removed OrderAccess::release, added assert and adjusted ciReplay code

7e94c2b

reinrich approved these changes Nov 28, 2022

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Nov 28, 2022

Completely removed MethodData_lock

4b81ea4

dholmes-ora approved these changes Dec 1, 2022

View reviewed changes

dean-long approved these changes Dec 1, 2022

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Dec 1, 2022

openjdk bot closed this Dec 1, 2022

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Dec 1, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8297389: resexhausted003 fails with assert(!thread->owns_locks()) failed: must release all locks when leaving VM #11316

8297389: resexhausted003 fails with assert(!thread->owns_locks()) failed: must release all locks when leaving VM #11316

TobiHartmann commented Nov 23, 2022 •

edited by openjdk bot

Loading

TobiHartmann commented Nov 23, 2022

bridgekeeper bot commented Nov 23, 2022

openjdk bot commented Nov 23, 2022

mlbridge bot commented Nov 23, 2022 •

edited

Loading

dholmes-ora left a comment

dholmes-ora Nov 23, 2022

dholmes-ora Nov 23, 2022

TobiHartmann commented Nov 28, 2022

reinrich left a comment

openjdk bot commented Nov 28, 2022 •

edited

Loading

TobiHartmann commented Nov 28, 2022

dean-long commented Nov 29, 2022

dholmes-ora commented Nov 29, 2022

TobiHartmann commented Nov 29, 2022

dholmes-ora commented Nov 30, 2022

dean-long commented Nov 30, 2022

TobiHartmann commented Nov 30, 2022

TobiHartmann commented Nov 30, 2022

dholmes-ora left a comment

TobiHartmann commented Dec 1, 2022

TobiHartmann commented Dec 1, 2022

TobiHartmann commented Dec 1, 2022

openjdk bot commented Dec 1, 2022

openjdk bot commented Dec 1, 2022

	if (!mh->init_method_counters(counters)) {
	MetadataFactory::free_metadata(mh->method_holder()->class_loader_data(), counters);
	}

8297389: resexhausted003 fails with assert(!thread->owns_locks()) failed: must release all locks when leaving VM #11316

8297389: resexhausted003 fails with assert(!thread->owns_locks()) failed: must release all locks when leaving VM #11316

Conversation

TobiHartmann commented Nov 23, 2022 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

TobiHartmann commented Nov 23, 2022

bridgekeeper bot commented Nov 23, 2022

openjdk bot commented Nov 23, 2022

mlbridge bot commented Nov 23, 2022 • edited Loading

Webrevs

dholmes-ora left a comment

Choose a reason for hiding this comment

dholmes-ora Nov 23, 2022

Choose a reason for hiding this comment

dholmes-ora Nov 23, 2022

Choose a reason for hiding this comment

TobiHartmann commented Nov 28, 2022

reinrich left a comment

Choose a reason for hiding this comment

openjdk bot commented Nov 28, 2022 • edited Loading

TobiHartmann commented Nov 28, 2022

dean-long commented Nov 29, 2022

dholmes-ora commented Nov 29, 2022

TobiHartmann commented Nov 29, 2022

dholmes-ora commented Nov 30, 2022

dean-long commented Nov 30, 2022

TobiHartmann commented Nov 30, 2022

TobiHartmann commented Nov 30, 2022

dholmes-ora left a comment

Choose a reason for hiding this comment

TobiHartmann commented Dec 1, 2022

TobiHartmann commented Dec 1, 2022

TobiHartmann commented Dec 1, 2022

openjdk bot commented Dec 1, 2022

openjdk bot commented Dec 1, 2022

TobiHartmann commented Nov 23, 2022 •

edited by openjdk bot

Loading

mlbridge bot commented Nov 23, 2022 •

edited

Loading

openjdk bot commented Nov 28, 2022 •

edited

Loading