<html>
<head>
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
Hi Stefan,<br>
<br>
<div class="moz-cite-prefix">On 08/30/2016 02:33 AM, Stefan
Johansson wrote:<br>
</div>
<blockquote
cite="mid:4537fb7c-f8ef-e57c-edc6-4e85fc87c3f1@oracle.com"
type="cite">
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
<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>
</blockquote>
Okay, I will fix before pushing it.<br>
<br>
Thanks,<br>
Sangheon<br>
<br>
<br>
<blockquote
cite="mid:4537fb7c-f8ef-e57c-edc6-4e85fc87c3f1@oracle.com"
type="cite"> <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>
</blockquote>
<br>
</body>
</html>