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 <boris.ostrov...@oracle.com>
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 <asm/e820.h>
> #include <asm/page.h>
>
> +#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);
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
