RFR (S/M): 8213827: NUMA heap allocation does not respect process membind/interleave settings [Was: Re: [PATCH] JDK NUMA Interleaving issue]

[sangheon.kim at oracle.com]

Hi Amith,

On 1/9/19 3:30 AM, amith pawar wrote:
> Hi Sangheon,
>
> Thanks for reviewing and updated with suggested changes. please check.
Thank you for addressing my comments.
But I can't see below comments addressed:
>> - Looking at 'enum' at os.hpp, we use Camel style.
I meant to change from 'Numa_allocation_policy' to 'NumaAllocationPolicy'.
>> - As usual, copyright year updates. I know it was correct when you 
>> posted. :)
Looking at the latest source code, only os_linux.hpp needs a new 
copyright year.
- * Copyright (c) 1999, 2018, Oracle and/or its affiliates. All rights 
reserved.
+ * Copyright (c) 1999, 2019, Oracle and/or its affiliates. All rights 
reserved.

Looking at the v5,
+      ls.print("UseNUMA is enabled and invoked in '%s' mode."
+                " Heap will be configured using NUMA memory nodes:", 
numa_mode);
There is one more space before " Heap.... ", please remove it.

I see the latest version that Thomas posted is v3, but your attached 
version is v5. :)

In addition, it would be better to provide webrev instead of a patch. ( 
http://openjdk.java.net/guide/codeReview.html )

Thanks,
Sangheon

>
> Thanks,
> Amit Pawar
>
> On Wed, Jan 9, 2019 at 12:45 AM <sangheon.kim at oracle.com 
> <mailto:sangheon.kim at oracle.com>> wrote:
>
>     Hi Thomas,
>
>     On 12/13/18 2:33 AM, Thomas Schatzl wrote:
>>     Hi Amit,
>>     On Thu, 2018-12-13 at 15:11 +0530, amith pawar wrote:
>>>     Hi Thomas,
>>>
>>>     Please find the attached patch updated as per your suggestion.
>>>     If everything OK then can you please commit this to repo ?
>>        looks good. We will need a second reviewer though, I am going to ask
>>     around.
>>
>>     Latest webrev:
>>     http://cr.openjdk.java.net/~tschatzl/8213827/webrev.3/
>>     <http://cr.openjdk.java.net/%7Etschatzl/8213827/webrev.3/>
>     Webrev.3 looks good to me.
>
>     I have some minor nits:
>     ----------------------------------------
>     src/hotspot/os/linux/os_linux.cpp
>     5012       for (int node = 0; node < Linux::numa_max_node(); node++) {
>     - Looks like 'node <*_=_* Linux::numa_max_node()' is the right one
>     to print the latest node?
>
>     ----------------------------------------
>     src/hotspot/os/linux/os_linux.hpp
>      271   enum Numa_allocation_policy{
>     - Looking at 'enum' at os.hpp, we use Camel style.
>     - There are missing space before '{'.
>
>     - As usual, copyright year updates. I know it was correct when you
>     posted. :)
>
>     Thanks,
>     Sangheon
>
>
>>     Thanks,
>>        Thomas
>>
>>
>
>
>
> -- 
> With best regards,
> amit pawar