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

[Coleen Phillimore]

Thanks!
Coleen

On 01/06/2014 02:47 PM, serguei.spitsyn at oracle.com wrote:
> On 1/3/14 12:00 PM, Coleen Phillmore wrote:
>>
>> 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/>
>
> This looks good to me.
>
> Thanks,
> Serguei
>
>>
>> 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
>>>>>
>>>>
>>>
>>
>