Re: [Qemu-devel] [RFC PATCH v0] numa: API to lookup NUMA node by address

[Igor Mammedov]

On Tue, 9 Jun 2015 09:40:54 -0300
Eduardo Habkost <address@hidden> wrote:

> On Tue, Jun 09, 2015 at 11:23:19AM +0200, Igor Mammedov wrote:
> > On Mon, 8 Jun 2015 12:51:39 -0300
> > Eduardo Habkost <address@hidden> wrote:
> > 
> > > On Mon, Jun 08, 2015 at 11:51:03AM +0200, Igor Mammedov wrote:
> > > > On Mon, 8 Jun 2015 11:28:18 +0530
> > > > Bharata B Rao <address@hidden> wrote:
> > > > 
> > > > > On Mon, May 25, 2015 at 02:42:40PM -0300, Eduardo Habkost wrote:
> > > > > > On Mon, May 25, 2015 at 01:17:57PM +0530, Bharata B Rao wrote:
> > > > > > > On Thu, May 14, 2015 at 11:39:06AM +0200, Paolo Bonzini wrote:
> > > > > > > > On 13/05/2015 20:06, Eduardo Habkost wrote:
> > > > > > > > > Also, this introduces a circular dependency between pc-dimm.c 
> > > > > > > > > and
> > > > > > > > > numa.c. Instead of that, pc-dimm could simply notify us when 
> > > > > > > > > a new
> > > > > > > > > device is realized (with just (addr, end, node) as 
> > > > > > > > > arguments), so we can
> > > > > > > > > save the list of memory ranges inside struct node_info.
> > > > > > > > > 
> > > > > > > > > I wonder if the memory API already provides something that 
> > > > > > > > > would help
> > > > > > > > > us. Paolo, do you see a way we could simply use a 
> > > > > > > > > MemoryRegion as input
> > > > > > > > > to lookup the NUMA node?
> > > > > > > > 
> > > > > > > > No, but I guess you could add a 
> > > > > > > > numa_get/set_memory_region_node_id API
> > > > > > > > that uses a hash table.  That's a variant of the "pc-dimm could 
> > > > > > > > simply
> > > > > > > > notify" numa.c that you propose above.
> > > > > > > 
> > > > > > > While you say we can't use MemoryRegion as input to lookup the 
> > > > > > > NUMA node,
> > > > > > > you suggest that we add numa_get/set_memory_region_node_id. Does 
> > > > > > > this API
> > > > > > > get/set NUMA node id for the given MemoryRegion ? 
> > > > > > 
> > > > > > I was going to suggest that, but it would require changing the
> > > > > > non-memdev code path to create a MemoryRegion for each node, too. So
> > > > > > having a numa_set_mem_node_id(start_addr, end_addr, node_id) API 
> > > > > > would
> > > > > > be simpler.
> > > > > 
> > > > > In order to save the list of memory ranges inside node_info, I tried 
> > > > > this
> > > > > approach where I call
> > > > > 
> > > > > numa_set_mem_node_id(dimm.addr, dimm.size, dimm.node) from
> > > > > 
> > > > > pc_dimm_realize(), but
> > > > > 
> > > > > the value of dimm.addr is finalized only later in ->plug().
> > > > > 
> > > > > So we would have to call this API from arch code like pc_dimm_plug().
> > > > > Is that acceptable ?
> > > 
> > > It looks acceptable to me, as pc.c already has all the rest of the
> > > NUMA-specific code for PC. I believe it would be interesting to keep all
> > > numa.o dependencies contained inside machine code.
> > > 
> > > > Could you query pc_dimms' numa property each time you need mapping
> > > > instead of additionally storing that mapping elsewhere?
> > > 
> > > The original patch did that, but I suggested the
> > > numa_set_mem_node_id() API for two reasons: 1) not requiring special
> > > cases for hotplug inside numa_get_node(); 2) not introducing a circular
> > > dependency between pc-dimm.c and numa.c.
> > What circular dependency doing foreach(pc-dimm) would introduce?
> > So far pc-dimm is independent from numa.c and a regular device with no
> > dependencies (except of on backend memdev) and has it's own 'numa' property
> > for providing that information to interested users. I'd rather keep
> > it separate form legacy numa.c:-numa handling.
> 
> pc-dimm.c already depends on numa.c because it checks nb_numa_nodes
> inside pc_dimm_realize().
check should be in pc_dimm_plug() instead of realize but I guess it saves
up duplication when pc-dimm reused with other targets, anyway we could move
it out into generic common function.

> 
> I don't understand what you mean by "legacy numa.c:-numa handling".
> Unless there's a way to query pc-dimm (or other code) for all
> (address -> numa_node) mappings without a special case for memory
> hotplug[1], I wouldn't call node_info[].node_mem "legacy".
> 
> [1] And this is exactly what I want to provide with
>     numa_set_mem_node_id(): an API that doesn't require special cases
>     for memory hotplug.
For x86 board makers usually define address ranges -> node mapping statically.
So node_info[].node_mem  & numa_set_mem_node_id() makes sense.
And sPAPR could probably do the same, no need to scan for pc-dimm devices.

 
[...]
> > What for one needs to pull dimm properties into numa.c?
> 
> I am not sure I parsed the question correctly, but:
> 
> Original commit message explains why numa_get_node(addr) is needed:
> 
> > > This is needed by sPAPR PowerPC to support
> > > ibm,dynamic-reconfiguration-memory device tree node which is needed
> > > for memory hotplug.
> 
> And to make this work, it needs to be aware of NUMA information for
> hotplugged memory too.
I've checked spapr_populate_drconf_memory() from original series,
it needs to be aware at startup about address ranges -> node mapping
including mapping partitioning of whole hotplug memory range
(i.e. not actual hotplugged memory).
-numa node_mem  & numa_set_mem_node_id() are sufficient for this purpose 

> My proposal is to do that inside the code that actually assigns
> addresses to pc-dimm: pc_dimm_realize() (pc.c). So pc-dimm.c and numa.c
> won't reference each other, and numa.c won't need any special code for
> hotplug.
you've probably meant arc_dimm_plug() instead of pc_dimm_realize().
but it shouldn't call numa_set_mem_node_id() since partitioning is static
and is done at startup time. 

> We could have common helpers later to avoid duplicating the same logic
> into other machines, but the point is that we don't need to make numa.c
> carry special memory hotplug code, and we don't need to make pc-dimm.c
> care about -numa.
agreed.

We don't have static partitioning of hotplug memory address space in
x86 target because it limits flexibility of hot-plugging any amount
of memory to any node. (but we could both a static partitioning using
SRAT table and override it _PXM method for dynamic assignment).

The issue with static partitioning is that mgmt tools have to know
addr & size of hotplug memory address space to partition it, but
QEMU doesn't provide that info so far.

> 
> >  
> > > Having a numa_set_memory_region_node_id(MemoryRegion *mr, int node) API
> > > would probably be better, and make the discussion about pc_dimm.addr
> > Wouldn't it be layering violation if we put (frontend) nodeid information
> > into (backend) MemoryRegion?
> 
> We wouldn't, it would be a numa.c function that would just keep track of
> the list of MemoryRegions for each NUMA node.
I sill don't get how idea to use  MemoryRegions applies to above,
you can have MemoryRegions for present memory but there isn't any
for not memory that hasn't been plugged in yet.
numa_set_mem_node_id() looks like a way to go and a simple one at that.

> 
> > 
> > > moot. But it would require changing
> > > memory_region_allocate_system_memory() to avoid
> > > allocate_system_memory_nonnuma() even in the !have_memdevs case.
> > > 
> > 
>