<html>
<head>
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<tt>Hi Sangheon,</tt><br>
<br>
<div class="moz-cite-prefix">On 2016-08-30 02:08, sangheon wrote:<br>
</div>
<blockquote
cite="mid:b38ddcb3-df81-fc4e-0c5e-c418c11e8576@oracle.com"
type="cite">
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
Hi Jon,<br>
<br>
Thanks for reviewing this.<br>
<br>
<div class="moz-cite-prefix">On 08/29/2016 02:55 PM, Jon Masamitsu
wrote:<br>
</div>
<blockquote
cite="mid:04c8ed54-0780-64fa-38bb-50c8af0210b4@oracle.com"
type="cite">
<meta content="text/html; charset=utf-8"
http-equiv="Content-Type">
<br>
<br>
<div class="moz-cite-prefix">On 08/29/2016 01:31 AM, Stefan
Johansson wrote:<br>
</div>
<blockquote
cite="mid:f81c0a6f-a242-3250-1819-b6a315b4b3f2@oracle.com"
type="cite">...<br>
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite"> <br>
<blockquote type="cite"> <br>
Another thing is the use of #ifdef to make this
conditional for Linux. Is this needed? Isn't the
return value for can_commit_large_page_memory()
the conditional we should care about? </blockquote>
We can know whether we are using 'pin region' from
the return value of can_commit_large_page_memory()
and UseLargePages flag. <br>
<br>
However the condition of comparison with page size
in Linux version of os::pd_free_memory() makes this
problem. For other platforms such as Windows, AIX
and BSD which have empty os::pd_free_memory(), it
doesn't matter. And Solaris doesn't have such
condition. This means for other platforms we don't
need to exit because of reverting to default page
size. <br>
<br>
I mean if can_commit_large_page_memory() returns
false and UseLargePages enabled, we will try to use
pin region. But NUMA could try to use default page
size. It is okay in general except on Linux because
of above reason. <br>
<br>
<blockquote type="cite">Or will we fail some
platform too early. If so, we could add another
capability method to the os class and use that to
avoid having the #ifdef in the code. <br>
</blockquote>
I also considered shortly to add a new method to
decide. <br>
And I agree that not using #ifdef is better in
general. But I'm not sure for this case as it is too
Linux implementation specific. i.e. Linux version is
implemented pd_free_memory() to conditionally commit
after comparing with page size. If Linux
pd_free_memory() becomes blank or the condition is
changed, the decision method also should be changed
which seems not worth for me. This is why I stopped
considering it. <br>
<br>
</blockquote>
Ok, I don't see things changing as a big problem, you
could let the new capability just return the same as
can_commit_large_page_memory() for Linux and have the
other platforms return true. This would have the same
maintenance requirements as the current solution in my
eyes. <br>
</blockquote>
I think we have to consider not only the maintenance but
also the necessity of the method. I don't think we could
re-use it and this is why I described it as too Linux
implementation specific. <br>
<br>
If you strongly want to introduce a new method, may I
ask what is expected name/roll of the method? <br>
<br>
</blockquote>
I see your point and I'll leave it up to a second reviewer
to decide which way to go. <br>
</blockquote>
OK. <br>
</blockquote>
</blockquote>
<br>
Stefan and Sangheon,<br>
<br>
I agree that having the #ifdef is unfortunate but I'm really
hesitant about<br>
adding an new API at the os:: level. We could give it a good
name but it<br>
seems like it would just be there for GC.<br>
<br>
A little better I think would be to add a field to
MuntableNUMASpace<br>
<br>
bool _must_use_large_pages;<br>
<br>
and add the #ifdef in MutableNUMASpace::MutableNUMASpace() :
..., _must_use_large_pages(false) ...<br>
<br>
<pre><span class="new"> 582 #ifdef LINUX</span>
<span class="new"> 583 // Changing the page size below can lead to freeing of memory. When using large pages</span>
<span class="new"> 584 // and the memory has been both reserved and committed, Linux does not support</span>
<span class="new"> 585 // freeing parts of it. So we fail initialization.</span>
<span class="new"> 586 if (UseLargePages && !os::can_commit_large_page_memory()) {</span>
<span class="new"> 587 _must_use_large_pages = true;</span>
<span class="new"> 588 }</span>
<span class="new"> 589 #endif // LINUX</span></pre>
There is not much code in the constructor and adding the #ifdef
is less<br>
distracting.<br>
<br>
Then the code in initialize() could be<br>
<pre> 580 if (base_space_size_pages / lgrp_spaces()->length() == 0
581 && page_size() > (size_t)os::vm_page_size()) {
if (_must_use_large_pages) {
<span class="new"> 587 vm_exit_during_initialization("Failed initializing NUMA with large pages. Too small heap size");
}
</span></pre>
</blockquote>
I think this would give better readability on 'initialize' method.<br>
<br>
Webrev:<br>
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Esangheki/8023905/webrev.2">http://cr.openjdk.java.net/~sangheki/8023905/webrev.2</a><br>
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Esangheki/8023905/webrev.2_to_1">http://cr.openjdk.java.net/~sangheki/8023905/webrev.2_to_1</a><br>
<br>
Stefan, what do you think?<br>
webrev.2 seems better for me.<br>
<br>
</blockquote>
This looks better to me as well, but the indentation in the
constructor seems to be a bit off. Please fix this before pushing. <br>
<br>
Thanks,<br>
Stefan<br>
<br>
<blockquote
cite="mid:b38ddcb3-df81-fc4e-0c5e-c418c11e8576@oracle.com"
type="cite"> Thanks,<br>
Sangheon<br>
<br>
<br>
<blockquote
cite="mid:04c8ed54-0780-64fa-38bb-50c8af0210b4@oracle.com"
type="cite">
<pre><span class="new">
</span><span class="new"></span>
</pre>
If you don't care for that suggestion, the patch as is looks
fine.<br>
<br>
Jon<br>
<pre><span class="new"></span></pre>
<br>
</blockquote>
<br>
</blockquote>
<br>
</body>
</html>