From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH v3 1/7] x86/numa: Make use of NUMA_NO_NODE consistent Date: Tue, 10 Feb 2015 10:41:58 +0000 Message-ID: <54D9E076.1080604__11802.321257028$1423565042$gmane$org@citrix.com> References: <1423512275-6531-1-git-send-email-boris.ostrovsky@oracle.com> <1423512275-6531-2-git-send-email-boris.ostrovsky@oracle.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1423512275-6531-2-git-send-email-boris.ostrovsky@oracle.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Boris Ostrovsky , jbeulich@suse.com, keir@xen.org, ian.jackson@eu.citrix.com, ian.campbell@citrix.com, stefano.stabellini@eu.citrix.com, wei.liu2@citrix.com Cc: dario.faggioli@citrix.com, port-xen@netbsd.org, ufimtseva@gmail.com, xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org On 09/02/15 20:04, Boris Ostrovsky wrote: > Various pieces of code test whether node value is NUMA_NO_NODE even > though pxm_to_node() may return (int)-1 for an invalid node. > > Make pxm_to_node() and setup_node() return u8 and have them return > NUMA_NO_NODE when necessary. > > Adjust code that tests for (node == -1). > > Signed-off-by: Boris Ostrovsky Looks fine. A few minor comments. > --- > xen/arch/x86/smpboot.c | 2 +- > xen/arch/x86/srat.c | 39 ++++++++++++++++++++++------------ > xen/arch/x86/x86_64/mm.c | 2 +- > xen/drivers/passthrough/vtd/iommu.c | 4 +- > xen/include/asm-x86/numa.h | 4 +- > 5 files changed, 31 insertions(+), 20 deletions(-) > > diff --git a/xen/arch/x86/smpboot.c b/xen/arch/x86/smpboot.c > index c54be7e..97956be 100644 > --- a/xen/arch/x86/smpboot.c > +++ b/xen/arch/x86/smpboot.c > @@ -877,7 +877,7 @@ int cpu_add(uint32_t apic_id, uint32_t acpi_id, uint32_t pxm) > > if ( !srat_disabled() ) > { > - if ( (node = setup_node(pxm)) < 0 ) > + if ( (node = setup_node(pxm)) == NUMA_NO_NODE ) > { > dprintk(XENLOG_WARNING, > "Setup node failed for pxm %x\n", pxm); > diff --git a/xen/arch/x86/srat.c b/xen/arch/x86/srat.c > index 29fc724..4dfa1c3 100644 > --- a/xen/arch/x86/srat.c > +++ b/xen/arch/x86/srat.c > @@ -21,13 +21,16 @@ > #include > #include > > +#define MAX_PXM 255 > + > static struct acpi_table_slit *__read_mostly acpi_slit; > > static nodemask_t memory_nodes_parsed __initdata; > static nodemask_t processor_nodes_parsed __initdata; > static nodemask_t nodes_found __initdata; > static struct node nodes[MAX_NUMNODES] __initdata; > -static u8 __read_mostly pxm2node[256] = { [0 ... 255] = NUMA_NO_NODE }; > +static u8 __read_mostly pxm2node[MAX_PXM + 1] = > + { [0 ... MAX_PXM] = NUMA_NO_NODE }; > > > static int num_node_memblks; > @@ -37,21 +40,29 @@ static int memblk_nodeid[NR_NODE_MEMBLKS]; > > static int node_to_pxm(int n); > > -int pxm_to_node(int pxm) > +u8 pxm_to_node(int pxm) You can make these parameters unsigned and do away with the unsigned casting. pxm appears to be unsigned in all the relevant call chains. > { > - if ((unsigned)pxm >= 256) > - return -1; > - /* Extend 0xff to (int)-1 */ > - return (signed char)pxm2node[pxm]; > + if ((unsigned)pxm > MAX_PXM) > + return NUMA_NO_NODE; > + > + return pxm2node[pxm]; > } > > -__devinit int setup_node(int pxm) > +__devinit u8 setup_node(int pxm) > { > - unsigned node = pxm2node[pxm]; > - if (node == 0xff) { > + u8 node; > + > + /* NUMA_NO_NODE is 255 */ > + BUILD_BUG_ON(MAX_NUMNODES > 254); > + > + if ((unsigned)pxm > MAX_PXM) > + return NUMA_NO_NODE; > + > + node = pxm2node[pxm]; > + if (node == NUMA_NO_NODE) { > if (nodes_weight(nodes_found) >= MAX_NUMNODES) > - return -1; > - node = first_unset_node(nodes_found); > + return NUMA_NO_NODE; > + node = first_unset_node(nodes_found); > node_set(node, nodes_found); > pxm2node[pxm] = node; > } > @@ -175,7 +186,7 @@ acpi_numa_x2apic_affinity_init(struct acpi_srat_x2apic_cpu_affinity *pa) > return; > pxm = pa->proximity_domain; > node = setup_node(pxm); > - if (node < 0) { > + if (node == NUMA_NO_NODE) { > printk(KERN_ERR "SRAT: Too many proximity domains %x\n", pxm); > bad_srat(); > return; > @@ -208,7 +219,7 @@ acpi_numa_processor_affinity_init(struct acpi_srat_cpu_affinity *pa) > pxm |= pa->proximity_domain_hi[2] << 24; > } > node = setup_node(pxm); > - if (node < 0) { > + if (node == NUMA_NO_NODE) { > printk(KERN_ERR "SRAT: Too many proximity domains %x\n", pxm); > bad_srat(); > return; > @@ -252,7 +263,7 @@ acpi_numa_memory_affinity_init(struct acpi_srat_mem_affinity *ma) > if (srat_rev < 2) > pxm &= 0xff; > node = setup_node(pxm); > - if (node < 0) { > + if (node == NUMA_NO_NODE) { > printk(KERN_ERR "SRAT: Too many proximity domains.\n"); > bad_srat(); > return; > diff --git a/xen/arch/x86/x86_64/mm.c b/xen/arch/x86/x86_64/mm.c > index d631aee..4817dad 100644 > --- a/xen/arch/x86/x86_64/mm.c > +++ b/xen/arch/x86/x86_64/mm.c > @@ -1353,7 +1353,7 @@ int memory_add(unsigned long spfn, unsigned long epfn, unsigned int pxm) > if ( !mem_hotadd_check(spfn, epfn) ) > return -EINVAL; > > - if ( (node = setup_node(pxm)) == -1 ) > + if ( (node = setup_node(pxm)) == NUMA_NO_NODE ) > return -EINVAL; > > if ( !valid_numa_range(spfn << PAGE_SHIFT, epfn << PAGE_SHIFT, node) ) > diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c > index 19d8165..fd48f7b 100644 > --- a/xen/drivers/passthrough/vtd/iommu.c > +++ b/xen/drivers/passthrough/vtd/iommu.c > @@ -190,14 +190,14 @@ u64 alloc_pgtable_maddr(struct acpi_drhd_unit *drhd, unsigned long npages) > struct acpi_rhsa_unit *rhsa; > struct page_info *pg, *cur_pg; > u64 *vaddr; > - int node = -1, i; > + int node = NUMA_NO_NODE, i; > > rhsa = drhd_to_rhsa(drhd); > if ( rhsa ) > node = pxm_to_node(rhsa->proximity_domain); > > pg = alloc_domheap_pages(NULL, get_order_from_pages(npages), > - (node == -1 ) ? 0 : MEMF_node(node)); > + (node == NUMA_NO_NODE ) ? 0 : MEMF_node(node)); Drop the stray space while you are editing this line. > if ( !pg ) > return 0; > > diff --git a/xen/include/asm-x86/numa.h b/xen/include/asm-x86/numa.h > index 5959860..2a43827 100644 > --- a/xen/include/asm-x86/numa.h > +++ b/xen/include/asm-x86/numa.h > @@ -21,7 +21,7 @@ struct node { > > extern int compute_hash_shift(struct node *nodes, int numnodes, > int *nodeids); > -extern int pxm_to_node(int nid); > +extern u8 pxm_to_node(int nid); The parameter should presumably be named pxm? ~Andrew > > #define ZONE_ALIGN (1UL << (MAX_ORDER+PAGE_SHIFT)) > #define VIRTUAL_BUG_ON(x) > @@ -33,7 +33,7 @@ extern int numa_off; > > extern int srat_disabled(void); > extern void numa_set_node(int cpu, int node); > -extern int setup_node(int pxm); > +extern u8 setup_node(int pxm); > extern void srat_detect_node(int cpu); > > extern void setup_node_bootmem(int nodeid, u64 start, u64 end);