Re: [Qemu-devel] [PATCH 0/7] spapr: rework memory nodes

[Nishanth Aravamudan <nacc@linux.vnet.ibm.com>]

On 18.06.2014 [16:33:55 -0300], Eduardo Habkost wrote:
> On Wed, Jun 18, 2014 at 11:28:53AM -0700, Nishanth Aravamudan wrote:
> > On 17.06.2014 [16:22:33 -0300], Eduardo Habkost wrote:
> > > On Tue, Jun 17, 2014 at 11:38:16AM -0700, Nishanth Aravamudan wrote:
> > > > On 17.06.2014 [11:07:00 -0300], Eduardo Habkost wrote:
> > > > <snip>
> > > > > > If it is canonical and kosher way of using NUMA in QEMU, ok, we can use it.
> > > > > > I just fail to see why we need a requirement for nodes to go consequently
> > > > > > here. And it confuses me as a user a bit if I can add "-numa
> > > > > > node,nodeid=22" (no memory, no cpus) but do not get to see it in the guest.
> > > > > 
> > > > > I agree with you it is confusing. But before we support that use case,
> > > > > we need to make sure auto-allocation is handled properly, because it
> > > > > would be hard to fix it later without breaking compatibility.
> > > > > 
> > > > > We probably just need a "present" field on struct NodeInfo, so
> > > > > machine-specific code and auto-allocation code can differentiate nodes
> > > > > that are not present on the command-line from empty nodes that were
> > > > > specified in the command-line.
> > > > 
> > > > What/where is struct NodeInfo?
> > > 
> > > It was introduced very recently. See the pull request at:
> > > 
> > >   From: "Michael S. Tsirkin" <mst@redhat.com>
> > >   Message-ID: <1403021756-15960-1-git-send-email-mst@redhat.com>
> > >   Subject: [Qemu-devel] [PULL 000/103] pc, pci, virtio, hotplug fixes, enhancements for 2.1
> > > 
> > >   git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git tags/for_upstream
> > 
> > Ah thank you very much!
> > 
> > Before I get cracking on some patches, wanted to clarify some things:
> > 
> > 1) We need something like a "present" field to deal with
> > auto-allocation, which indicates a user-specified NUMA node.
> > 
> > 2) We need something like a "defined" field to indicate which entries
> > are actually valid and which aren't just 0 because they weren't ever set
> > in order to support sparse node numbering.
> > 	2a) We could add a defined field to indicate the defined
> > 	entries, iterate over the entire array and skip those not
> > 	defined [keeps index:nodeid mapping, changes all loops]
> > 	2b) We could add a nodeid field to indicate the defined entries,
> > 	iterate over only nb_numa_nodes [breaks index:nodeid, keeps
> > 	loops the same, but requires using the nodeid member in the
> > 	loops, not guaranteed for the array to be sorted on nodeid]
> > 
> > I'm currently in favor of 2b, with perhaps a call to qsort on the array
> > after parsing to sort by node id? I'd have to audit the users of the
> > array to make sure they use the nodeid member and not the index, but
> > that should be straightforward.
> 
> As the holes in the node ID space don't seem to be frequently large, and
> the ID space is currently very small (we support 8-bit IDs only), 2a
> looks much simpler to implement and review. We can always change the
> code to use 2b if we decide to support larger node IDs in the future.

Ah, I didn't even check to see that MAX_NODES is so small, will do.

> (And we don't even need to iterate over the entire array. We just need
> to iterate up to the highest ID seen on the commend-line.)

Yep, that's a good shortcut, will add it to my changes. That is
equivalent, fwiw, to tracking how many valid nodes you've seen in any
given loop and only checking up to the number known).

Thanks,
Nish