<html>
<head>
<meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<div class="moz-cite-prefix"><br>
Hi Thomas,<br>
<br>
Thanks for fixing this.<br>
<br>
According to the MSDN documentation for VirtualAlloc the address
parameter can be rounded down if it is not properly aligned:<br>
<a class="moz-txt-link-freetext" href="http://msdn.microsoft.com/en-us/library/windows/desktop/aa366887%28v=vs.85%29.aspx">http://msdn.microsoft.com/en-us/library/windows/desktop/aa366887%28v=vs.85%29.aspx</a><br>
<br>
Does that mean that we in os::reserve_memory_special() should
assert that addr is large page aligned? If it is not we will
probably get a different return address than we asked for anyway.
I had a quick look through the call chain. I am not sure it is
always large page aligned. Do we need to align it in
ReservedSpace::initialize() or should we just assert that it is
aligned? Stefan's large page changes enforces much more alignment,
so maybe it is worth waiting with this patch until he has pushed
his changes?<br>
<br>
Some minor comments:<br>
<br>
The webrev looks a bit odd here. Is there a format issue in the
patch or is it just the webrev?<br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~tschatzl/8016825/webrev/src/os/posix/vm/os_posix.cpp.frames.html">http://cr.openjdk.java.net/~tschatzl/8016825/webrev/src/os/posix/vm/os_posix.cpp.frames.html</a><br>
<br>
You use Verbose for logging in your test. I think that is fine.
Stefan recently added a new flag in one of his webrevs called
<meta http-equiv="content-type" content="text/html; charset=UTF-8">
VerboseInternalVMTests:<br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~stefank/8007074/webrev.00/src/share/vm/runtime/globals.hpp.udiff.html">http://cr.openjdk.java.net/~stefank/8007074/webrev.00/src/share/vm/runtime/globals.hpp.udiff.html</a><br>
That has not been pushed yet, but maybe it is worth introducing
this flag with your change instead?<br>
<br>
Minor format thing in the test code. You have spaces between the
type and the star. I think we normally don't have that. So for
example:<br>
<br>
char * result = <br>
<br>
should probably be:<br>
<br>
char* result =<br>
<br>
BTW, maybe we can have a better name than result? Maybe
base_address?<br>
<br>
<br>
I don't understand the allocation sizes. Why do we pick 16 large
pages?<br>
<br>
Would it be possible to do it like this instead?<br>
<br>
* First allocate 3 large pages at any address<br>
* Remember the returned address (base)<br>
* Release the memory<br>
* Allocate 2 large pages at (base address + one large page size)<br>
<br>
I think that will reduce the risk of entering this case:<br>
<br>
5627 if (actual_location == NULL) {<br>
5628 if (Verbose) {<br>
5629 gclog_or_tty->print("Failed to allocate any memory
at "PTR_FORMAT" size "SIZE_FORMAT". Skipping remainder of test.",<br>
5630 expected_location, large_allocation_size);<br>
5631 }<br>
5632 }<br>
<br>
I like the fact that you added a test, but as you said it does not
test that much. It does help a bit, so I think we should keep it.
But have you thought about trying to write a JTreg test that can
be run with UseLargePages and then check with the OS that large
pages are actually being used? I think it will be much more work
to write such a test, but I think it would catch more issues.<br>
<br>
Thanks,<br>
Bengt<br>
<br>
<br>
On 7/15/13 11:33 AM, Thomas Schatzl wrote:<br>
</div>
<blockquote cite="mid:1373880816.5052.9.camel@cirrus" type="cite">
<pre wrap="">Hi all,
can I have reviews for the following change?
The change fixes the allocation of memory using large pages on Windows
when a particular address is requested under some circumstances.
In particular, VirtualAlloc is always called with a NULL address,
meaning there is no preferred allocation address, which is not the
desired behavior.
See src/os/windows/vm/os_windows.cpp line 3155 in the original code.
The other part of the changes contain a test case for that.
This test case is a bit convoluted, as it needs to handle a few edge
cases which are valid behaviors (mostly not getting large pages in the
first place) that should not cause a failed test.
There is no check whether the returned memory consists of large pages; the call to VirtualAlloc() with MEM_LARGE_PAGES always returns large page memory on success.
webrev:
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~tschatzl/8016825/webrev/">http://cr.openjdk.java.net/~tschatzl/8016825/webrev/</a>
jbs:
<a class="moz-txt-link-freetext" href="https://jbs.oracle.com/bugs/browse/JDK-8016825">https://jbs.oracle.com/bugs/browse/JDK-8016825</a>
bugs.sun
<a class="moz-txt-link-freetext" href="http://bugs.sun.com/view_bug.do?bug_id=8016825">http://bugs.sun.com/view_bug.do?bug_id=8016825</a>
testing:
jprt running the test case, manual testing
To reproduce the test case, you must give yourself permission to use
large pages (<a class="moz-txt-link-freetext" href="http://msdn.microsoft.com/en-us/library/ms190730.aspx">http://msdn.microsoft.com/en-us/library/ms190730.aspx</a>) and
run the VM in an elevated shell with -XX:+UseLargePages and -XX:
+ExecuteInternalVMTests .
Thanks,
Thomas
</pre>
</blockquote>
<br>
</body>
</html>