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

[Jan Beulich <jbeulich@suse.com>]

On 01.06.2022 04:53, Wei Chen wrote:
>> From: Jan Beulich <jbeulich@suse.com>
>> Sent: 2022年5月31日 21:21
>>
>> 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.)
> 
> The 2nd condition here, you meant is "(nd->end == end && nd->start == start)"
> or just "nd->start == start" after "&&"?

No, I mean the entire 2nd if().

>>> +		/*
>>> +		 * 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).
>>
> 
> Yes, I will add "="in both cases. But for switching around, I also
> wanted to make a better line up. But if nid == memblk_nodeid[i],
> the check of (nd_start < nd->start && nd->end < nd_end) is meaningless.
> I'll adjust their order in the next version if you think this is
> acceptable.

I wasn't referring to the "nid != memblk_nodeid[i]" part at all. What
I'm after is for the two range checks to come as close as possible to
what the other range check does. (Which, as I notice only now, would
include the dropping of the unnecessary inner pair of parentheses.)
E.g. (there are other variations of it)

	        if (nid != memblk_nodeid[i] &&
		    nd->start >= nd_start && nd->end <= nd_end)
			return INTERLEAVE;

>>> @@ -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.
>>
> 
> Why I don't need this one? Did you mean I can use
> switch (conflicting_memblks(...)) directly?

Yes. Why could this not be possible?

>>>  		       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.)
>>
> 
> Thanks, I will adjust it to:
> 	case NO_CONFLICT:
> 		break;
> 	default:
> 		ASSERT_UNREACHABLE();
> in next version.

As said - I consider this form less desirable, as it'll defer
noticing of an issue from build-time to runtime. If you think that
form is better, may I ask why?

Jan