From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48248) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1chC3i-0000XX-Es for qemu-devel@nongnu.org; Fri, 24 Feb 2017 04:21:03 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1chC3e-0003we-Dq for qemu-devel@nongnu.org; Fri, 24 Feb 2017 04:21:02 -0500 Date: Fri, 24 Feb 2017 10:20:54 +0100 From: Igor Mammedov Message-ID: <20170224102054.7bdee6b5@nial.brq.redhat.com> In-Reply-To: <1487784248-112130-1-git-send-email-imammedo@redhat.com> References: <1487784248-112130-1-git-send-email-imammedo@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [RFC] spapr: ensure that all threads within core are on the same NUMA node List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: qemu-devel@nongnu.org Cc: lvivier@redhat.com, thuth@redhat.com, agraf@suse.de, mdroth@linux.vnet.ibm.com, qemu-ppc@nongnu.org, David Gibson On Wed, 22 Feb 2017 18:24:08 +0100 Igor Mammedov wrote: > Threads within a core probably shouldn't be on different > NUMA nodes, so if user has misconfured command line make > fail to start and let user fix it. > For now use the first thread on the core as source > of core's node-id. > > I'm suggesting this to make sure that it would be possible > later map legacy cpu-index based CLI to core-id based > internal map in possible_cpus array and completly eleminate > numa_info[XXX].node_cpu bitmaps, leaving only possible_cpus > as storage of mapping information. > > CCing SPAPR mantainers for oppinion if enforcement > makas sense from plaform poin of view and if we could go > ahead with this or I should look for another way > to deal with legacy -numa CLI. > > Signed-off-by: Igor Mammedov > CC: qemu-ppc@nongnu.org > CC: lvivier@redhat.com > CC: David Gibson > CC: thuth@redhat.com > CC: mdroth@linux.vnet.ibm.com > CC: agraf@suse.de Considering there aren't any objections I'm reposting it as a patch with fixed/sanitized commit message. > --- > hw/ppc/spapr_cpu_core.c | 23 +++++++++++++++-------- > 1 file changed, 15 insertions(+), 8 deletions(-) > > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c > index 55cd045..1499a8b 100644 > --- a/hw/ppc/spapr_cpu_core.c > +++ b/hw/ppc/spapr_cpu_core.c > @@ -50,8 +50,6 @@ static void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu, > Error **errp) > { > CPUPPCState *env = &cpu->env; > - CPUState *cs = CPU(cpu); > - int i; > > /* Set time-base frequency to 512 MHz */ > cpu_ppc_tb_init(env, SPAPR_TIMEBASE_FREQ); > @@ -70,12 +68,6 @@ static void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu, > } > } > > - /* Set NUMA node for the added CPUs */ > - i = numa_get_node_for_cpu(cs->cpu_index); > - if (i < nb_numa_nodes) { > - cs->numa_node = i; > - } > - > xics_cpu_setup(spapr->xics, cpu); > > qemu_register_reset(spapr_cpu_reset, cpu); > @@ -159,11 +151,13 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp) > const char *typename = object_class_get_name(scc->cpu_class); > size_t size = object_type_get_instance_size(typename); > Error *local_err = NULL; > + int core_node_id = numa_get_node_for_cpu(cc->core_id);; > void *obj; > int i, j; > > sc->threads = g_malloc0(size * cc->nr_threads); > for (i = 0; i < cc->nr_threads; i++) { > + int node_id; > char id[32]; > CPUState *cs; > > @@ -172,6 +166,19 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp) > object_initialize(obj, size, typename); > cs = CPU(obj); > cs->cpu_index = cc->core_id + i; > + > + /* Set NUMA node for the added CPUs */ > + node_id = numa_get_node_for_cpu(cs->cpu_index); > + if (node_id != core_node_id) { > + error_setg(&local_err, "Invalid node-id=%d of thread[cpu-index: %d]" > + " on CPU[core-id: %d, node-id: %d], node-id must be the same", > + node_id, cs->cpu_index, cc->core_id, core_node_id); > + goto err; > + } > + if (node_id < nb_numa_nodes) { > + cs->numa_node = node_id; > + } > + > snprintf(id, sizeof(id), "thread[%d]", i); > object_property_add_child(OBJECT(sc), id, obj, &local_err); > if (local_err) {