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

[Eduardo Habkost]

On Wed, Jul 02, 2014 at 02:02:14PM -0700, Nishanth Aravamudan wrote:
> On 02.07.2014 [15:21:38 -0300], Eduardo Habkost wrote:
> > 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.
> 
> Ok, so if I send out another revision with the fixed j initialization
> below, is there anything else in my changes that you would like fixed?

I don't see any additional issues.

> 
[...]
> > > > 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.
> 
> Got it, thanks.
> 
> > 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.
> 
> Do you have a preference for which of these to do?

The one we find to be easier. :)

An unit test may require untangling numa.o dependencies. qtest will
probably require parsing the "info numa" output.

A qtest case would cover more code (not just numa.c, but command-ilne
handling on vl.c, and monitor code).

> 
> > 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.
> 
> I'm not comfortable saying I can get to this, as I still don't really
> know the ACPI code, but I can put it on my todo list, at least.
> 
> > The tests won't be a requirement to me, but they would surely be welcome
> > (and would have detected the j=0 mistake above).
> 
> I think it makes sense to put this in now, as it would have caught the
> original issue(s) with sparse node numbering as well.
> 
> Thanks,
> Nish
> 

-- 
Eduardo