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

[Stefan Karlsson]

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?

>
>> 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/88a6bd28/attachment.html