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

RE: [PATCH 11/37] xen/x86: abstract neutral code from acpi_numa_memory_affinity_init


  • To: Jan Beulich <jbeulich@xxxxxxxx>
  • From: Wei Chen <Wei.Chen@xxxxxxx>
  • Date: Wed, 26 Jan 2022 10:39:14 +0000
  • Accept-language: en-US
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=none; dmarc=none; dkim=none; 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=x2BZCvhovCMNnmEWxeFTF+UWgef68eeDSrf+ldkITuA=; b=ZNfq6bcm0oHAPgYxibsBrNFNCYXnbqZA//s5wfYMblZ2isDFGUKro0B2oPwkNBwTPfSm29XGswvJtnC4lE/6UhH17jgXGp2CKw2Ex1KKnDAJEsxbQNn8h8xYeY7F7cz9BxdJzGmWv5FXeRSYOUrG1ZIcF+FYrqkTah48+sqT8EejKzOuBOscLurFakUkSSWOs9Mi5VtFXvXy43Gj/WvtV86sTRl8Jn0ZqjIAklsBEgAfeWO0760ztfigucc5+i8haxSYf7/nr527L6QiVzu8/6QA6wc3Vj1pN4C+37LT4Il9TbpNyTsMUJWkw5vvZ5CBOcYsbo8MgJQNrIgkRnZeZg==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=dakYow+C33quss08nP6BJ/x5O5TJjxwaLTSgDgZ/pL3HL04652t2lD5ieF8G9HOHn/52wd0k1lMF06G9RAe+fm+cO2kh5KCi1uctvy4oU8nn5vsvY9FPuX1vkTniuM7MhZFlpXj24VPzadkxyJt9VjxIRonMz8MoUM0mEuRfwV4Wbt5OsDdNd98zVY1YWelZTGDjypozX+0OVQVVDQqRFs5hOQ/n+UUiExPPtNS0iOXVsKlk7btqbCNkiefcy3yLzjwumyQQN+HIvr4IIywNSkXKe9C4MV9JRzr3DpwNhcTnX//eRRS5PVBQF0YJ+aw8z+1jpn9YS1nurxfriT0wuQ==
  • Authentication-results-original: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=arm.com;
  • Cc: Bertrand Marquis <Bertrand.Marquis@xxxxxxx>, "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>, "sstabellini@xxxxxxxxxx" <sstabellini@xxxxxxxxxx>, "julien@xxxxxxx" <julien@xxxxxxx>
  • Delivery-date: Wed, 26 Jan 2022 10:39:37 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
  • Nodisclaimer: true
  • Original-authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=arm.com;
  • Thread-index: AQHXsHMOTNAGPK2yVEyFmawOhLakc6xzJCgAgAKROiA=
  • Thread-topic: [PATCH 11/37] xen/x86: abstract neutral code from acpi_numa_memory_affinity_init

Hi Jan,

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: 2022年1月25日 0:51
> To: Wei Chen <Wei.Chen@xxxxxxx>
> Cc: Bertrand Marquis <Bertrand.Marquis@xxxxxxx>; xen-
> devel@xxxxxxxxxxxxxxxxxxxx; sstabellini@xxxxxxxxxx; julien@xxxxxxx
> Subject: Re: [PATCH 11/37] xen/x86: abstract neutral code from
> acpi_numa_memory_affinity_init
> 
> On 23.09.2021 14:02, Wei Chen wrote:
> > There is some code in acpi_numa_memory_affinity_init to update node
> > memory range and update node_memblk_range array. This code is not
> > ACPI specific, it can be shared by other NUMA implementation, like
> > device tree based NUMA implementation.
> >
> > So in this patch, we abstract this memory range and blocks relative
> > code to a new function. This will avoid exporting static variables
> > like node_memblk_range. And the PXM in neutral code print messages
> > have been replaced by NODE, as PXM is ACPI specific.
> >
> > Signed-off-by: Wei Chen <wei.chen@xxxxxxx>
> 
> SRAT is an ACPI concept, which I assume has no meaning with DT. Hence
> any generically usable logic here wants, I think, separating out into
> a file which is not SRAT-specific (peeking ahead, specifically not a
> file named "numa_srat.c"). This may in turn require some more though

When I created the file, I wanted to place non-ACPI/DT specific code in
a new file. But I was confused about how to name it. I chose numa_srat.c
as the file name because I thought the device tree is also a static
resource table. But it seems this name is still misleading, because
ACPI SRAT is well known. 

> regarding the proper split between the stuff remaining in srat.c and
> the stuff becoming kind of library code. In particular this may mean
> moving some of the static variables as well, and with them perhaps
> some further functions (while I did peek ahead, I didn't look closely
> at the later patch doing the actual movement). And it is then hard to
> see why the separation needs to happen in two steps - you could move
> the generically usable code to a new file right away.
> 

OK, I will reduce the steps. And I think the "new file" can be common/numa.c.
Because the generically usable code are some logical functions to check numa
memory blocks/ranges and update nodes, we don't need a "numa_srat.c".

> > --- a/xen/arch/x86/srat.c
> > +++ b/xen/arch/x86/srat.c
> > @@ -104,6 +104,14 @@ nodeid_t setup_node(unsigned pxm)
> >     return node;
> >  }
> >
> > +bool __init numa_memblks_available(void)
> > +{
> > +   if (num_node_memblks < NR_NODE_MEMBLKS)
> > +           return true;
> > +
> > +   return false;
> > +}
> 
> Please can you avoid expressing things in more complex than necessary
> ways? Here I don't see why it can't just be

OK, I will simplify it.

> 
> bool __init numa_memblks_available(void)
> {
>       return num_node_memblks < NR_NODE_MEMBLKS;
> }
> 
> > @@ -301,69 +309,35 @@ static bool __init
> is_node_memory_continuous(nodeid_t nid,
> >     return true;
> >  }
> >
> > -/* Callback for parsing of the Proximity Domain <-> Memory Area
> mappings */
> > -void __init
> > -acpi_numa_memory_affinity_init(const struct acpi_srat_mem_affinity *ma)
> > +/* Neutral NUMA memory affinity init function for ACPI and DT */
> > +int __init numa_update_node_memblks(nodeid_t node,
> > +           paddr_t start, paddr_t size, bool hotplug)
> 
> Indentation.

OK.

> 
> >  {
> > -   paddr_t start, end;
> > -   unsigned pxm;
> > -   nodeid_t node;
> > +   paddr_t end = start + size;
> >     int i;
> >
> > -   if (srat_disabled())
> > -           return;
> > -   if (ma->header.length != sizeof(struct acpi_srat_mem_affinity)) {
> > -           bad_srat();
> > -           return;
> > -   }
> > -   if (!(ma->flags & ACPI_SRAT_MEM_ENABLED))
> > -           return;
> > -
> > -   start = ma->base_address;
> > -   end = start + ma->length;
> > -   /* Supplement the heuristics in l1tf_calculations(). */
> > -   l1tf_safe_maddr = max(l1tf_safe_maddr, ROUNDUP(end, PAGE_SIZE));
> > -
> > -   if (num_node_memblks >= NR_NODE_MEMBLKS)
> > -   {
> > -           dprintk(XENLOG_WARNING,
> > -                "Too many numa entry, try bigger NR_NODE_MEMBLKS \n");
> > -           bad_srat();
> > -           return;
> > -   }
> > -
> > -   pxm = ma->proximity_domain;
> > -   if (srat_rev < 2)
> > -           pxm &= 0xff;
> > -   node = setup_node(pxm);
> > -   if (node == NUMA_NO_NODE) {
> > -           bad_srat();
> > -           return;
> > -   }
> > -   /* It is fine to add this area to the nodes data it will be used
> later*/
> > +   /* 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);
> > +           bool mismatch = !hotplug != !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,
> > +           printk("%sSRAT: NODE %u (%"PRIpaddr"-%"PRIpaddr") overlaps
> with itself (%"PRIpaddr"-%"PRIpaddr")\n",
> 
> Nit: Unlike PXM, which is an acronym, "node" doesn't want to be all upper
> case.
> 

OK.

> Also did you check that the node <-> PXM association is known to a reader
> of a log at this point in time?
> 

Yes, I read your comment below. The original log contains node <-> PXM mapping.
Because PXM is ACPI specific, I think in neutral code, we just need node in
log. But this change removed the log of node <-> PXM association, it was my
mistake. I will add them back in next version. But I don't think they will stay 
in
neutal code, it would be in ACPI specific code.

I also had wanted to keep PXM <-> node mapping in the log, so I set up PXM to
node 1-1 mapping for the device tree. But then I thought it was an unnecessary
burden on the device tree, so I selected to remove PXM in log.  

> > +                  mismatch ? KERN_ERR : KERN_WARNING, node, start, end,
> >                    node_memblk_range[i].start, node_memblk_range[i].end);
> >             if (mismatch) {
> > -                   bad_srat();
> > -                   return;
> > +                   return -1;
> >             }
> >     } 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]),
> > +                  "SRAT: NODE %u (%"PRIpaddr"-%"PRIpaddr") overlaps with
> NODE %u (%"PRIpaddr"-%"PRIpaddr")\n",
> > +                  node, start, end, memblk_nodeid[i],
> >                    node_memblk_range[i].start, node_memblk_range[i].end);
> > -           bad_srat();
> > -           return;
> > +           return -1;
> 
> Please no -1 return values. Either a function means to return boolean,
> in which case it should use bool / true / false, or it means to return
> a proper errno-style error code.
> 

Ok, I will do it.

> > @@ -375,26 +349,69 @@ acpi_numa_memory_affinity_init(const struct
> acpi_srat_mem_affinity *ma)
> >                     if (nd->end < end)
> >                             nd->end = end;
> >
> > -                   /* Check whether this range contains memory for other
> nodes */
> > -                   if (!is_node_memory_continuous(node, nd->start, 
> > nd->end))
> {
> > -                           bad_srat();
> > -                           return;
> > -                   }
> > +                   if (!is_node_memory_continuous(node, nd->start, 
> > nd->end))
> > +                           return -1;
> >             }
> >     }
> > -   printk(KERN_INFO "SRAT: Node %u PXM %u %"PRIpaddr"-%"PRIpaddr"%s\n",
> > -          node, pxm, start, end,
> > -          ma->flags & ACPI_SRAT_MEM_HOT_PLUGGABLE ? " (hotplug)" : "");
> > +
> > +   printk(KERN_INFO "SRAT: Node %u %"PRIpaddr"-%"PRIpaddr"%s\n",
> > +          node, start, end, hotplug ? " (hotplug)" : "");
> 
> Continuing from a comment further up: Here you remove an instance of
> logging the node <-> PXM association.
> 

see my above comments.

> >     node_memblk_range[num_node_memblks].start = start;
> >     node_memblk_range[num_node_memblks].end = end;
> >     memblk_nodeid[num_node_memblks] = node;
> > -   if (ma->flags & ACPI_SRAT_MEM_HOT_PLUGGABLE) {
> > +   if (hotplug) {
> >             __set_bit(num_node_memblks, memblk_hotplug);
> >             if (end > mem_hotplug_boundary())
> >                     mem_hotplug_update_boundary(end);
> >     }
> >     num_node_memblks++;
> > +
> > +   return 0;
> > +}
> > +
> > +/* Callback for parsing of the Proximity Domain <-> Memory Area
> mappings */
> > +void __init
> > +acpi_numa_memory_affinity_init(const struct acpi_srat_mem_affinity *ma)
> > +{
> > +   unsigned pxm;
> > +   nodeid_t node;
> > +   int ret;
> > +
> > +   if (srat_disabled())
> > +           return;
> > +   if (ma->header.length != sizeof(struct acpi_srat_mem_affinity)) {
> > +           bad_srat();
> > +           return;
> > +   }
> > +   if (!(ma->flags & ACPI_SRAT_MEM_ENABLED))
> > +           return;
> > +
> > +   /* Supplement the heuristics in l1tf_calculations(). */
> > +   l1tf_safe_maddr = max(l1tf_safe_maddr,
> > +                   ROUNDUP((ma->base_address + ma->length), PAGE_SIZE));
> 
> Indentation and unnecessary pair of parentheses.
> 

OK.

> > +   if (!numa_memblks_available())
> > +   {
> 
> For code you touch, please try to bring it into consistent style. Here
> the brace wants to move to the previous line, seeing that the file is
> using Linux style.
> 

OK.

> > +           dprintk(XENLOG_WARNING,
> > +                "Too many numa entry, try bigger NR_NODE_MEMBLKS \n");
> 
> Here you want to fix indentation and ideally also format and grammar of
> the actual log message.
> 

I will fix it, thanks.

> > +           bad_srat();
> > +           return;
> > +   }
> > +
> > +   pxm = ma->proximity_domain;
> > +   if (srat_rev < 2)
> > +           pxm &= 0xff;
> > +   node = setup_node(pxm);
> > +   if (node == NUMA_NO_NODE) {
> > +           bad_srat();
> > +           return;
> > +   }
> > +
> > +   ret = numa_update_node_memblks(node, ma->base_address, ma->length,
> > +                                   ma->flags & 
> > ACPI_SRAT_MEM_HOT_PLUGGABLE);
> 
> Indentation again.

Ok.

> 
> Jan


 


Rackspace

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