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

[Zhengyu Gu]

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.

> 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.

> *
> *
> 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,

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