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

[sangheon.kim at oracle.com]

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.

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