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