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

[Coleen Phillmore]

On 1/3/2014 2:38 PM, Jon Masamitsu wrote:
>
> 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");
> }

I think the OrderAccess::storestore() should accomplish this. 
chunks_in_use() shouldn't be null after this though except for at 
deletion time.   So this code seems overly paranoid about the C++ 
compiler not doing the stores.  Actually the C++ compiler could keep all 
of this in a register and we'd never see things in the right order.

Here's a new version with the OrderAccess::storestore();   I reran the 
parallel class loading tests on this.

http://cr.openjdk.java.net/~coleenp/8029178_2/ 
<http://cr.openjdk.java.net/%7Ecoleenp/8029178_2/>

Thanks,
Coleen
>
> 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
>>>
>>
>