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

[Stefan Karlsson]

On 05/23/2013 03:06 PM, Zhengyu Gu wrote:
>
> 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?

I just tried to understand what the code in bullet 1 above was actually 
doing:

-  MemTracker::record_virtual_memory_type((address)heap_rs.base(), mtGC);
+  NMTTrackOp op(NMTTrackOp::TypeOp);
+  op.execute_op((address)heap_rs.base(), 0, mtGC);

StefanK

>
> 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/5092f36f/attachment.html