Re: [PATCH v4 7/8] xen/x86: add detection of memory interleaves for different nodes

[Jan Beulich <jbeulich@suse.com>]

On 23.05.2022 08:25, Wei Chen wrote:
> @@ -119,20 +125,45 @@ int valid_numa_range(paddr_t start, paddr_t end, nodeid_t node)
>  	return 0;
>  }
>  
> -static __init int conflicting_memblks(paddr_t start, paddr_t end)
> +static
> +enum conflicts __init conflicting_memblks(nodeid_t nid, paddr_t start,
> +					  paddr_t end, paddr_t nd_start,
> +					  paddr_t nd_end, unsigned int *mblkid)
>  {
> -	int i;
> +	unsigned int i;
>  
> +	/*
> +	 * Scan all recorded nodes' memory blocks to check conflicts:
> +	 * Overlap or interleave.
> +	 */
>  	for (i = 0; i < num_node_memblks; i++) {
>  		struct node *nd = &node_memblk_range[i];
> +
> +		*mblkid = i;
> +
> +		/* Skip 0 bytes node memory block. */
>  		if (nd->start == nd->end)
>  			continue;
> +		/*
> +		 * Use memblk range to check memblk overlaps, include the
> +		 * self-overlap case.
> +		 */
>  		if (nd->end > start && nd->start < end)
> -			return i;
> +			return OVERLAP;
>  		if (nd->end == end && nd->start == start)
> -			return i;
> +			return OVERLAP;

Knowing that nd's range is non-empty, is this 2nd condition actually
needed here? (Such an adjustment, if you decided to make it and if not
split out to a separate patch, would need calling out in the
description.)

> +		/*
> +		 * Use node memory range to check whether new range contains
> +		 * memory from other nodes - interleave check. We just need
> +		 * to check full contains situation. Because overlaps have
> +		 * been checked above.
> +		 */
> +	        if (nid != memblk_nodeid[i] &&
> +		    (nd_start < nd->start && nd->end < nd_end))
> +			return INTERLEAVE;

Doesn't this need to be <= in both cases (albeit I think one of the two
expressions would want switching around, to better line up with the
earlier one, visible in context further up).

> @@ -275,10 +306,13 @@ acpi_numa_processor_affinity_init(const struct acpi_srat_cpu_affinity *pa)
>  void __init
>  acpi_numa_memory_affinity_init(const struct acpi_srat_mem_affinity *ma)
>  {
> +	enum conflicts status;

I don't think you need this local variable.

> @@ -310,42 +344,78 @@ acpi_numa_memory_affinity_init(const struct acpi_srat_mem_affinity *ma)
>  		bad_srat();
>  		return;
>  	}
> +
> +	/*
> +	 * For the node that already has some memory blocks, we will
> +	 * expand the node memory range temporarily to check memory
> +	 * interleaves with other nodes. We will not use this node
> +	 * temp memory range to check overlaps, because it will mask
> +	 * the overlaps in same node.
> +	 *
> +	 * Node with 0 bytes memory doesn't need this expandsion.
> +	 */
> +	nd_start = start;
> +	nd_end = end;
> +	nd = &nodes[node];
> +	if (nd->start != nd->end) {
> +		if (nd_start > nd->start)
> +			nd_start = nd->start;
> +
> +		if (nd_end < nd->end)
> +			nd_end = nd->end;
> +	}
> +
>  	/* It is fine to add this area to the nodes data it will be used later*/
> -	i = conflicting_memblks(start, end);
> -	if (i < 0)
> -		/* everything fine */;
> -	else if (memblk_nodeid[i] == node) {
> -		bool mismatch = !(ma->flags & ACPI_SRAT_MEM_HOT_PLUGGABLE) !=
> -		                !test_bit(i, memblk_hotplug);
> -
> -		printk("%sSRAT: PXM %u (%"PRIpaddr"-%"PRIpaddr") overlaps with itself (%"PRIpaddr"-%"PRIpaddr")\n",
> -		       mismatch ? KERN_ERR : KERN_WARNING, pxm, start, end,
> -		       node_memblk_range[i].start, node_memblk_range[i].end);
> -		if (mismatch) {
> +	status = conflicting_memblks(node, start, end, nd_start, nd_end, &i);
> +	switch(status)
> +	{

Style: Missing blank before ( and the brace goes on the same line here
(Linux style).

> +	case OVERLAP:
> +	{

Please omit braces at case labels unless you need a new scope to declare
variables.

> +		if (memblk_nodeid[i] == node) {
> +			bool mismatch = !(ma->flags &
> +					  ACPI_SRAT_MEM_HOT_PLUGGABLE) !=
> +			                !test_bit(i, memblk_hotplug);
> +
> +			printk("%sSRAT: PXM %u (%"PRIpaddr"-%"PRIpaddr") overlaps with itself (%"PRIpaddr"-%"PRIpaddr")\n",
> +			       mismatch ? KERN_ERR : KERN_WARNING, pxm, start,
> +			       end, node_memblk_range[i].start,
> +			       node_memblk_range[i].end);
> +			if (mismatch) {
> +				bad_srat();
> +				return;
> +			}
> +			break;
> +		} else {
> +			printk(KERN_ERR
> +			       "SRAT: PXM %u (%"PRIpaddr"-%"PRIpaddr") overlaps with PXM %u (%"PRIpaddr"-%"PRIpaddr")\n",
> +			       pxm, start, end, node_to_pxm(memblk_nodeid[i]),
> +			       node_memblk_range[i].start,
> +			       node_memblk_range[i].end);
>  			bad_srat();
>  			return;
>  		}

To limit indentation depth, on of the two sides of the conditional can
be moved out, by omitting the unnecessary "else". To reduce the diff
it may be worthwhile to invert the if() condition, allowing the (then
implicit) "else" case to remain (almost) unchanged from the original.

> -	} else {
> +	}
> +
> +	case INTERLEAVE:
> +	{
>  		printk(KERN_ERR
> -		       "SRAT: PXM %u (%"PRIpaddr"-%"PRIpaddr") overlaps with PXM %u (%"PRIpaddr"-%"PRIpaddr")\n",
> -		       pxm, start, end, node_to_pxm(memblk_nodeid[i]),
> +		       "SRAT： PXM %u: (%"PRIpaddr"-%"PRIpaddr") interleaves with PXM %u memblk (%"PRIpaddr"-%"PRIpaddr")\n",
> +		       node, nd_start, nd_end, node_to_pxm(memblk_nodeid[i]),

Hmm, you have PXM in the log message text, but you still pass "node" as
first argument.

Since you're touching all these messages, could I ask you to convert
all ranges to proper mathematical interval representation? I.e.
[start,end) here aiui as the end addresses look to be non-inclusive.

>  		       node_memblk_range[i].start, node_memblk_range[i].end);
>  		bad_srat();
>  		return;
>  	}
> -	if (!(ma->flags & ACPI_SRAT_MEM_HOT_PLUGGABLE)) {
> -		struct node *nd = &nodes[node];
>  
> -		if (!node_test_and_set(node, memory_nodes_parsed)) {
> -			nd->start = start;
> -			nd->end = end;
> -		} else {
> -			if (start < nd->start)
> -				nd->start = start;
> -			if (nd->end < end)
> -				nd->end = end;
> -		}
> +	default:
> +		break;

This wants to be "case NO_CONFLICT:", such that the compiler would
warn if a new enumerator appears without adding code here. (An
alternative - which personally I don't like - would be to put
ASSERT_UNREACHABLE() in the default: case. The downside is that
then the issue would only be noticeable at runtime.)

Jan