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

[Per Liden]

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.
>>
>