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

[Thomas Schatzl]

Hi,

On Fri, 2018-11-30 at 11:11 +0100, Thomas Schatzl wrote:
> Hi Amit,
> 
> [...]
> > 
> > Sorry. Please check the attached patch. 
> 
> I created a webrev at 
> http://cr.openjdk.java.net/~tschatzl/8213827/webrev/ .
> 
> Let the reviews begin :)
> 

style issues:

  - random whitespaces in many places.
  - all if's or if-then-else require braces
  - only class members have a leading underscore
  - do not declare local variables long before they are used. That
makes the code hard to read.
  - do not introduce local variables for no reason. E.g.

2907         struct bitmask *bmp;

[...unrelated code...]

2911         bmp = _numa_get_interleave_mask();
2912         set_numa_interleave_ptr(&bmp);
2913         bmp = _numa_get_membind();
2914         set_numa_membind_ptr(&bmp);

is much less readable than

set_numa_interleave_ptr(_numa_get_interleave_mask());
set_numa_membind_ptr(_numa_get_membind_mask());

  - in the type declaration, please put the * next to the type. While
this is kind-of wrong, the majority of existing code does so (not only
in these files). 

code issues:

  - interleave/membind mode are mutually exclusive, which the code
kind-of tries to establish (and check) using special values for eg. the
value of Linux::_numa_interleave_ptr.

I think the code gets a lot more readable if you introduce an enum and
an explicit global variable holding the current NUMA node. If you do
not want to do that, move the condition "Linux::_numa_interleave_ptr !=
0" into a new method.

 - move the new NUMA initialization code in os::init_2() into a new
method please. It's gotten very long now.

 - please avoid duplicating code; ie. 

5039       // Check for membind mode.
5040       bmp = Linux::_numa_membind_ptr;
5041       for (int node = 0; node <= Linux::numa_max_node() ; node++)
{
5042         if (Linux::_numa_bitmask_isbitset(bmp, node)) {
5043           _is_membind = true;
5044         }
5045       }

5048       bmp = Linux::_numa_interleave_ptr;
5049       for (int node = 0; node <= Linux::numa_max_node() ; node++)
{
5050         if (Linux::_numa_bitmask_isbitset(bmp, node)) {
5051           _is_interleaved = true;
5052           // Set membind to false as interleave mode allows all
nodes to be used.
5053           _is_membind = false;
5054         }
5055       }

and right after:

Something like:

is_interleave = is_any_bit_in_bitmask_set(Linux::_numa_interleave_ptr);
is_membind = !is_interleave &&
is_any_bit_in_bitmask_set(Linux::_numa_membind_ptr);

with an appropriate is_any_bit_in_bitmask_set() helper method is much
more clear.

(Even better, as suggested above use a global "NUMA mode" enum that is
filled in somewhere instead of multiple flags)

  - Use a LogStream to print the log messages instead of rolling your
own code to create a string containing the node number. The code that
is there exhibits a potential buffer overflow (with many NUMA nodes),
and a potential  out-of-bounds write. 

  - the log message for selected NUMA mode and nodes should be a single
log message.

  - partially pre-existing: the numa_set_xxx_ptr methods take a
"struct bitmask**". Could you explain why "**"? All of those store the
dereferenced result anyway, so why not pass the dereferenced pointer
right away?

This also simplifies the code at the callers.

  - partially pre-existing: I would prefer if the numa_set_xx_ptr
methods were all renamed to numa_set_xx_bitmask. The "ptr" suffix
seems redundant, and most importantly very unspecific.

  - in numa_interleave_memory(), why does the code not make the
membind/interleave mode distinction when the v1 API is used?

That method could probably cleaned up a bit, i.e. extracting common
code.

  - Pre-existing: numa_interleave_memory() could be cleaned up; why
do we have two pointers to _numa_interleave_memory_v2 and
_numa_interleave_memory around if their parameters are the same and
we only ever use one of them?

That might be an additional cleanup that needs to be done.

  - please split out the change that disables UseNUMA if there is
a single node only. It seems relatively unrelated, and so should imho
be looked at (evaluated) separately.

While reviewing I already fixed a few of the above issues, see

http://cr.openjdk.java.net/~tschatzl/8213827/webrev.0_to_1 (diff)
http://cr.openjdk.java.net/~tschatzl/8213827/webrev.1 (full)

webrevs. You can use it as starting point.

Thanks,
  Thomas