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

[Jon Masamitsu]

Looks good.

Jon

On 01/03/2014 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/>
>
> 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
>>>>
>>>
>>
>