Request for review (s): 7173959 : Jvm crashed during coherence exabus (tmb) testing

[Coleen Phillimore]

On 12/14/2012 10:24 AM, Bengt Rutisson wrote:
>
> Hi Coleen,
>
> Thanks for looking at this!
>
> On 12/14/12 2:21 PM, Coleen Phillimore wrote:
>> On 12/14/2012 3:48 AM, Bengt Rutisson wrote:
>>>
>>> Hi again Runtime team,
>>>
>>> I need one more review for this change. It is a P1 bug with a fairly 
>>> small change. Any takers?
>>>
>>> Latest webrev based on comments from David Holmes and Vitaly 
>>> Davidovich:
>>> http://cr.openjdk.java.net/~brutisso/7173959/webrev.03/
>>
>> Hi Bengt,   This code looks good to me but I asked Zhengyu to check 
>> it to make sure it doesn't have any bad NMT interactions.
>
> Thanks! He just replied.
>>
>> Also the writeup is great below, but can you add these comments to 
>> the code in os_posix.cpp.   I had to go back to the email to figure 
>> out why you are doing this.
>
> I'm not really sure where to put such a comment. I think the writeup I 
> did mostly makes sense when you compare it to the old code. Also, the 
> affected code is in the middle of ReservedSpace::initialize(). I think 
> it would look strange with a large comment there. In the 
> os_posix/windows files the comment does not make much sense in my 
> opinion.
>
> I could add a comment in ReservedSpace::initialize() that refers to 
> the Jira issue. But I'm not a big fan of such comments.

I agree we shouldn't point to bugs in the source code.   A comment at 
the top of the os_posix version of this function that paraphrases below 
why you are not re-reserving in a loop like windows would be 
sufficient.   That trimming memory at either end is allowed on 
posix-like os's and is thread-safe, something like:

// Multiple threads can race in this code, and can remap over each other 
with MAP_FIXED, so on posix, unmap the section
// at the start and at the end of the chunk that we mapped to get 
requested alignment.

os_windows:

// Multiple threads can race in this code but it's not possible to remap 
with a section of virtual space to get requested alignment, like 
posix-like os's.
// Windows prevents multiple thread from remapping over each other so 
this loop is thread-safe.

I just want something to document why they are different.

>
>>
>> I don't understand why there are racing threads.   I thought all the 
>> heap space was initialized at the start of the vm when it is single 
>> threaded?
>
> I would have to look up exactly when we set up the heap to see if we 
> are single threaded at that point. The issue we saw was with setting 
> up the GC bitmaps. At that point we are multithreaded and we are for 
> example mapping memory for the GC worker threads at the same time.

I see.

Coleen
>
>
> Thanks again for looking at this!
> Bengt
>
>>
>> thanks,
>> Coleen
>>
>>>
>>> Thanks,
>>> Bengt
>>>
>>>
>>> On 12/14/12 9:29 AM, Bengt Rutisson wrote:
>>>>
>>>> Hi David,
>>>>
>>>> On 12/14/12 1:38 AM, David Holmes wrote:
>>>>> Hi Bengt,
>>>>>
>>>>> That all sounds good to me.
>>>>>
>>>>> With regard to potential overflow perhaps a simple check that 
>>>>> extra_size does not overflow? If that is true and extra_base is 
>>>>> not null then I think all the other calculations must be valid.
>>>>
>>>> Yes, that makes sense. I'll add an overflow check for the extra_size.
>>>>
>>>> Thanks,
>>>> Bengt
>>>>
>>>>>
>>>>> Thanks,
>>>>> David
>>>>>
>>>>> On 13/12/2012 10:58 PM, Bengt Rutisson wrote:
>>>>>>
>>>>>> Hi David,
>>>>>>
>>>>>> Updated webrev:
>>>>>> http://cr.openjdk.java.net/~brutisso/7173959/webrev.02/
>>>>>>
>>>>>> I moved the alignment of "size" back into 
>>>>>> ReservedSpace::initialize()
>>>>>> since we actually store the size in an instance variable (_size), 
>>>>>> so I
>>>>>> think it is a bit risky to just do the align_up in
>>>>>> os::reserve_memory_aligned(). We probably want the instance 
>>>>>> variables
>>>>>> _size and _alignment in ReservedSpace to be consistent.
>>>>>>
>>>>>> I added an assert in os::reserve_memory_aligned() to verify that the
>>>>>> size is correctly aligned. I also added the assert you suggested to
>>>>>> check that alignment is page size aligned.
>>>>>>
>>>>>> Bengt
>>>>>>
>>>>>> On 12/13/12 11:50 AM, David Holmes wrote:
>>>>>>> On 13/12/2012 8:37 PM, Bengt Rutisson wrote:
>>>>>>>>
>>>>>>>> Hi again,
>>>>>>>>
>>>>>>>> Updated webrev:
>>>>>>>> http://cr.openjdk.java.net/~brutisso/7173959/webrev.01/
>>>>>>>>
>>>>>>>> I removed the comment and the alignment.
>>>>>>>>
>>>>>>>> But I did not add an assert just yet.
>>>>>>>>
>>>>>>>> At the top of ReservedSpace::initialize() we have this code:
>>>>>>>>
>>>>>>>> const size_t granularity = os::vm_allocation_granularity();
>>>>>>>> assert((size & (granularity - 1)) == 0,
>>>>>>>> "size not aligned to os::vm_allocation_granularity()");
>>>>>>>> assert((alignment & (granularity - 1)) == 0,
>>>>>>>> "alignment not aligned to os::vm_allocation_granularity()");
>>>>>>>>
>>>>>>>>
>>>>>>>> Where os::vm_allocation_granularity() is implemented as page 
>>>>>>>> size on all
>>>>>>>> platforms except Windows. There we look it up from the Windows 
>>>>>>>> API. I
>>>>>>>> assume that is a page size too, but since the Windows code in 
>>>>>>>> our patch
>>>>>>>> does not unmap based on the alignment it should be safe either 
>>>>>>>> way.
>>>>>>>>
>>>>>>>> Is this assert enough or would you like me to add an assert 
>>>>>>>> closer to
>>>>>>>> the code block were I did the changes?
>>>>>>>
>>>>>>> As this is a separate method now I think an assert in the method
>>>>>>> itself would not go astray.
>>>>>>>
>>>>>>> David
>>>>>>> -----
>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Bengt
>>>>>>>>
>>>>>>>>
>>>>>>>> On 12/13/12 11:02 AM, Bengt Rutisson wrote:
>>>>>>>>>
>>>>>>>>> Hi David,
>>>>>>>>>
>>>>>>>>> Thanks for looking at this!
>>>>>>>>>
>>>>>>>>> On 12/13/12 8:33 AM, David Holmes wrote:
>>>>>>>>>> Hi Bengt,
>>>>>>>>>>
>>>>>>>>>> This has to be one of the absolute best review requests I've 
>>>>>>>>>> ever
>>>>>>>>>> read :) Thank you.
>>>>>>>>>
>>>>>>>>> Wow! Thanks! :)
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Just a couple of queries.
>>>>>>>>>>
>>>>>>>>>> os_posix.cpp:
>>>>>>>>>>
>>>>>>>>>> Love the diagram :)
>>>>>>>>>
>>>>>>>>> It was Mikael's way of making sure we got it right.
>>>>>>>>>
>>>>>>>>>> I'm assuming that "alignment" has to be >= the underlying 
>>>>>>>>>> page size,
>>>>>>>>>> and in fact must be a multiple of the underlying page size ? 
>>>>>>>>>> (As I
>>>>>>>>>> assume you can only unmap whole numbers of pages). If so does 
>>>>>>>>>> that
>>>>>>>>>> need to be checked somewhere?
>>>>>>>>>
>>>>>>>>> Good point. I'll add an assert to check that.
>>>>>>>>>
>>>>>>>>>> In virtualSpace.cpp:
>>>>>>>>>>
>>>>>>>>>> // Reserve size large enough to do manual alignment and
>>>>>>>>>> // increase size to a multiple of the desired alignment
>>>>>>>>>> size = align_size_up(size, alignment);
>>>>>>>>>> ! base = os::reserve_memory_aligned(size, alignment);
>>>>>>>>>>
>>>>>>>>>> The comment doesn't seem necessary now, and the align_size_up 
>>>>>>>>>> seems
>>>>>>>>>> redundant.
>>>>>>>>>
>>>>>>>>> You are right. I'll remove the comment and the alignment.
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>> Bengt
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Thanks,
>>>>>>>>>> David
>>>>>>>>>>
>>>>>>>>>> On 13/12/2012 4:42 PM, Bengt Rutisson wrote:
>>>>>>>>>>>
>>>>>>>>>>> Hi all,
>>>>>>>>>>>
>>>>>>>>>>> Could I have a couple of reviews for this change?
>>>>>>>>>>>
>>>>>>>>>>> http://cr.openjdk.java.net/~brutisso/7173959/webrev.00/
>>>>>>>>>>>
>>>>>>>>>>> This is for a bug originally reported by the Coherence team:
>>>>>>>>>>>
>>>>>>>>>>> 7173959 : Jvm crashed during coherence exabus (tmb) testing
>>>>>>>>>>> http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7173959
>>>>>>>>>>>
>>>>>>>>>>> The original analysis and proposed fix was done by Mikael 
>>>>>>>>>>> Gerdin
>>>>>>>>>>> and me
>>>>>>>>>>> together. I'll handle the webrev and push since Mikael is on 
>>>>>>>>>>> vacation
>>>>>>>>>>> starting today. But Mikael did a great job tracking down 
>>>>>>>>>>> this very
>>>>>>>>>>> difficult bug, so he should have a large part of the credit 
>>>>>>>>>>> for this
>>>>>>>>>>> bug
>>>>>>>>>>> fix,
>>>>>>>>>>>
>>>>>>>>>>> Description from the CR:
>>>>>>>>>>>
>>>>>>>>>>> The reason that we crash is due to how we re-map memory when we
>>>>>>>>>>> want to
>>>>>>>>>>> align it for large pages in ReservedSpace::initialize().
>>>>>>>>>>>
>>>>>>>>>>> Here is what happens:
>>>>>>>>>>>
>>>>>>>>>>> The reservation of memory is split up to a few steps. When 
>>>>>>>>>>> we want a
>>>>>>>>>>> chunk of memory with large pages we first just reserve some 
>>>>>>>>>>> memory
>>>>>>>>>>> large
>>>>>>>>>>> enough for what we need. Then we realize that we want large 
>>>>>>>>>>> pages,
>>>>>>>>>>> so we
>>>>>>>>>>> want to re-map the reserved memory to use large pages. But 
>>>>>>>>>>> since this
>>>>>>>>>>> requires that we have a large-page-aligned memory chunk we may
>>>>>>>>>>> have to
>>>>>>>>>>> fix the recently reserved memory chunk up a bit.
>>>>>>>>>>>
>>>>>>>>>>> We do this in ReservedSpace::initialize() by first releasing 
>>>>>>>>>>> the
>>>>>>>>>>> memory
>>>>>>>>>>> we just reserved. Then requesting more memory than we actually
>>>>>>>>>>> need to
>>>>>>>>>>> make sure that we have enough space to do manual large page
>>>>>>>>>>> alignment.
>>>>>>>>>>> After we have gotten this memory we figure out a nicely 
>>>>>>>>>>> aligned start
>>>>>>>>>>> address. We then release the memory again and then reserve just
>>>>>>>>>>> enough
>>>>>>>>>>> memory but using the aligned start address as a fixed 
>>>>>>>>>>> address to make
>>>>>>>>>>> sure that we get the memory we wanted in an aligned way.
>>>>>>>>>>>
>>>>>>>>>>> This is done in a loop to make sure that we eventually get some
>>>>>>>>>>> memory.
>>>>>>>>>>> The interesting code looks like this:
>>>>>>>>>>>
>>>>>>>>>>> do {
>>>>>>>>>>> char* extra_base = os::reserve_memory(extra_size, NULL, 
>>>>>>>>>>> alignment);
>>>>>>>>>>> if (extra_base == NULL) return;
>>>>>>>>>>> // Do manual alignement
>>>>>>>>>>> base = (char*) align_size_up((uintptr_t) extra_base, 
>>>>>>>>>>> alignment);
>>>>>>>>>>> assert(base >= extra_base, "just checking");
>>>>>>>>>>> // Re-reserve the region at the aligned base address.
>>>>>>>>>>> os::release_memory(extra_base, extra_size); // (1)
>>>>>>>>>>> base = os::reserve_memory(size, base); // (2)
>>>>>>>>>>> } while (base == NULL);
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> There is a race here between releasing the memory in (1) and
>>>>>>>>>>> re-reserving it in (2). But the loop is supposed to handle 
>>>>>>>>>>> this race.
>>>>>>>>>>>
>>>>>>>>>>> The problem is that on posix platforms you can remap the 
>>>>>>>>>>> same memory
>>>>>>>>>>> area several times. The call in (2) will use mmap with 
>>>>>>>>>>> MAP_FIXED.
>>>>>>>>>>> This
>>>>>>>>>>> means that the OS will think that you know exactly what you are
>>>>>>>>>>> doing.
>>>>>>>>>>> So, if part of the memory has been mapped already by the 
>>>>>>>>>>> process it
>>>>>>>>>>> will
>>>>>>>>>>> just go ahead and reuse that memory.
>>>>>>>>>>>
>>>>>>>>>>> This means that if we are having multiple threads that do 
>>>>>>>>>>> mmap. We
>>>>>>>>>>> can
>>>>>>>>>>> end up with a situation where we release our mapping in (1). 
>>>>>>>>>>> Then
>>>>>>>>>>> another thread comes in and maps part of the memory that we 
>>>>>>>>>>> used to
>>>>>>>>>>> have. Then we remap over that memory again in (2) with 
>>>>>>>>>>> MAP_FIXED.
>>>>>>>>>>> Now we
>>>>>>>>>>> have a situation where two threads in our process have 
>>>>>>>>>>> mapped the
>>>>>>>>>>> same
>>>>>>>>>>> memory. If both threads try to use it or if one of the 
>>>>>>>>>>> threads unmap
>>>>>>>>>>> part or all of the memory we will crash.
>>>>>>>>>>>
>>>>>>>>>>> On posix it is possible to unmap any part of a mapped chunk. 
>>>>>>>>>>> So, our
>>>>>>>>>>> proposed solution to the race described above is to not 
>>>>>>>>>>> unmap all
>>>>>>>>>>> memory
>>>>>>>>>>> in (1) but rather just unmap the section at the start and at 
>>>>>>>>>>> the
>>>>>>>>>>> end of
>>>>>>>>>>> the chunk that we mapped to get alignment. This also removes 
>>>>>>>>>>> the need
>>>>>>>>>>> for the loop.
>>>>>>>>>>>
>>>>>>>>>>> However, on Windows you can only unmap _all_ of the memory 
>>>>>>>>>>> that you
>>>>>>>>>>> have
>>>>>>>>>>> mapped. On the other hand Windows also will not allow you to 
>>>>>>>>>>> map over
>>>>>>>>>>> other mappings, so the original code is actually safe. If we 
>>>>>>>>>>> keep
>>>>>>>>>>> the loop.
>>>>>>>>>>>
>>>>>>>>>>> So, our solution is to treat this differently on Windows and 
>>>>>>>>>>> posix
>>>>>>>>>>> platforms.
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Thanks,
>>>>>>>>>>> Bengt
>>>>>>>>>
>>>>>>>>
>>>>>>
>>>>
>>>
>>
>