Re: [PATCH] x86: UV BAU fix for non-consecutive nasids

[Ingo Molnar]

* Cliff Wickman <cpw@xxxxxxx> wrote:

> @@ -1365,12 +1373,14 @@ uv_activation_descriptor_init(int node,
>  		memset(bd2, 0, sizeof(struct bau_desc));
>  		bd2->header.sw_ack_flag = 1;
>  		/*
> -		 * base_dest_nodeid is the nasid of the first uvhub
> -		 * in the partition. The bit map will indicate uvhub numbers,
> -		 * which are 0-N in a partition. Pnodes are unique system-wide.
> +		 * The base_dest_nasid set in the message header is the nasid
> +		 * of the first uvhub in the partition. The bit map will
> +		 * indicate destination pnode numbers relative to that base.
> +		 * They may not be consecutive if nasid striding is being used.
>  		 */
> -		bd2->header.base_dest_nodeid = UV_PNODE_TO_NASID(uv_partition_base_pnode);
> -		bd2->header.dest_subnodeid = 0x10; /* the LB */
> +		bd2->header.base_dest_nasid =
> +					UV_PNODE_TO_NASID(part_base_pnode);
> +		bd2->header.dest_subnodeid = UV_LB_SUBNODEID;
>  		bd2->header.command = UV_NET_ENDPOINT_INTD;
>  		bd2->header.int_both = 1;

This code is sick. Illness: too long lines.

checkpatch warned you but you applied the wrong cure and
broke the lines in an ugly way.

Can you	think of another cure? One of the many options:

  - eliminate many repetitive strings
  - reduce indentation by the introduction of helper inlines
  - sensible shortening of variable/field/definition names

... applies to this particular case and will solve the problem.

> @@ -1528,6 +1539,15 @@ static int __init uv_init_per_cpu(int nu
>  		bcp = &per_cpu(bau_control, cpu);
>  		memset(bcp, 0, sizeof(struct bau_control));
>  		pnode = uv_cpu_hub_info(cpu)->pnode;
> +		if ((pnode - base_part_pnode) >= UV_DISTRIBUTION_SIZE) {
> +			printk(KERN_EMERG
> +				"cpu %d pnode %d-%d beyond %d; BAU disabled\n",
> +				cpu, pnode, base_part_pnode,
> +				UV_DISTRIBUTION_SIZE);

See my points above.

> @@ -1585,6 +1605,20 @@ static int __init uv_init_per_cpu(int nu
>  nextsocket:
>  			socket++;
>  			socket_mask = (socket_mask >> 1);
> +			/* each socket gets a local array of pnodes/hubs */
> +			bcp = smaster;
> +			bcp->target_hub_and_pnode = kmalloc_node(
> +				sizeof(struct hub_and_pnode) *
> +				num_possible_cpus(), GFP_KERNEL, bcp->osnode);
> +			memset(bcp->target_hub_and_pnode, 0,
> +				sizeof(struct hub_and_pnode) *
> +				num_possible_cpus());
> +			for_each_present_cpu(tcpu) {
> +				bcp->target_hub_and_pnode[tcpu].pnode =
> +					uv_cpu_hub_info(tcpu)->pnode;
> +				bcp->target_hub_and_pnode[tcpu].uvhub =
> +					uv_cpu_hub_info(tcpu)->numa_blade_id;
> +			}

See my points above.

Thanks,

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/