<html>
<head>
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<font size="-1"><font face="Verdana">Looks good.<br>
<br>
<br>
</font></font>
<pre class="moz-signature" cols="72">Azeem Jiva
@javawithjiva</pre>
<br>
On 03/23/2012 07:56 AM, Bengt Rutisson wrote:
<blockquote cite="mid:4F6C72EB.9070906@oracle.com" type="cite">
<br>
Hi all,
<br>
<br>
Could I have a couple of reviews for this change? This is on the
critical-watch list for hs23, so it would be great if I could get
some reviews already today.
<br>
<br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~brutisso/7103665/webrev.00/">http://cr.openjdk.java.net/~brutisso/7103665/webrev.00/</a>
<br>
<br>
Some background:
<br>
<br>
There are two problems that this fix solves:
<br>
<br>
(1) When we run with -XX:+UseNUMA we have to make the heap
parsable since we leave some holes in the heap. We do this by
faking Integer arrays in those places. However, on a large heap
those holes can be larger than what can be covered by a single
Integer array. The fix is to allocate several arrays in those
cases.
<br>
<br>
The easy fix would have been to call
CollectedHeap::fill_with_objects() instead of
CollectedHeap::fill_with_object(). Note the extra "s" in the
method name. But MutableNUMASpace::ensure_parsability(), where the
change is needed, need to know about all the objects that are
allocated so I saw no better solution than to implement my own
loop. Any ideas on how I could re-use fill_with_objects() instead
are welcome.
<br>
<br>
(2) CollectedHeap::_filler_array_max_size should be the maximum
size that can be covered by a fake Integer array. This value is in
words, but since the word size is different on different platforms
but the Integer size is always the same we need to convert between
word and sizeof(jint) at some point. Unfortunately we do the
conversion in the wrong direction, which means that on 64 bit
platforms we end up with a max size that is 4 times larger than
intended. This in turn makes us overflow an int when we convert
from a word size to the length of the array that we are setting
up.
<br>
<br>
Here is the code that overflows:
<br>
<br>
328 void
<br>
329 CollectedHeap::fill_with_array(HeapWord* start, size_t words,
bool zap)
<br>
330 {
<br>
331 assert(words >= filler_array_min_size(), "too small for
an array");
<br>
332 assert(words <= filler_array_max_size(), "too big for a
single object");
<br>
333
<br>
334 const size_t payload_size = words -
filler_array_hdr_size();
<br>
335 const size_t len = payload_size * HeapWordSize /
sizeof(jint);
<br>
336
<br>
337 // Set the length first for concurrent GC.
<br>
338 ((arrayOop)start)->set_length((int)len);
<br>
339 post_allocation_setup_common(Universe::intArrayKlassObj(),
start, words);
<br>
340 DEBUG_ONLY(zap_filler_array(start, words, zap);)
<br>
341 }
<br>
<br>
If filler_array_max_size() on line 332, which returns
CollectedHeap::_filler_array_max_size, has a too large value we
will overflow the int conversation on line 338.
<br>
<br>
Testing:
<br>
This fix solves the issue that was found in the reproducer that I
could set up on a Linux x64 machine. I have asked the one who
initially reported the issue to run on their Solaris amd64 setup.
I will also try to set up a reproducer on a Solaris machine.
<br>
<br>
I also ran the fix through JPRT. It did not fail, but there are no
NUMA tests in there as far as I know. But the change for issue (2)
was hopefully tested.
<br>
<br>
Thanks,
<br>
Bengt
<br>
</blockquote>
</body>
</html>