From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55722) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1X2Rfs-0002dP-Oz for qemu-devel@nongnu.org; Wed, 02 Jul 2014 17:02:49 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1X2Rfd-00067d-Uf for qemu-devel@nongnu.org; Wed, 02 Jul 2014 17:02:40 -0400 Received: from e7.ny.us.ibm.com ([32.97.182.137]:49315) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1X2Rfd-00066J-RE for qemu-devel@nongnu.org; Wed, 02 Jul 2014 17:02:25 -0400 Received: from /spool/local by e7.ny.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Wed, 2 Jul 2014 17:02:24 -0400 Date: Wed, 2 Jul 2014 14:02:14 -0700 From: Nishanth Aravamudan Message-ID: <20140702210214.GC29053@linux.vnet.ibm.com> References: <20140701201108.GA21770@linux.vnet.ibm.com> <20140701201328.GB21770@linux.vnet.ibm.com> <20140701203957.GU3222@otherpad.lan.raisama.net> <20140701205006.GA428@linux.vnet.ibm.com> <20140702182138.GY3222@otherpad.lan.raisama.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20140702182138.GY3222@otherpad.lan.raisama.net> Subject: Re: [Qemu-devel] [PATCH 2/2 v5] numa: enable sparse node numbering on ppc List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eduardo Habkost Cc: "Michael S. Tsirkin" , Alexey Kardashevskiy , Hu Tao , qemu-devel@nongnu.org, qemu-ppc@nongnu.org, Anton Blanchard , David Rientjes , Igor Mammedov 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