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

[Bengt Rutisson]

Hi Coleen,

Thanks for suggesting the comments. I added them to the os files.


David, Coleen, Zhengyu and Vitaly,

Thanks for the reviews! All set to push this now.

Bengt


On 12/14/12 4:49 PM, Coleen Phillimore wrote:
> 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
>>>>>>>>>>
>>>>>>>>>
>>>>>>>
>>>>>
>>>>
>>>
>>
>