RFR 8029178: Parallel class loading test anonymous-simple gets SIGSEGV in Metaspace:,:contains

[Jon Masamitsu]

On 1/3/2014 11:00 AM, Coleen Phillmore wrote:
>
> Jon,  Thanks for reviewing this.
>
> On 1/3/2014 1:09 PM, Jon Masamitsu wrote:
>> Coleen,
>>
>> http://cr.openjdk.java.net/~coleenp/8029178/src/share/vm/memory/metachunk.hpp.frames.html 
>>
>>
>> 146   bool contains(const void* ptr) { return bottom() < ptr && ptr 
>> <= _top; }
>>
>> I think bottom() points to the first chunk.  Also _top points to the 
>> start
>> of the unallocated space (next place where a chunk will be allocated).
>> So I would think this should be
>>
>> bool contains(const void* ptr) { return bottom() <=ptr && ptr < _top; }
>>
>>
>
> Yes, I agree.
>>
>> http://cr.openjdk.java.net/~coleenp/8029178/src/share/vm/memory/metaspace.cpp.frames.html 
>>
>>
>> Don't you need some locking to protect the "chunks_in_use()" lists?
>>
>> 2382 bool SpaceManager::contains(const void *ptr) {
>> 2383   for (ChunkIndex i = ZeroIndex; i < NumberOfInUseLists; i = 
>> next_chunk_index(i))
>> 2384   {
>> 2385     Metachunk* curr = chunks_in_use(i);
>> 2386     while (curr != NULL) {
>> 2387       if (curr->contains(ptr)) return true;
>> 2388       curr = curr->next();
>> 2389     }
>> 2390   }
>> 2391   return false;
>> 2392 }
>> 2393
>
> If new chunks are added while this is iterating over them, they are 
> added to the front of the list.  This is ok because we would only call 
> this for metadata that is already allocated in one of the chunks.   I 
> don't want to add a lock but I do need some memory ordering 
> instructions.   I'll move the one that I had on virtual space lists to 
> where we add the chunks.

Ok.  Do you think it is worth code in set_chunks_in_use() that checks
that the list head is set after the new chunk has been added?

Something like

set_chunks_in_use(new_chunk)
if (chunks_in_use() != NULL)
     assert(new_chunk->next == chunks_in_use(), "Always add new chunk to 
list first");
}

Just a thought.

Jon



>
> The Metaspace/SpaceManager destructors can only be called for the 
> _unloading list, which is created at a safepoint and we don't walk 
> those lists in CLDG::contains.
>
> Thank you for pointing these out.   I'll retest and send out another 
> webrev.
>
> Coleen
>>
>> Jon
>>
>>
>> On 1/2/2014 9:25 AM, Coleen Phillmore wrote:
>>> Summary: Metaspace::contains cannot look at purged metaspaces while 
>>> CMS concurrently deallocates them.
>>>
>>> Removed 2 calls to is_metaspace_object where the object may be in a 
>>> deallocated metaspace.  Removed walking virtual space lists for 
>>> determining contains because the virtual space list can change 
>>> concurrently with the walk.   CLDG::contains is slower but no 
>>> slowdowns with testing were observed.
>>>
>>> Tested by SQE testbase tests, jtreg tests.   Functional testing by 
>>> parallel class loading tests and nsk/coverage/arguments/arguments008 
>>> (ie. calls Method::is_valid_method)
>>>
>>> open webrev at http://cr.openjdk.java.net/~coleenp/8029178/
>>> bug link https://bugs.openjdk.java.net/browse/JDK-8029178
>>>
>>> Thanks,
>>> Coleen
>>
>