From d207ed3f7cb810e3c0c8a8cd4d9aaa65164c6d16 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20Gr=C3=B6nlund?= Date: Mon, 17 Mar 2025 10:47:18 +0000 Subject: [PATCH] 8352066: JVM.commit() and JVM.flush() exhibit race conditions against JFR epochs Reviewed-by: egahlin --- src/hotspot/share/jfr/jni/jfrJniMethod.cpp | 12 ++++---- .../checkpoint/jfrCheckpointManager.hpp | 2 +- .../recorder/service/jfrRecorderService.cpp | 5 ++-- .../stacktrace/jfrStackTraceRepository.cpp | 6 +--- .../stacktrace/jfrStackTraceRepository.hpp | 3 +- .../share/jfr/support/jfrIntrinsics.cpp | 30 +++---------------- .../share/jfr/writers/jfrJavaEventWriter.cpp | 19 +++++------- .../share/jfr/writers/jfrJavaEventWriter.hpp | 2 +- 8 files changed, 23 insertions(+), 56 deletions(-) diff --git a/src/hotspot/share/jfr/jni/jfrJniMethod.cpp b/src/hotspot/share/jfr/jni/jfrJniMethod.cpp index e0e6979a4e6..cc618e3cfbc 100644 --- a/src/hotspot/share/jfr/jni/jfrJniMethod.cpp +++ b/src/hotspot/share/jfr/jni/jfrJniMethod.cpp @@ -300,13 +300,13 @@ JVM_ENTRY_NO_ENV(jobject, jfr_new_event_writer(JNIEnv* env, jclass jvm)) return JfrJavaEventWriter::new_event_writer(thread); JVM_END -NO_TRANSITION(void, jfr_event_writer_flush(JNIEnv* env, jclass jvm, jobject writer, jint used_size, jint requested_size)) - JfrJavaEventWriter::flush(writer, used_size, requested_size, JavaThread::current()); -NO_TRANSITION_END +JVM_ENTRY_NO_ENV(void, jfr_event_writer_flush(JNIEnv* env, jclass jvm, jobject writer, jint used_size, jint requested_size)) + JfrJavaEventWriter::flush(writer, used_size, requested_size, thread); +JVM_END -NO_TRANSITION(jlong, jfr_commit(JNIEnv* env, jclass jvm, jlong next_position)) - return JfrJavaEventWriter::commit(next_position); -NO_TRANSITION_END +JVM_ENTRY_NO_ENV(jlong, jfr_commit(JNIEnv* env, jclass jvm, jlong next_position)) + return JfrJavaEventWriter::commit(next_position, thread); +JVM_END JVM_ENTRY_NO_ENV(void, jfr_flush(JNIEnv* env, jclass jvm)) JfrRepository::flush(thread); diff --git a/src/hotspot/share/jfr/recorder/checkpoint/jfrCheckpointManager.hpp b/src/hotspot/share/jfr/recorder/checkpoint/jfrCheckpointManager.hpp index 8dd3ed34e97..8a8582eb6f2 100644 --- a/src/hotspot/share/jfr/recorder/checkpoint/jfrCheckpointManager.hpp +++ b/src/hotspot/share/jfr/recorder/checkpoint/jfrCheckpointManager.hpp @@ -1,5 +1,5 @@ /* - * Copyright (c) 2016, 2023, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2016, 2025, Oracle and/or its affiliates. All rights reserved. * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. * * This code is free software; you can redistribute it and/or modify it diff --git a/src/hotspot/share/jfr/recorder/service/jfrRecorderService.cpp b/src/hotspot/share/jfr/recorder/service/jfrRecorderService.cpp index 31b2311af6c..07fe019c9af 100644 --- a/src/hotspot/share/jfr/recorder/service/jfrRecorderService.cpp +++ b/src/hotspot/share/jfr/recorder/service/jfrRecorderService.cpp @@ -486,6 +486,7 @@ void JfrRecorderService::safepoint_clear() { assert(SafepointSynchronize::is_at_safepoint(), "invariant"); _checkpoint_manager.begin_epoch_shift(); _storage.clear(); + _checkpoint_manager.notify_threads(); _chunkwriter.set_time_stamp(); JfrDeprecationManager::on_safepoint_clear(); JfrStackTraceRepository::clear(); @@ -650,9 +651,7 @@ size_t JfrRecorderService::flush() { if (_string_pool.is_modified()) { total_elements += flush_stringpool(_string_pool, _chunkwriter); } - if (_stack_trace_repository.is_modified()) { - total_elements += flush_stacktrace(_stack_trace_repository, _chunkwriter); - } + total_elements += flush_stacktrace(_stack_trace_repository, _chunkwriter); return flush_typeset(_checkpoint_manager, _chunkwriter) + total_elements; } diff --git a/src/hotspot/share/jfr/recorder/stacktrace/jfrStackTraceRepository.cpp b/src/hotspot/share/jfr/recorder/stacktrace/jfrStackTraceRepository.cpp index a30c0d8f262..c5909d49b9e 100644 --- a/src/hotspot/share/jfr/recorder/stacktrace/jfrStackTraceRepository.cpp +++ b/src/hotspot/share/jfr/recorder/stacktrace/jfrStackTraceRepository.cpp @@ -92,13 +92,9 @@ void JfrStackTraceRepository::destroy() { _leak_profiler_instance = nullptr; } -bool JfrStackTraceRepository::is_modified() const { - return _last_entries != _entries; -} - size_t JfrStackTraceRepository::write(JfrChunkWriter& sw, bool clear) { MutexLocker lock(JfrStacktrace_lock, Mutex::_no_safepoint_check_flag); - if (_entries == 0) { + if ((_entries == _last_entries) && !clear) { return 0; } int count = 0; diff --git a/src/hotspot/share/jfr/recorder/stacktrace/jfrStackTraceRepository.hpp b/src/hotspot/share/jfr/recorder/stacktrace/jfrStackTraceRepository.hpp index 84c11d159f1..5e2ba316588 100644 --- a/src/hotspot/share/jfr/recorder/stacktrace/jfrStackTraceRepository.hpp +++ b/src/hotspot/share/jfr/recorder/stacktrace/jfrStackTraceRepository.hpp @@ -1,5 +1,5 @@ /* - * Copyright (c) 2011, 2023, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2011, 2025, Oracle and/or its affiliates. All rights reserved. * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. * * This code is free software; you can redistribute it and/or modify it @@ -56,7 +56,6 @@ class JfrStackTraceRepository : public JfrCHeapObj { static void destroy(); bool initialize(); - bool is_modified() const; static size_t clear(); static size_t clear(JfrStackTraceRepository& repo); size_t write(JfrChunkWriter& cw, bool clear); diff --git a/src/hotspot/share/jfr/support/jfrIntrinsics.cpp b/src/hotspot/share/jfr/support/jfrIntrinsics.cpp index 306e223ee2e..54208b4aebf 100644 --- a/src/hotspot/share/jfr/support/jfrIntrinsics.cpp +++ b/src/hotspot/share/jfr/support/jfrIntrinsics.cpp @@ -37,35 +37,14 @@ static void assert_precondition(JavaThread* jt) { DEBUG_ONLY(JfrJavaSupport::check_java_thread_in_java(jt);) assert(jt->has_last_Java_frame(), "invariant"); } - -static void assert_epoch_identity(JavaThread* jt, u2 current_epoch) { - assert_precondition(jt); - // Verify the epoch updates got written through also to the vthread object. - const u2 epoch_raw = ThreadIdAccess::epoch(jt->vthread()); - const bool excluded = epoch_raw & excluded_bit; - assert(!excluded, "invariant"); - assert(!JfrThreadLocal::is_excluded(jt), "invariant"); - const u2 vthread_epoch = epoch_raw & epoch_mask; - assert(vthread_epoch == current_epoch, "invariant"); -} -#endif // ASSERT +#endif void* JfrIntrinsicSupport::write_checkpoint(JavaThread* jt) { DEBUG_ONLY(assert_precondition(jt);) assert(JfrThreadLocal::is_vthread(jt), "invariant"); - const u2 vthread_thread_local_epoch = JfrThreadLocal::vthread_epoch(jt); - const u2 current_epoch = ThreadIdAccess::current_epoch(); - MACOS_AARCH64_ONLY(ThreadWXEnable __wx(WXWrite, jt)); - if (vthread_thread_local_epoch == current_epoch) { - // After the epoch test in the intrinsic, the thread sampler interleaved - // and suspended the thread. As part of taking a sample, it updated - // the vthread object and the thread local "for us". We are good. - DEBUG_ONLY(assert_epoch_identity(jt, current_epoch);) - ThreadInVMfromJava transition(jt); - return JfrJavaEventWriter::event_writer(jt); - } const traceid vthread_tid = JfrThreadLocal::vthread_id(jt); - // Transition before reading the epoch generation anew, now as _thread_in_vm. Can safepoint here. + // Transition before reading the epoch generation, now as _thread_in_vm. + MACOS_AARCH64_ONLY(ThreadWXEnable __wx(WXWrite, jt)); ThreadInVMfromJava transition(jt); JfrThreadLocal::set_vthread_epoch(jt, vthread_tid, ThreadIdAccess::current_epoch()); return JfrJavaEventWriter::event_writer(jt); @@ -74,12 +53,11 @@ void* JfrIntrinsicSupport::write_checkpoint(JavaThread* jt) { void* JfrIntrinsicSupport::return_lease(JavaThread* jt) { DEBUG_ONLY(assert_precondition(jt);) MACOS_AARCH64_ONLY(ThreadWXEnable __wx(WXWrite, jt)); - ThreadStateTransition::transition_from_java(jt, _thread_in_native); + ThreadInVMfromJava transition(jt); assert(jt->jfr_thread_local()->has_java_event_writer(), "invariant"); assert(jt->jfr_thread_local()->shelved_buffer() != nullptr, "invariant"); JfrJavaEventWriter::flush(jt->jfr_thread_local()->java_event_writer(), 0, 0, jt); assert(jt->jfr_thread_local()->shelved_buffer() == nullptr, "invariant"); - ThreadStateTransition::transition_from_native(jt, _thread_in_Java); return nullptr; } diff --git a/src/hotspot/share/jfr/writers/jfrJavaEventWriter.cpp b/src/hotspot/share/jfr/writers/jfrJavaEventWriter.cpp index 7f2d931623b..3a70d416058 100644 --- a/src/hotspot/share/jfr/writers/jfrJavaEventWriter.cpp +++ b/src/hotspot/share/jfr/writers/jfrJavaEventWriter.cpp @@ -118,7 +118,7 @@ bool JfrJavaEventWriter::initialize() { } void JfrJavaEventWriter::flush(jobject writer, jint used, jint requested, JavaThread* jt) { - DEBUG_ONLY(JfrJavaSupport::check_java_thread_in_native(jt)); + DEBUG_ONLY(JfrJavaSupport::check_java_thread_in_vm(jt)); assert(writer != nullptr, "invariant"); JfrBuffer* const current = jt->jfr_thread_local()->java_buffer(); assert(current != nullptr, "invariant"); @@ -130,9 +130,6 @@ void JfrJavaEventWriter::flush(jobject writer, jint used, jint requested, JavaTh const bool is_valid = buffer->free_size() >= (size_t)(used + requested); u1* const new_current_position = is_valid ? buffer->pos() + used : buffer->pos(); assert(start_pos_offset != invalid_offset, "invariant"); - // can safepoint here - MACOS_AARCH64_ONLY(ThreadWXEnable __wx(WXWrite, jt)); - ThreadInVMfromNative transition(jt); oop const w = JNIHandles::resolve_non_null(writer); assert(w != nullptr, "invariant"); w->long_field_put(start_pos_offset, (jlong)buffer->pos()); @@ -147,11 +144,10 @@ void JfrJavaEventWriter::flush(jobject writer, jint used, jint requested, JavaTh } } -jlong JfrJavaEventWriter::commit(jlong next_position) { +jlong JfrJavaEventWriter::commit(jlong next_position, JavaThread* jt) { assert(next_position != 0, "invariant"); - JavaThread* const jt = JavaThread::current(); assert(jt != nullptr, "invariant"); - DEBUG_ONLY(JfrJavaSupport::check_java_thread_in_native(jt)); + DEBUG_ONLY(JfrJavaSupport::check_java_thread_in_vm(jt)); JfrThreadLocal* const tl = jt->jfr_thread_local(); assert(tl != nullptr, "invariant"); assert(tl->has_java_event_writer(), "invariant"); @@ -167,12 +163,11 @@ jlong JfrJavaEventWriter::commit(jlong next_position) { } // set_pos() has release semantics current->set_pos(next); - if (!current->lease()) { - return next_position; + if (current->lease()) { + flush(tl->java_event_writer(), 0, 0, jt); + return 0; // signals that the buffer lease was returned. } - assert(current->lease(), "invariant"); - flush(tl->java_event_writer(), 0, 0, jt); - return 0; // signals that the buffer lease was returned. + return next_position; } class JfrJavaEventWriterNotificationClosure : public ThreadClosure { diff --git a/src/hotspot/share/jfr/writers/jfrJavaEventWriter.hpp b/src/hotspot/share/jfr/writers/jfrJavaEventWriter.hpp index 92a9a7a693e..36067bd3870 100644 --- a/src/hotspot/share/jfr/writers/jfrJavaEventWriter.hpp +++ b/src/hotspot/share/jfr/writers/jfrJavaEventWriter.hpp @@ -50,7 +50,7 @@ class JfrJavaEventWriter : AllStatic { static jobject event_writer(JavaThread* t); static jobject new_event_writer(TRAPS); static void flush(jobject writer, jint used, jint requested, JavaThread* jt); - static jlong commit(jlong next_position); + static jlong commit(jlong next_position, JavaThread* jt); }; #endif // SHARE_JFR_WRITERS_JFRJAVAEVENTWRITER_HPP