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

[Nishanth Aravamudan]

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?

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

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?

> 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