Round 2: RFR: 8013651 NMT: reserve/release sequence id's in incorrect order due to race

[Zhengyu Gu]

On 5/23/2013 8:14 AM, Stefan Karlsson wrote:
> On 05/23/2013 01:57 PM, Zhengyu Gu wrote:
>> Hi Stefan,
>>
>> Thanks for review.  Although this is old webrev, your comments are 
>> still valid.
>> On 5/23/2013 1:48 AM, Stefan Karlsson wrote:
>>> Zhengyu,
>>>
>>> Not a full review, but I looked at some parts that affect the GC 
>>> code. I have couple of comments.
>>>
>>> 1) I see this pattern all over the GC code:
>>> -  MemTracker::record_virtual_memory_type((address)heap_rs.base(), mtGC);
>>> +  NMTTrackOp op(NMTTrackOp::TypeOp);
>>> +  op.execute_op((address)heap_rs.base(), 0, mtGC);
>>> Could you create a static function in NMTTrackOp so that we can 
>>> change these lines into a one-liner? For example:
>>> NMTTrackOp::execute(NMTTrackOp::TypeOp, (address)heap_rs.base(), 0, mtGC
>> Sure.
>>> 2) I don't understand what TypeOp means here.
>>>
>> Type operation is to assign a category to the virtual memory. For 
>> example, once VM reserves memory for JavaHeap, NMT needs to tag this 
>> memory to mtJavaHeap.
>
> So the call above means tag this memory address at heap_rs.base() with 
> mtGC?
Are you referring the code in src/share/vm/memory/cardTableModRefBS.cpp, 
line 96? should card table be categorized to GC?

Please let me know if I tagged it wrong.

Thanks,

-Zhengyu

>
>>
>>> 3) You have an implementation for the constructor, both in the hpp 
>>> file and the cpp file:
>>> *+   NMTTrackOp(NMTMemoryOps op, Thread* thr = NULL) { }
>>> *
>> I think above constructor is under #ifndef INCLUDE_NMT. It is a No-Op 
>> dummy class.
>
> I see. That's a really confusing way to exclude code, IMHO.
>
>>
>>> *
>>> *
>>> 4) The following functions bring in a dependency against atomic.hpp 
>>> (actually, they need atomic.inline.hpp):
>>> +  static void inc_pending_op_count() { Atomic::inc(&_pending_op_count); }
>>> +  static void dec_pending_op_count() {
>>> +    Atomic::dec(&_pending_op_count);
>>> +    assert(_pending_op_count >= 0, "Sanity check");
>>> +  }
>>> This just adds to the include mess which makes the code much more 
>>> susceptible to circular include dependencies.
>>>
>>> It's better to move the implementation to the .cpp,or inline.hpp if 
>>> they really need to be inlined.
>>>
>> Will do.
>
> Thanks!
>
> StefanK
>
>>
>> Thanks,
>>
>> -Zhengyu
>>
>>> thanks,
>>> StefanK
>>>
>>> On 05/22/2013 11:41 PM, Zhengyu Gu wrote:
>>>> Oops, forgot the link: 
>>>> http://cr.openjdk.java.net/~zgu/8013651/webrev.01/
>>>>
>>>> Thanks,
>>>>
>>>> -Zhengyu
>>>>
>>>> On 5/22/2013 10:28 AM, Zhengyu Gu wrote:
>>>>> Based on the discussion with Karen, Coleen and Harold, following 
>>>>> changes are made:
>>>>>
>>>>> 1) Renamed NMTTrackOp to NMTTracker, avoid the confusion with VM 
>>>>> operations.
>>>>> 2) Used NMTTracker's dtor to discard the tracking operation if no 
>>>>> recording is done.
>>>>>
>>>>> Tests:
>>>>>  - JPRT
>>>>>  - vm.quick.testlist on Linux 32, Solaris sparcv9 and Windows 32.
>>>>>
>>>>> Thanks,
>>>>>
>>>>> -Zhengyu
>>>>>
>>>>>
>>>>> On 5/14/2013 10:01 AM, Zhengyu Gu wrote:
>>>>>> There can be race conditions between the memory operations and 
>>>>>> the book keeping records are written. For example, thread 1 
>>>>>> releases a virtual memory block, before it can write the release 
>>>>>> record, thread 2 reserves the same virtual memory block and 
>>>>>> writes reservation first, as result, NMT indicates the block is 
>>>>>> "released".
>>>>>>
>>>>>> The solution is that, for those operations that can cause the 
>>>>>> race conditions, NMT should pre-reserve sequence number for it, 
>>>>>> if the operation succeeds, NMT uses pre-reserved sequence number 
>>>>>> to write the record.
>>>>>>
>>>>>> The tricky part is that, a sequence number is only good for the 
>>>>>> generation it is acquired, when there are reserved sequence 
>>>>>> number, NMT has to prevent itself from entering so called 
>>>>>> "sync-point" where the generation can be advanced.
>>>>>>
>>>>>>
>>>>>> Bug: http://bugs.sun.com/view_bug.do?bug_id=8013651
>>>>>> Webrev: http://cr.openjdk.java.net/~zgu/8013651/webrev/ 
>>>>>> <http://cr.openjdk.java.net/%7Ezgu/8013651/webrev/>
>>>>>>
>>>>>>
>>>>>> Tests:
>>>>>>    1) JPRT
>>>>>>    2) vm.quick.testlist on Linux 32, Linux x64 and Solaris Sparcv9
>>>>>>    3) Kitchensink on Linux 32, Linux x64, Solaris Sparcv9 and 
>>>>>> Windows x64
>>>>>>    4) NMT jtreg tests on Linux 32, Linux x64 and Solaris Sparcv9
>>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>>
>>>>>> -Zhengyu
>>>>>
>>>>
>>>
>>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/attachments/20130523/788c7836/attachment-0001.html