Re: [Qemu-devel] [PATCH 2/2 v5] numa: enable sparse node numbering on ppc

[Nishanth Aravamudan]

On 01.07.2014 [17:39:57 -0300], Eduardo Habkost wrote:
> On Tue, Jul 01, 2014 at 01:13:28PM -0700, Nishanth Aravamudan wrote:
> [...]
> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > index 12472c6..cdefafe 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -1121,6 +1121,18 @@ PcGuestInfo *pc_guest_info_init(ram_addr_t 
> > below_4g_mem_size,
> >      guest_info->ram_size = below_4g_mem_size + above_4g_mem_size;
> >      guest_info->apic_id_limit = pc_apic_id_limit(max_cpus);
> >      guest_info->apic_xrupt_override = kvm_allows_irq0_override();
> > +    /* No support for sparse NUMA node IDs yet: */
> > +    for (i = max_numa_nodeid - 1; i >= 0; i--) {
> > +        /* Report large node IDs first, to make mistakes easier to spot */
> > +        if (!numa_info[i].present) {
> > +            error_report("numa: Node ID missing: %d", i);
> > +            exit(EXIT_FAILURE);
> > +        }
> > +    }
> > +
> > +    /* This must be always true if all nodes are present */
> > +    assert(num_numa_nodes == max_numa_nodeid);
> > +
> 
> I wonder if there's a better place where we could put this check.

Well, only i386 and ppc support NUMA, afaict. So I'm not sure where it
makes sense to put it. I guess we could have a flag that the
architectures set that indicates sparse NUMA support or not, and put
this in the generic code.

Or do you mean putting this check somewhere else in the PC init code?

> >      guest_info->numa_nodes = num_numa_nodes;
> >      guest_info->node_mem = g_malloc0(guest_info->numa_nodes *
> >                                      sizeof *guest_info->node_mem);
> [...]
> > diff --git a/numa.c b/numa.c
> > index 5930df0..a689e52 100644
> > --- a/numa.c
> > +++ b/numa.c
> [...]
> > @@ -225,9 +220,12 @@ void set_numa_nodes(void)
> >           * must cope with this anyway, because there are BIOSes out there 
> > in
> >           * real machines which also use this scheme.
> >           */
> > -        if (i == num_numa_nodes) {
> > -            for (i = 0; i < max_cpus; i++) {
> > -                set_bit(i, numa_info[i % num_numa_nodes].node_cpu);
> > +        if (i == max_numa_nodeid) {
> > +            for (i = 0, j = 0; i < max_cpus; i++) {
> 
> Doesn't j need to be initialized to -1, here?

Arrgh, sorry had been messing with your suggestion to use a while loop.
You're right, it needs to be -1 here.

> Except for that, patch looks good to me. But I would be more comfortable
> with it if we had automated tests to help ensure we are not breaking
> compatibility of existing NUMA command-line conbinations with these
> changes.

Is that the test target in the qemu source? Are there examples of any
such NUMA tests already?

Thanks,
Nish