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

[Stefan Karlsson]

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

2) I don't understand what TypeOp means here.

3) You have an implementation for the constructor, both in the hpp file 
and the cpp file:

*+   NMTTrackOp(NMTMemoryOps op, Thread* thr = NULL) { }

*

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.

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