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 Thu, 2018-11-29 at 16:38 +0530, amith pawar wrote:
> Hi Thomas,
> 
> Sorry for the late reply and please check my inline comments below.
> 
> On Thu, Nov 22, 2018 at 5:18 PM Thomas Schatzl <
> thomas.schatzl at oracle.com> wrote:
> > Hi Amith,
> > 
> >   welcome to the OpenJDK community! :)
> 
> Thanks 
> >   
> > On Fri, 2018-11-09 at 17:53 +0530, amith pawar wrote:
> > > Hi all,
> > > 
> > >[...]
> >
> > Is it possible to summarize the problem and the changes with the
> > following few lines:
> > [...]
> 
> This is also OK. 

I updated JDK-8213827.

> > - there have been other NUMA related patches from AMD recently, in
> > particular JDK-what is the relation to JDK-8205051? The text there
> > reads awfully similar to this one, but I *think* JDK-8205051 is
> > actually about making sure that the parallel gc eden is not put on
> > inactive nodes.
> > Can you confirm this (talk to your colleague) so that we can fix
> > the description too?
> 
> JDK-8205051 is related to early GC.
> JDK-8189922 specific to membind case where heap was allocated on non
> requested NUMA nodes.
> The proposed patch fixes two following issues
> 1. Similar to JDK-8189922 but for interleave case.
> 2. OLDGen, S1 and S2 memory interleaving is incorrect when run on
> fewer NUMA nodes.

Okay.

> > - fyi, we are currently working on NUMA aware memory allocation
> > support for G1 in JEP 345 (JDK-8210473). It may be useful to sync
> > up a bit to not step on each other's toes (see below).
> 
> We are not working anything related to G1. It may effect G1 if
> numa_make_global function is called.

That's why we need to be a bit careful here.

> > [... broken patch...]
> > 
> > I tried to apply the patch to the jdk/jdk tree, which failed; I
> > then started to manually apply it but stopped after not being able
> > to find the context of some hunks. I do not think this change
> > applies to the latest source tree.
> > 
> > Please make sure that the patch applies to the latest jdk/jdk tree
> > with errors. All changes generally must first go into the latest
> > dev tree before you can apply for backporting.
> > 
> > Could you please send the patch as attachment (not copy&pasted) to
> > this list and cc me? Then I can create a webrev out of it.
> > 
> > I did look a bit over the patch as much as I could (it's very hard
> > trying to review a diff), some comments:
> 
> Sorry. Please check the attached patch. 

I created a webrev at 
http://cr.openjdk.java.net/~tschatzl/8213827/webrev/ .

Let the reviews begin :)

> >   - the _numa_interleave_mmory_v2 function pointer is never
> > assigned to in the patch in the CR, so it will not be used. Please
> > make sure the patch is complete.
> > Actually it is never defined anywhere, ie. the patch unlikely
> > actually compiles even if I could apply it somewhere.
> > 
> > Please avoid frustrating reviewers by sending incomplete patches.
> 
> Sorry again. Faced same issue when copied from the defect link but
> worked for me from the mail. Now i have attached the patch. Can you
> please update defect link with this patch ?

This one applies correctly.

I will provide review comments in a separate email.

Thanks,
  Thomas