RFR(XL): 8220310: Implementation: NUMA-Aware Memory Allocation for G1, Mutator (1/3)

[Per Liden]

Hi Sangheon,

On 10/23/19 6:20 PM, sangheon.kim at oracle.com wrote:
> Hi Per,
> 
> Thanks for taking a look at this.
> 
> I agree all your comments and here's the webrev.
> - All comments from Per.
> - Move G1PageBasedVirtualSpace::page_size() near to page_start() from Kim.
> 
> Webrev:
> http://cr.openjdk.java.net/~sangheki/8220310/webrev.6
> http://cr.openjdk.java.net/~sangheki/8220310/webrev.6.inc
> Testing: build test for linux, solaris, windows and mac.

Thanks for fixing. os changes look good to me.

/Per

> 
> FYI, as I think existing numa related API names and -1 stuff seem not 
> good, I planned to refine those later after pushing. But as you said 
> following existing rule and then refine all together later seems better.
> 
> Thanks,
> Sangheon
> 
> 
> On 10/23/19 1:21 AM, Per Liden wrote:
>> Hi Sangheon,
>>
>> I noticed that this patch adds os::numa_get_address_id(). That name is 
>> misleading as it doesn't return an "address id", but a "numa node id". 
>> However, the terminology used in the os class for numa node is "group" 
>> (for example, numa_get_groups_num, numa_get_group_id, etc). So I'd 
>> suggest we instead name this os::numa_get_group_id(void* address), 
>> i.e. an overload of os::numa_get_group_id().
>>
>> Btw, I think that the numa related names used in the os class are odd, 
>> but I guess that are brought over from Solaris. We can refine those at 
>> some later time if we want, but for now I think we should follow the 
>> naming convention that we have there.
>>
>> Also, I don't think this function should print warnings, as that's up 
>> to the caller to decide what to do, what to print, etc.
>>
>> Furthermore, I suggest we remove os::InvalidNUMAId. Other numa 
>> functions in the os class returns -1 on error, so I think we should do 
>> that here too.
>>
>> Here's a patch with the proposed changes:
>>
>>
>> diff --git a/src/hotspot/os/linux/os_linux.cpp 
>> b/src/hotspot/os/linux/os_linux.cpp
>> --- a/src/hotspot/os/linux/os_linux.cpp
>> +++ b/src/hotspot/os/linux/os_linux.cpp
>> @@ -3007,7 +3007,7 @@
>>    return 0;
>>  }
>>
>> -int os::numa_get_address_id(void* address) {
>> +int os::numa_get_group_id(void* address) {
>>  #ifndef MPOL_F_NODE
>>  #define MPOL_F_NODE     (1<<0)  // Return next IL mode instead of 
>> node mask
>>  #endif
>> @@ -3016,11 +3016,10 @@
>>  #define MPOL_F_ADDR     (1<<1)  // Look up VMA using address
>>  #endif
>>
>> -  int id = InvalidNUMAId;
>> +  int id = 0;
>>
>>    if (syscall(SYS_get_mempolicy, &id, NULL, 0, address, MPOL_F_NODE | 
>> MPOL_F_ADDR) == -1) {
>> -    warning("Failed to get numa id at " PTR_FORMAT " with errno=%d", 
>> p2i(address), errno);
>> -    return InvalidNUMAId;
>> +    return -1;
>>    }
>>    return id;
>>  }
>> diff --git a/src/hotspot/share/gc/g1/g1NUMA.cpp 
>> b/src/hotspot/share/gc/g1/g1NUMA.cpp
>> --- a/src/hotspot/share/gc/g1/g1NUMA.cpp
>> +++ b/src/hotspot/share/gc/g1/g1NUMA.cpp
>> @@ -164,7 +164,7 @@
>>
>>  uint G1NUMA::index_of_address(HeapWord *address) const {
>>    int numa_id = os::numa_get_address_id((void*)address);
>> -  if (numa_id == os::InvalidNUMAId) {
>> +  if (numa_id == -1) {
>>      return UnknownNodeIndex;
>>    } else {
>>      return index_of_node_id(numa_id);
>> @@ -201,7 +201,7 @@
>>    if (!is_enabled()) {
>>      return;
>>    }
>> -
>> +
>>    if (size_in_bytes == 0) {
>>      return;
>>    }
>> diff --git a/src/hotspot/share/runtime/os.hpp 
>> b/src/hotspot/share/runtime/os.hpp
>> --- a/src/hotspot/share/runtime/os.hpp
>> +++ b/src/hotspot/share/runtime/os.hpp
>> @@ -374,10 +374,7 @@
>>    static size_t numa_get_leaf_groups(int *ids, size_t size);
>>    static bool   numa_topology_changed();
>>    static int    numa_get_group_id();
>> -
>> -  static const int InvalidNUMAId = -1;
>> -
>> -  static int numa_get_address_id(void* address);
>> +  static int    numa_get_group_id(void* address);
>>
>>    // Page manipulation
>>    struct page_info {
>>
>>
>> cheers,
>> Per
>>
>>
>> On 10/16/19 7:54 PM, sangheon.kim at oracle.com wrote:
>>> Hi Kim, Stefan and Thomas,
>>>
>>> Many thanks for the reviews and suggestions!
>>>
>>> Kim,
>>> I will move page_size() near page_start() before push as you suggested.
>>> As you know, all 3 patches will be pushed together though.
>>>
>>> Thanks,
>>> Sangheon
>>>
>>>
>>> On 10/16/19 7:00 AM, Kim Barrett wrote:
>>>>> On Oct 15, 2019, at 10:33 AM, sangheon.kim at oracle.com wrote:
>>>>>
>>>>> Hi all,
>>>>>
>>>>> Here's revised webrev which addresses:
>>>>> 1) G1RegionToSpaceMapper checks mtJavaHeap and then conditionally 
>>>>> calls G1NUMA::request_memory_on_node() (Kim)
>>>>> 2) The signature of G1NUMA::request_memory_on_node(void* address, 
>>>>> ,) is changed to have actual address instead of page index. (Stefan)
>>>>> 3) Some local variable name changes at G1RegionToSpaceMapper. i -> 
>>>>> region_idx, idx -> page_idx (for local style, used idx instead of 
>>>>> index)
>>>>>
>>>>> webrev:
>>>>> http://cr.openjdk.java.net/~sangheki/8220310/webrev.5/
>>>>> http://cr.openjdk.java.net/~sangheki/8220310/webrev.5.inc/
>>>>> Testing: hs-tier 1 ~ 5, with/without UseNUMA
>>>> Looks good.
>>>>
>>>> In g1PageBasedVirtualSpace.cpp, could the newly added definition of 
>>>> page_size()
>>>> be moved to be near the existing definition of page_start()? I don’t 
>>>> need a new
>>>> webrev if you move it.
>>>>
>>>
>