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

[Thomas Schatzl]

Hi Amit,

On Sat, 2018-12-08 at 04:50 +0530, amith pawar wrote:
> Hi Thomas,
> 
> Please check my inline comments and attached patch is updated as per
> your suggestion.

  thanks a lot! Looks very good now.

Fixed issues:

- you can use the LogStream directly for writing, you do not need to
create an outputStream from it.

- "isrunning_..." should be renamed "is_running_..."

There are still some "is" prefixes glued to the predicate left.

Here are webrevs:

http://cr.openjdk.java.net/~tschatzl/8213827/webrev.3/ (latest webrev)
http://cr.openjdk.java.net/~tschatzl/8213827/webrev.2_to_3/ (above
suggested changes)
http://cr.openjdk.java.net/~tschatzl/8213827/webrev.2/ (original
changes)

Some comments further down.

> 
> On Mon, Dec 3, 2018 at 6:12 PM Thomas Schatzl <
> thomas.schatzl at oracle.com> wrote:
> > 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?
> 
> Following bitmask pointers are defined in libnuma library. 'dlvsym'
> will return the address of pointer which needs to be de-referenced.
> Updated other methods as suggested.
> struct bitmask *numa_all_nodes_ptr;
> struct bitmask *numa_no_nodes_ptr;
> struct bitmask *numa_all_cpus_ptr; 
> > 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.
> 
> Newly introduced membind and interleave bitmask pointer names are
> changed. Also above mentioned three pointers are imported. If you
> suggest will rename these also.

If you want we can do this in a separate changeset. Otherwise, please
do right away.
Please also fix the glued "is" in the isnode_in_bound_nodes,
isbound_to_single_node, isnode_in_existing_nodes,
isnode_in_configured_nodes methods.

> >   - in numa_interleave_memory(), why does the code not make the
> > membind/interleave mode distinction when the v1 API is used?
> 
> v2 API's works as expected for membind/interleave case but
> numa_get_membind_v1 API's always returns null . v1 and v2 prototypes
> are also not same. Please check below.
> nodemask_t numa_get_membind_v1(void)
> struct bitmask * numa_get_membind_v2(void)
> 
> void numa_interleave_memory_v1(void *mem, size_t size, const
> nodemask_t *mask)
> void numa_interleave_memory_v2(void *mem, size_t size, struct bitmask
> *bmp)

Okay, understood. Thanks!

Thanks,
  Thomas