From f7a9592fe1678a1b345281c54c3d515d77a86e1b Mon Sep 17 00:00:00 2001 From: coleenp Date: Tue, 7 Jan 2014 13:26:56 -0500 Subject: [PATCH] 8029178: Parallel class loading test anonymous-simple gets SIGSEGV in Metaspace::contains Summary: Metaspace::contains cannot look at purged metaspaces while CMS concurrently deallocates them. Reviewed-by: mgerdin, sspitsyn, jmasa --- src/share/vm/classfile/classLoaderData.cpp | 21 +++----- src/share/vm/classfile/classLoaderData.hpp | 4 +- src/share/vm/code/dependencies.cpp | 2 - src/share/vm/memory/allocation.cpp | 3 +- src/share/vm/memory/allocation.hpp | 2 +- src/share/vm/memory/metachunk.hpp | 2 + src/share/vm/memory/metaspace.cpp | 57 +++++++++++----------- src/share/vm/memory/metaspace.hpp | 2 +- src/share/vm/oops/klass.cpp | 2 - src/share/vm/runtime/os.cpp | 2 - 10 files changed, 43 insertions(+), 54 deletions(-) diff --git a/src/share/vm/classfile/classLoaderData.cpp b/src/share/vm/classfile/classLoaderData.cpp index 273e0184d..892ea92fb 100644 --- a/src/share/vm/classfile/classLoaderData.cpp +++ b/src/share/vm/classfile/classLoaderData.cpp @@ -648,12 +648,12 @@ GrowableArray* ClassLoaderDataGraph::new_clds() { return array; } -#ifndef PRODUCT -// for debugging and hsfind(x) -bool ClassLoaderDataGraph::contains(address x) { - // I think we need the _metaspace_lock taken here because the class loader - // data graph could be changing while we are walking it (new entries added, - // new entries being unloaded, etc). +// For profiling and hsfind() only. Otherwise, this is unsafe (and slow). This +// is done lock free to avoid lock inversion problems. It is safe because +// new ClassLoaderData are added to the end of the CLDG, and only removed at +// safepoint. The _unloading list can be deallocated concurrently with CMS so +// this doesn't look in metaspace for classes that have been unloaded. +bool ClassLoaderDataGraph::contains(const void* x) { if (DumpSharedSpaces) { // There are only two metaspaces to worry about. ClassLoaderData* ncld = ClassLoaderData::the_null_class_loader_data(); @@ -670,16 +670,11 @@ bool ClassLoaderDataGraph::contains(address x) { } } - // Could also be on an unloading list which is okay, ie. still allocated - // for a little while. - for (ClassLoaderData* ucld = _unloading; ucld != NULL; ucld = ucld->next()) { - if (ucld->metaspace_or_null() != NULL && ucld->metaspace_or_null()->contains(x)) { - return true; - } - } + // Do not check unloading list because deallocation can be concurrent. return false; } +#ifndef PRODUCT bool ClassLoaderDataGraph::contains_loader_data(ClassLoaderData* loader_data) { for (ClassLoaderData* data = _head; data != NULL; data = data->next()) { if (loader_data == data) { diff --git a/src/share/vm/classfile/classLoaderData.hpp b/src/share/vm/classfile/classLoaderData.hpp index 2cb21801d..9e76b5044 100644 --- a/src/share/vm/classfile/classLoaderData.hpp +++ b/src/share/vm/classfile/classLoaderData.hpp @@ -90,9 +90,9 @@ class ClassLoaderDataGraph : public AllStatic { static void dump() { dump_on(tty); } static void verify(); -#ifndef PRODUCT // expensive test for pointer in metaspace for debugging - static bool contains(address x); + static bool contains(const void* x); +#ifndef PRODUCT static bool contains_loader_data(ClassLoaderData* loader_data); #endif diff --git a/src/share/vm/code/dependencies.cpp b/src/share/vm/code/dependencies.cpp index 4419bdd98..b4a3aee7f 100644 --- a/src/share/vm/code/dependencies.cpp +++ b/src/share/vm/code/dependencies.cpp @@ -655,8 +655,6 @@ inline Metadata* Dependencies::DepStream::recorded_metadata_at(int i) { } else { o = _deps->oop_recorder()->metadata_at(i); } - assert(o == NULL || o->is_metaspace_object(), - err_msg("Should be metadata " PTR_FORMAT, o)); return o; } diff --git a/src/share/vm/memory/allocation.cpp b/src/share/vm/memory/allocation.cpp index 7b87bc3a7..98410227a 100644 --- a/src/share/vm/memory/allocation.cpp +++ b/src/share/vm/memory/allocation.cpp @@ -71,9 +71,8 @@ bool MetaspaceObj::is_shared() const { return MetaspaceShared::is_in_shared_space(this); } - bool MetaspaceObj::is_metaspace_object() const { - return Metaspace::contains((void*)this); + return ClassLoaderDataGraph::contains((void*)this); } void MetaspaceObj::print_address_on(outputStream* st) const { diff --git a/src/share/vm/memory/allocation.hpp b/src/share/vm/memory/allocation.hpp index 739753311..590b74f5d 100644 --- a/src/share/vm/memory/allocation.hpp +++ b/src/share/vm/memory/allocation.hpp @@ -264,7 +264,7 @@ class ClassLoaderData; class MetaspaceObj { public: - bool is_metaspace_object() const; // more specific test but slower + bool is_metaspace_object() const; bool is_shared() const; void print_address_on(outputStream* st) const; // nonvirtual address printing diff --git a/src/share/vm/memory/metachunk.hpp b/src/share/vm/memory/metachunk.hpp index 3bd4d15bc..e873dc6a3 100644 --- a/src/share/vm/memory/metachunk.hpp +++ b/src/share/vm/memory/metachunk.hpp @@ -143,6 +143,8 @@ class Metachunk : public Metabase { void set_is_tagged_free(bool v) { _is_tagged_free = v; } #endif + bool contains(const void* ptr) { return bottom() <= ptr && ptr < _top; } + NOT_PRODUCT(void mangle();) void print_on(outputStream* st) const; diff --git a/src/share/vm/memory/metaspace.cpp b/src/share/vm/memory/metaspace.cpp index 19c03661a..5a7e4ec2f 100644 --- a/src/share/vm/memory/metaspace.cpp +++ b/src/share/vm/memory/metaspace.cpp @@ -513,8 +513,6 @@ class VirtualSpaceList : public CHeapObj { // Unlink empty VirtualSpaceNodes and free it. void purge(ChunkManager* chunk_manager); - bool contains(const void *ptr); - void print_on(outputStream* st) const; class VirtualSpaceListIterator : public StackObj { @@ -558,7 +556,7 @@ class SpaceManager : public CHeapObj { private: - // protects allocations and contains. + // protects allocations Mutex* const _lock; // Type of metadata allocated. @@ -595,7 +593,11 @@ class SpaceManager : public CHeapObj { private: // Accessors Metachunk* chunks_in_use(ChunkIndex index) const { return _chunks_in_use[index]; } - void set_chunks_in_use(ChunkIndex index, Metachunk* v) { _chunks_in_use[index] = v; } + void set_chunks_in_use(ChunkIndex index, Metachunk* v) { + // ensure lock-free iteration sees fully initialized node + OrderAccess::storestore(); + _chunks_in_use[index] = v; + } BlockFreelist* block_freelists() const { return (BlockFreelist*) &_block_freelists; @@ -708,6 +710,8 @@ class SpaceManager : public CHeapObj { void print_on(outputStream* st) const; void locked_print_chunks_in_use_on(outputStream* st) const; + bool contains(const void *ptr); + void verify(); void verify_chunk_size(Metachunk* chunk); NOT_PRODUCT(void mangle_freed_chunks();) @@ -1159,8 +1163,6 @@ bool VirtualSpaceList::create_new_virtual_space(size_t vs_word_size) { } else { assert(new_entry->reserved_words() == vs_word_size, "Reserved memory size differs from requested memory size"); - // ensure lock-free iteration sees fully initialized node - OrderAccess::storestore(); link_vs(new_entry); return true; } @@ -1287,19 +1289,6 @@ void VirtualSpaceList::print_on(outputStream* st) const { } } -bool VirtualSpaceList::contains(const void *ptr) { - VirtualSpaceNode* list = virtual_space_list(); - VirtualSpaceListIterator iter(list); - while (iter.repeat()) { - VirtualSpaceNode* node = iter.get_next(); - if (node->reserved()->contains(ptr)) { - return true; - } - } - return false; -} - - // MetaspaceGC methods // VM_CollectForMetadataAllocation is the vm operation used to GC. @@ -2392,6 +2381,21 @@ MetaWord* SpaceManager::allocate_work(size_t word_size) { return result; } +// This function looks at the chunks in the metaspace without locking. +// The chunks are added with store ordering and not deleted except for at +// unloading time. +bool SpaceManager::contains(const void *ptr) { + for (ChunkIndex i = ZeroIndex; i < NumberOfInUseLists; i = next_chunk_index(i)) + { + Metachunk* curr = chunks_in_use(i); + while (curr != NULL) { + if (curr->contains(ptr)) return true; + curr = curr->next(); + } + } + return false; +} + void SpaceManager::verify() { // If there are blocks in the dictionary, then // verfication of chunks does not work since @@ -3463,17 +3467,12 @@ void Metaspace::print_on(outputStream* out) const { } } -bool Metaspace::contains(const void * ptr) { - if (MetaspaceShared::is_in_shared_space(ptr)) { - return true; +bool Metaspace::contains(const void* ptr) { + if (vsm()->contains(ptr)) return true; + if (using_class_space()) { + return class_vsm()->contains(ptr); } - // This is checked while unlocked. As long as the virtualspaces are added - // at the end, the pointer will be in one of them. The virtual spaces - // aren't deleted presently. When they are, some sort of locking might - // be needed. Note, locking this can cause inversion problems with the - // caller in MetaspaceObj::is_metadata() function. - return space_list()->contains(ptr) || - (using_class_space() && class_space_list()->contains(ptr)); + return false; } void Metaspace::verify() { diff --git a/src/share/vm/memory/metaspace.hpp b/src/share/vm/memory/metaspace.hpp index c22d21707..96427cb98 100644 --- a/src/share/vm/memory/metaspace.hpp +++ b/src/share/vm/memory/metaspace.hpp @@ -225,7 +225,7 @@ class Metaspace : public CHeapObj { MetaWord* expand_and_allocate(size_t size, MetadataType mdtype); - static bool contains(const void *ptr); + bool contains(const void* ptr); void dump(outputStream* const out) const; // Free empty virtualspaces diff --git a/src/share/vm/oops/klass.cpp b/src/share/vm/oops/klass.cpp index 22b570bbf..bf5dae060 100644 --- a/src/share/vm/oops/klass.cpp +++ b/src/share/vm/oops/klass.cpp @@ -376,8 +376,6 @@ void Klass::append_to_sibling_list() { } bool Klass::is_loader_alive(BoolObjectClosure* is_alive) { - assert(ClassLoaderDataGraph::contains((address)this), "is in the metaspace"); - #ifdef ASSERT // The class is alive iff the class loader is alive. oop loader = class_loader(); diff --git a/src/share/vm/runtime/os.cpp b/src/share/vm/runtime/os.cpp index e2d930a1d..e2ea94275 100644 --- a/src/share/vm/runtime/os.cpp +++ b/src/share/vm/runtime/os.cpp @@ -1081,7 +1081,6 @@ void os::print_location(outputStream* st, intptr_t x, bool verbose) { } -#ifndef PRODUCT // Check if in metaspace. if (ClassLoaderDataGraph::contains((address)addr)) { // Use addr->print() from the debugger instead (not here) @@ -1089,7 +1088,6 @@ void os::print_location(outputStream* st, intptr_t x, bool verbose) { " is pointing into metadata", addr); return; } -#endif // Try an OS specific find if (os::find(addr, st)) { -- GitLab