[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-hotspot-runtime-dev
Subject: Request for review (s): 7173959 : Jvm crashed during coherence exabus (tmb) testing
From: bengt.rutisson () oracle ! com (Bengt Rutisson)
Date: 2012-12-18 20:29:59
Message-ID: 50D0D247.6010608 () oracle ! com
[Download RAW message or body]
Hi all,
One final update on this. I pushed this change both in to the hotspot-gc
repository and to the hs24 repository:
http://hg.openjdk.java.net/hsx/hotspot-gc/hotspot/rev/730cc4ddd550
http://hg.openjdk.java.net/hsx/hsx24/hotspot/rev/e1d9b04b560b
But I forgot to add "Contributed-by" for Mikael Gerdin ( mgerdin). That
should definitely have been the case since he deserves a large part of
the credit for this fix.
Hopefully this email message will be of some help in recording his work
for this fix for future reference.
Bengt
On 12/17/12 8:36 AM, Bengt Rutisson wrote:
>
> 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
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/attachments/20121218/048acdf3/attachment-0001.html \
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic