Port-xen archive

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]

Re: [PATCH v3 1/7] x86/numa: Make use of NUMA_NO_NODE consistent



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.ostrovsky%oracle.com@localhost>

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);




Home | Main Index | Thread Index | Old Index