[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


  • To: Wei Chen <Wei.Chen@xxxxxxx>
  • From: Jan Beulich <jbeulich@xxxxxxxx>
  • Date: Thu, 19 May 2022 08:17:58 +0200
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=suse.com; dmarc=pass action=none header.from=suse.com; dkim=pass header.d=suse.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=P0WPu9sAXKxQTk+WGl5yre8/RwW5c771AEUl+e2xJQc=; b=SGtoniKLCrZmS4zKrVBnFGjOKnBxBwvkPKV1GsgmpGdG8PP2AFG+AurmPTOVpw9k3JofpAZUhUimYUkOrrVSSZMa4mfnqozxJzEOJ7VNEcucPOsc99AYesyNUMABBy2/uufg0qq3w9bMd8rXQ56d8JXVs+u9zJS5Sqd3X+5ms4SX46VPPhHtS0nEIfT2tYgp6qZCYYZDp02eo3aaBnwyWWydJwxW1UYgJXsfIiLto1Rt9z7niuOTQA6u8nU5jLSqO8LRKLms2QzaxNfIdirrQGdGSs2ExeNd8OVmK/RLMGjsh0Kip6ljX8jArRFVUv9SkcA0GgFAeNVfHqff81WGQg==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=nMWxmo2VGINRn2FaRsSK4Ivb8dmLy7FBgGOr+FgRSiF1yYeuoA9CEjZNTlo7oSaktR/2iolefLQlZldtPCv4IFokng/DgKWtfIkyQM/2w8/eg3qgHYI44tklDqBcvGZvtFRKPbkPjKTyrPD34iv7Qm3PdCFVCEmlScsTdX1am+LJhMp+swb31euHlR7LBKrsXPUNPJ4XAUkoQOHjm0iAXA1o51DqWTwYWxeD/b/YTPTdBWWMpAl1IT6FWfqnZDV0TPEycQmA8QsN2fzqX0IQBuB3EdReEBZk2J/fUjA98uQfEQ0kJaPJDmXFPdXbbCrd60re2Lm5DrIYARZ3ZeOkUg==
  • Authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=suse.com;
  • Cc: nd <nd@xxxxxxx>, Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, Roger Pau Monné <roger.pau@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, Jiamei Xie <Jiamei.Xie@xxxxxxx>, "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>
  • Delivery-date: Thu, 19 May 2022 06:18:09 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On 19.05.2022 05:37, Wei Chen wrote:
>> From: Jan Beulich <jbeulich@xxxxxxxx>
>> Sent: 2022年5月18日 21:31
>>
>> On 11.05.2022 03:46, Wei Chen wrote:
>>> @@ -310,42 +342,67 @@ 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 < end)
>>
>> Did you mean nd->end here on the right side of < ? By intentionally
> 
> Oh! thanks for pointing out this one! Yes, right side should be nd->end.
> 
>> not adding "default:" in the body, you then also allow the compiler
>> to point out that addition of a new enumerator also needs handling
>> here.
>>
> 
> Did you mean, we need to add if ... else ... in this block? If yes,
> is it ok to update this block like:
>       if (nd->start != nd->end) {
>               nd_start = min(nd_start, nd->start);
>               nd_end = max(nd_end, nd->end);
>       }
> ?

No. I attached this part about "default:" late in the process of writing
the reply, and I did put it in the wrong spot. I'm sorry. It really was
meant to go ...

>>> +                   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);
>>> +   if (status == ERR_OVERLAP) {
>>
>> Please use switch(status) when checking enumerated values.
>>
> 
> Ok, I will do it.

... here, explaining the request to use switch().

>>> +                   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;
>>> +                   }
>>> +           } 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;
>>>             }
>>> -   } else {
>>> +   } else if (status == ERR_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: Node %u: (%"PRIpaddr"-%"PRIpaddr") interleaves
>> with node %u memblk (%"PRIpaddr"-%"PRIpaddr")\n",
>>> +                  node, nd_start, nd_end, memblk_nodeid[i],
>>
>> Please log pxm (not node) here just like is done in the overlap case.
>> The remote node ID will then require converting to PXM, of course.
>>
> 
> Ok, will use PXM here. But I have question for upcoming changes, if we
> move this part of code to common. As device tree NUMA doesn't have
> PXM concept (even I can use a fake node_to_pxm to do 1:1 mapping), so
> can we still use PXM here?

This will want properly abstracting once made common, yes. What the correct
model on Arm/DT is I can't really tell. But my (earlier voiced) request
remains: What is logged should by referring the firmware provided values,
not Xen-internal ones. Otherwise someone reading the log cannot easily
know / derive what's wrong where.

Jan




 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.