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

[Eduardo Habkost]

On Tue, Jul 01, 2014 at 01:50:06PM -0700, Nishanth Aravamudan wrote:
> 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?

I mean somewhere else in the PC init code. But as today the code that
calls pc_guest_info_init() and pc_memory_init() is duplicated in both
pc_piix.c and pc_q35.c, this looks like the best place we have.

> 
> > >      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?

I use 'make check' to run them, they are in the tests/ directory.

I am not aware of any NUMA-related test, but I see two possible ways of
testing it: using qtest and asking for for the NUMA node info through
the monitor, or a unit test for numa.c that simply calls
numa_node_parse() and set_numa_nodes(), and then checks the result on
numa_info[] directly.

A third option may be using qtest and checking the resulting ACPI tables
directly. It would cover even more code, but would be specific to PC.

The tests won't be a requirement to me, but they would surely be welcome
(and would have detected the j=0 mistake above).

-- 
Eduardo