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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 01.06.2022 04:53, Wei Chen wrote:
>> From: Jan Beulich <jbeulich@xxxxxxxx>
>> 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