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

RE: [PATCH v2 4/9] xen/x86: Use ASSERT instead of VIRTUAL_BUG_ON for phys_to_nid


  • To: Jan Beulich <jbeulich@xxxxxxxx>
  • From: Wei Chen <Wei.Chen@xxxxxxx>
  • Date: Tue, 12 Jul 2022 07:20:22 +0000
  • Accept-language: en-US
  • Arc-authentication-results: i=2; mx.microsoft.com 1; spf=pass (sender ip is 63.35.35.123) smtp.rcpttodomain=lists.xenproject.org smtp.mailfrom=arm.com; dmarc=pass (p=none sp=none pct=100) action=none header.from=arm.com; dkim=pass (signature was verified) header.d=armh.onmicrosoft.com; arc=pass (0 oda=1 ltdi=1 spf=[1,1,smtp.mailfrom=arm.com] dkim=[1,1,header.d=arm.com] dmarc=[1,1,header.from=arm.com])
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=arm.com; dmarc=pass action=none header.from=arm.com; dkim=pass header.d=arm.com; arc=none
  • Arc-message-signature: i=2; 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=dMXwU/OwbKgIu/WSNNo2xHANK0CcAR8RrXbJ/2VMpE4=; b=CpbUyarP1Vt62EL2L7k+JzJWn+2b6okoUjZEvJ2/Y/Fowkbkakd0FX0+CUOu9CmPb3CZ4wmnq/ygHr6RN3JqGZmnct0XQ7E1EaDTcayguiJqMmdv03mej7dMmaAfQhO2NW0JZiPOg//aQBBdGSinlaZVDtQJGsQdSO+bujI7pumr7wb9k8sVr1Z5Ubpve35KhAev1P/knsBXG3YQMFDzouT35sgNGICFDgZvGXR+BaVySaaa9w2NN38sXP5HgvUzeLc5/lGt29vh68Jzb6mP4zwJxJJMrENGygL0hscmUxUUCCbU8bbOR64MhruX0QZAnAz48dbBUqMr6HBaX5fKbA==
  • 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=dMXwU/OwbKgIu/WSNNo2xHANK0CcAR8RrXbJ/2VMpE4=; b=ZPjaFQx3eqnUbzPxbYqO6xI+eMO3GRJQmWuhvAhSHJ2lvr8i4gobwstzDq0KTomKLktpoA8zV1O9ydcxjrSjwd/Tsp8IotMJOiJBtbqTCJvQp8QL2SV2jlacDh/aamwGXaWOy6Bq8vGDmRry9EnPCACayWuqTpSZnm6rvfS3wV40cdElG3JREwkiC21MbXFp/MLXioVuXm1dnW4fzcFX2dpsBcbaJA3Uv0ODNrH85ujRUN4Q7LjW61oqQ6BUamE5AL5LF76pUkKZL3zwhqugRuAT4bfKQUyw2Jf5iR7UwvuWWZwIzaTkwfSvVXTiNhzjxf8l6dETXXrA1Y1C1zI6Ug==
  • Arc-seal: i=2; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=pass; b=SQDq87JVkJ+zTgLrUBlAfZagO/A5LCmO3XxR0onyZToInh1EAsToL+N3O+eSHWZOv6V1kMRpr3nloXwyZo08/UKBR7W/mBU0cppI905GYS4jWinsf9vckZh7edJ8Np68/RkkduEKwEzYHCwhdE3Bp4baeAXbIr6XoEp3SfK9EEx7QvCOUuT7ujc8Uh0JltoVu0zSSEFZ1rwDyWw8hmHWXbmwH2bUp49iH0ZxZulUsMQARoL+6Wn6hkah0zpLfwVX1sQM0MLIW7tRLV8xk0pIdmXqWdoYNiP0L7ZOzxNCL0Vw4ir0TNAbEG/7IbZopSnM75uGXh9KZRjFSbQNXqfRFA==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=LHe1FU2Yno3bIBWBAlBFWrVl3bslBDNHijK2UqcawXHI5nz3QrpooiCeA6MP7wjdJb+WJmBwN0HzV0j+cbNWrEsnfm51GnyAk3WFTEIuxTWm95dCthlVQiaFWDZRoqcj45Lv/1GYzzCYBu44gUzcfJQnOp3z1EMXOsxMuwKXJ7O3Qa2srDliJhY6Oq8LCqG3SMm6CgJXWDJrcQT/67pWB5OIggpwphV38yOBhq190Hl9PcpYXVZ/1SEDmJIgGV0ayM3kOlBOzlqmdfRnEosHC3NXTMMo8WJoldqiqWrgsf9w1MSGJ8op3YAbID6hKO6b0kX+Io530TFHePxsLw5fRA==
  • Authentication-results-original: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=arm.com;
  • Cc: nd <nd@xxxxxxx>, Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, George Dunlap <george.dunlap@xxxxxxxxxx>, Julien Grall <julien@xxxxxxx>, Stefano Stabellini <sstabellini@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, Jiamei Xie <Jiamei.Xie@xxxxxxx>, "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>
  • Delivery-date: Tue, 12 Jul 2022 07:20:54 +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: AQHYktq/lgTeemGHCUuLgTuwCRELQK14uj+AgAGe/lA=
  • Thread-topic: [PATCH v2 4/9] xen/x86: Use ASSERT instead of VIRTUAL_BUG_ON for phys_to_nid

Hi Jan,

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: 2022年7月11日 14:32
> To: Wei Chen <Wei.Chen@xxxxxxx>
> Cc: nd <nd@xxxxxxx>; Andrew Cooper <andrew.cooper3@xxxxxxxxxx>; George
> Dunlap <george.dunlap@xxxxxxxxxx>; Julien Grall <julien@xxxxxxx>; Stefano
> Stabellini <sstabellini@xxxxxxxxxx>; Wei Liu <wl@xxxxxxx>; Jiamei Xie
> <Jiamei.Xie@xxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
> Subject: Re: [PATCH v2 4/9] xen/x86: Use ASSERT instead of VIRTUAL_BUG_ON
> for phys_to_nid
> 
> On 08.07.2022 16:54, Wei Chen wrote:
> > VIRTUAL_BUG_ON is an empty macro used in phys_to_nid. This
> > results in two lines of error-checking code in phys_to_nid
> > that is not actually working and causing two compilation
> > errors:
> > 1. error: "MAX_NUMNODES" undeclared (first use in this function).
> >    This is because in the common header file, "MAX_NUMNODES" is
> >    defined after the common header file includes the ARCH header
> >    file, where phys_to_nid has attempted to use "MAX_NUMNODES".
> >    This error was resolved after we moved the phys_to_nid from
> >    x86 ARCH header file to common header file.
> > 2. error: wrong type argument to unary exclamation mark.
> >    This is because, the error-checking code contains !node_data[nid].
> >    But node_data is a data structure variable, it's not a pointer.
> >
> > So, in this patch, we use ASSERT instead of VIRTUAL_BUG_ON to
> > enable the two lines of error-checking code. And fix the left
> > compilation errors by replacing !node_data[nid] to
> > !node_data[nid].node_spanned_pages. Although NUMA allows one node
> > can only have CPUs but without any memory. And node with 0 bytes
> > of memory might have an entry in memnodemap[] theoretically. But
> > that doesn't mean phys_to_nid can find any valid address from a
> > node with 0 bytes memory.
> >
> > Signed-off-by: Wei Chen <wei.chen@xxxxxxx>
> > Tested-by: Jiamei Xie <jiamei.xie@xxxxxxx>
> > ---
> > v1 -> v2:
> > 1. Move from part#1 to part#2. (Comment from NUMA part1 series)
> > 2. Refine the justification of using
> >    !node_data[nid].node_spanned_pages. (From NUMA part1 series)
> > 3. Use ASSERT to replace VIRTUAL_BUG_ON in phys_to_nid.
> > 4. Adjust the conditional express for ASSERT.
> > 5. Move MAX_NUMNODES from xen/numa.h to asm/numa.h for x86.
> > 6. Use conditional macro to gate MAX_NUMNODES for other architectures.
> 
> The change looks okay, but can you please clarify what these last two
> points describe? They don't seem to match any change ...
> 

I moved this patch form Part#1 to Part#2, but forgot to remove these
two change logs. In Part#1, we do those changes, but after we moved
this patch to Part#2, those changes are unnecessary. I will remove
these two lines of change logs. Sorry for the confusion!

Cheers,
Wei Chen

> > --- a/xen/include/xen/numa.h
> > +++ b/xen/include/xen/numa.h
> > @@ -36,8 +36,6 @@ struct node {
> >  extern int compute_hash_shift(struct node *nodes, int numnodes,
> >                                nodeid_t *nodeids);
> >
> > -#define VIRTUAL_BUG_ON(x)
> > -
> >  /* Enumerations for NUMA status. */
> >  enum numa_mode {
> >      numa_on = 0,
> > @@ -77,9 +75,9 @@ extern struct node_data node_data[];
> >  static inline __attribute__((pure)) nodeid_t phys_to_nid(paddr_t addr)
> >  {
> >      nodeid_t nid;
> > -    VIRTUAL_BUG_ON((paddr_to_pdx(addr) >> memnode_shift) >=
> memnodemapsize);
> > +    ASSERT((paddr_to_pdx(addr) >> memnode_shift) < memnodemapsize);
> >      nid = memnodemap[paddr_to_pdx(addr) >> memnode_shift];
> > -    VIRTUAL_BUG_ON(nid >= MAX_NUMNODES || !node_data[nid]);
> > +    ASSERT(nid < MAX_NUMNODES && node_data[nid].node_spanned_pages);
> >      return nid;
> >  }
> >
> 
> ... in the entire patch.
> 
> Jan

 


Rackspace

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