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

Zhengyu Gu zhengyu.gu at oracle.com
Fri Jun 7 07:55:53 PDT 2013 

David,

On 6/6/2013 10:31 PM, David Holmes wrote:
> Hi Zhengyu,
>
> Not a full review by any means. I understand the issue being addressed 
> in basic form, and see how you have addressed it, but I'm not familiar 
> enough with the NMT details to evaluate in depth.
>
> Minor typos in memTracker.cpp comments:
>
> shoul -> should
> boundry -> boundary
>
Thanks to point out.

> I'm confused about the expectations of the constructor:
>
> MemTracker::Tracker::Tracker(MemoryOperation op, Thread* thr)
>
> Is thr, if not NULL, always the current thread? If so, then I don't 
> think it would be allocating from a SafepointSafe state; and if not 
> then it could change state immediately after you have checked it!
>
Yes, thr is the current thread, and we actually do classloading in 
_thread_in_native state, following is the stack.

 > 
jvm.dll!MemTracker::Tracker::Tracker(MemTracker::Tracker::MemoryOperation op, 
Thread * thr)  Line 729    C++
      jvm.dll!MemTracker::record_arena_size(unsigned char * addr, 
unsigned int size)  Line 344    C++
      jvm.dll!Arena::set_size_in_bytes(unsigned int size)  Line 532 + 
0xd bytes    C++
      jvm.dll!Arena::grow(unsigned int x, 
AllocFailStrategy::AllocFailEnum alloc_failmode)  Line 570    C++
      jvm.dll!Arena::Amalloc(unsigned int x, 
AllocFailStrategy::AllocFailEnum alloc_failmode)  Line 388 + 0x10 
bytes    C++
      jvm.dll!ResourceArea::allocate_bytes(unsigned int size, 
AllocFailStrategy::AllocFailEnum alloc_failmode)  Line 71    C++
      jvm.dll!resource_allocate_bytes(unsigned int size, 
AllocFailStrategy::AllocFailEnum alloc_failmode)  Line 39    C++
      jvm.dll!ClassPathZipEntry::open_stream(const char * name)  Line 
265 + 0xe bytes    C++
      jvm.dll!LazyClassPathEntry::open_stream(const char * name) Line 
323    C++
      jvm.dll!ClassLoader::load_classfile(Symbol * h_name, Thread * 
__the_thread__)  Line 900 + 0x17 bytes    C++
      jvm.dll!SystemDictionary::load_instance_class(Symbol * class_name, 
Handle class_loader, Thread * __the_thread__)  Line 1284 + 0x14 bytes    C++
      jvm.dll!SystemDictionary::resolve_instance_class_or_null(Symbol * 
name, Handle class_loader, Handle protection_domain, Thread * 
__the_thread__)  Line 769 + 0x18 bytes    C++
      jvm.dll!SystemDictionary::resolve_or_null(Symbol * class_name, 
Handle class_loader, Handle protection_domain, Thread * __the_thread__)  
Line 227 + 0x15 bytes    C++
      jvm.dll!SystemDictionary::resolve_or_fail(Symbol * class_name, 
Handle class_loader, Handle protection_domain, bool throw_error, Thread 
* __the_thread__)  Line 166 + 0x15 bytes    C++
      jvm.dll!ConstantPool::klass_at_impl(constantPoolHandle this_oop, 
int which, Thread * __the_thread__)  Line 252 + 0x17 bytes    C++
      jvm.dll!ConstantPool::klass_at(int which, Thread * 
__the_thread__)  Line 352 + 0x1b bytes    C++
      jvm.dll!InterpreterRuntime::anewarray(JavaThread * thread, 
ConstantPool * pool, int index, int size)  Line 187 + 0x10 bytes C++
      02ee6241()
      jvm.dll!JavaCalls::call_helper(JavaValue * result, methodHandle * 
m, JavaCallArguments * args, Thread * __the_thread__)  Line 402 + 0x36 
bytes    C++
      jvm.dll!os::os_exception_wrapper(void (JavaValue *, methodHandle 
*, JavaCallArguments *, Thread *)* f, JavaValue * value, methodHandle * 
method, JavaCallArguments * args, Thread * thread)  Line 113 + 0x13 
bytes    C++
      jvm.dll!JavaCalls::call(JavaValue * result, methodHandle method, 
JavaCallArguments * args, Thread * __the_thread__)  Line 307 + 0x1a 
bytes    C++
jvm.dll!InstanceKlass::call_class_initializer_impl(instanceKlassHandle 
this_oop, Thread * __the_thread__)  Line 1125 + 0x1f bytes    C++
      jvm.dll!InstanceKlass::call_class_initializer(Thread * 
__the_thread__)  Line 1093 + 0xd bytes    C++
      jvm.dll!InstanceKlass::initialize_impl(instanceKlassHandle 
this_oop, Thread * __the_thread__)  Line 843    C++
      jvm.dll!InstanceKlass::initialize(Thread * __the_thread__) Line 
502 + 0xd bytes    C++
      jvm.dll!initialize_class(Symbol * class_name, Thread * 
__the_thread__)  Line 973 + 0x20 bytes    C++
      jvm.dll!Threads::create_vm(JavaVMInitArgs * args, bool * 
canTryAgain)  Line 3479 + 0xf bytes    C++
      jvm.dll!JNI_CreateJavaVM(JavaVM_ * * vm, void * * penv, void * 
args)  Line 5113 + 0xd bytes    C++
      java.exe!013713c1()
      [Frames below may be incorrect and/or missing, no symbols loaded 
for java.exe]
      java.exe!01371e78()
      java.exe!0137cf67()
      java.exe!0137ab83()
      java.exe!0137cfa5()
      java.exe!0137ac0d()
      kernel32.dll!768b3677()
      ntdll.dll!76fe9f42()
      ntdll.dll!76fe9f15()


> In memTracker.hpp for the !INCLUDE_NMT case class Tracker doesn't have 
> an allocation-type as a supertype. I'm also unclear whether the 
> methods that have:
>
> return Tracker();
>
> are going to be returning a stack-allocated object, or whether this 
> will actually force use of a copy-constructor at the call site. We 
> really want this to be a no-op (or as close as possible) when NMT is 
> not compiled in. Perhaps a static singleton instance of Tracker could 
> be used instead?
>
Yes, static on can help solaris. Windows and Linux all optimize the copy 
constructor away.

Thanks,

-Zhengyu

> Thanks,
> David
>
> On 5/06/2013 12:26 AM, Zhengyu Gu wrote:
>> Round 3:
>>
>> Based on Coleen and Stefan's comment, reverted most of NMT tracking
>> calls to original to reduce code changes.
>>
>> http://cr.openjdk.java.net/~zgu/8013651/webrev.03/
>> <http://cr.openjdk.java.net/%7Ezgu/8013651/webrev.03/>
>>
>>
>> Tests:
>>    JPRT
>>    vm.quick.testlist on Linux 32
>>
>> 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
>>>
>>

More information about the hotspot-runtime-dev mailing list