[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: Wei Chen <wei.chen@xxxxxxx>
  • From: Jan Beulich <jbeulich@xxxxxxxx>
  • Date: Mon, 11 Jul 2022 08:32:23 +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=K5v0hQa6b7GiCLPmZm5YtHqd20yvQvIonetEoOetWwQ=; b=bB6rQF5T3y198fTkT1mHsWd/KGacxu4RPHcstWdgMb9qH8y5NZVeyOjUidlobhs0avtLf9GPh3nJoPKIzg9wrwOknRNm7uroT6DJ/y6kRUDUfRYcToGQJeAyCDql3P1xWD9Ns+i2KsMFWSvEQC54IfdUq3YzuFw1XSqaSCJ/ldG6arAiVyh7vffANRppgpAQGBwHV4THAxfn0C1+fMn5hrbBqkjAzqPQZMvOH8hfDwDgiDiz5mSE7Zaeheaor+Qpu3DdvtiMVvOdCHsQuPP1MFkOCoAhYylW3HsrLVWg17qxxo2tMHFjkHsXGeo5wX3BLjhcTeBXCumn4RrQE754Eg==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=COuPCYtfXJk3AEbDobTkJmFpM5jjZRFOOr642elFGP+8bx1AJm0D7+ZHZPDFsc3ABKLWQXsBTJiU7MGaJUE43PUs2kA0gj7C7w4P+TCXwITbnw18TVPpfCLk91Zi1cHuxiK/RzjVRiv8tkcNgKujuRl++1S0zzDYH1avaFfgWysNCo1KWvIw/Nc/2UBGAi16BbkQuveioY0YyPKMnSrsN6s/ZSLHxwnTMpc+n0nlmnIL8UFHgCgPLVzBg2Wrz5tAxdPdnJZooPeb7Rz4IMVRSGQOJ64OULxg3bEW7A3qBIqbLIu7uh9mHm3ywrlSukOlMo2vbz7VlkhZL8S8ocRzSQ==
  • Authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=suse.com;
  • Cc: 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
  • Delivery-date: Mon, 11 Jul 2022 06:32:32 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

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 ...

> --- 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®.