From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57102) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Yspbu-0002S8-Pl for qemu-devel@nongnu.org; Thu, 14 May 2015 05:39:24 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Yspbq-0001Bh-No for qemu-devel@nongnu.org; Thu, 14 May 2015 05:39:22 -0400 Received: from mx1.redhat.com ([209.132.183.28]:55967) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Yspbq-0001B4-Eq for qemu-devel@nongnu.org; Thu, 14 May 2015 05:39:18 -0400 Message-ID: <55546D3A.8010509@redhat.com> Date: Thu, 14 May 2015 11:39:06 +0200 From: Paolo Bonzini MIME-Version: 1.0 References: <1430982264-25497-1-git-send-email-bharata@linux.vnet.ibm.com> <20150513180607.GK25766@thinpad.lan.raisama.net> In-Reply-To: <20150513180607.GK25766@thinpad.lan.raisama.net> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [RFC PATCH v0] numa: API to lookup NUMA node by address List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eduardo Habkost , Bharata B Rao Cc: imammedo@redhat.com, qemu-devel@nongnu.org, david@gibson.dropbear.id.au On 13/05/2015 20:06, Eduardo Habkost wrote: > Also, this introduces a circular dependency between pc-dimm.c and > numa.c. Instead of that, pc-dimm could simply notify us when a new > device is realized (with just (addr, end, node) as arguments), so we can > save the list of memory ranges inside struct node_info. > > I wonder if the memory API already provides something that would help > us. Paolo, do you see a way we could simply use a MemoryRegion as input > to lookup the NUMA node? No, but I guess you could add a numa_get/set_memory_region_node_id API that uses a hash table. That's a variant of the "pc-dimm could simply notify" numa.c that you propose above. Paolo > >> + qapi_free_MemoryDeviceInfoList(info_list); >> + error_setg(errp, "Address 0x" RAM_ADDR_FMT " doesn't belong to any " >> + "NUMA node", addr); >> + >> + return -1; >> +} >> + >> +static void numa_set_mem_address(int nodenr) >> +{ >> + if (nodenr) { >> + numa_info[nodenr].mem_start = numa_info[nodenr-1].mem_end; > > You isolated the code inside a function, but it requires the function to > be called in a specific nodenr order. I would just make it a loop that > calculates mem_start and mem_end for all nodes, then you won't need a > special case for node 0. > >> + } else { >> + numa_info[nodenr].mem_start = 0; >> + } >> + numa_info[nodenr].mem_end = numa_info[nodenr].mem_start + >> + numa_info[nodenr].node_mem; > > Now that we have specific fields for the memory ranges, it would be > interesting to reuse mem_start and mem_end inside > memory_region_allocate_system_memory() instead of duplicating the > address calculation there. > >> +} >> + >> static void numa_node_parse(NumaNodeOptions *node, QemuOpts *opts, Error **errp) >> { >> uint16_t nodenr; >> @@ -276,6 +333,10 @@ void parse_numa_opts(MachineClass *mc) >> } >> >> for (i = 0; i < nb_numa_nodes; i++) { >> + numa_set_mem_address(i); >> + } >> + >> + for (i = 0; i < nb_numa_nodes; i++) { >> if (!bitmap_empty(numa_info[i].node_cpu, MAX_CPUMASK_BITS)) { >> break; >> } >> -- >> 2.1.0 >> >> >