| Message ID | 20210923120236.3692135-20-wei.chen@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add device tree based NUMA support to Arm | expand |
On 23.09.2021 14:02, Wei Chen wrote: > VIRTUAL_BUG_ON that is using in phys_to_nid is an empty macro. This > results in two lines of error-checking code in phys_to_nid are not > actually working. It also covers up two compilation errors: > 1. error: ‘MAX_NUMNODES’ undeclared (first use in this function). > This is because MAX_NUMNODES is defined in xen/numa.h. > But asm/numa.h is a dependent file of xen/numa.h, we can't > include xen/numa.h in asm/numa.h. This error has been fixed > after we move phys_to_nid to xen/numa.h. This could easily be taken care of by move MAX_NUMNODES up ahead of the asm/numa.h inclusion point. And then the change here would become independent of the rest of the series (and could hence go in early). > 2. error: wrong type argument to unary exclamation mark. > This is becuase, 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 in VIRTUAL_BUG_ON to enable the two > lines of error-checking code. May I suggest to drop VIRTUAL_BUG_ON() and instead use ASSERT() directly? Jan
Hi Jan, > -----Original Message----- > From: Jan Beulich <jbeulich@suse.com> > Sent: 2022年1月18日 0:22 > To: Wei Chen <Wei.Chen@arm.com> > Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>; xen- > devel@lists.xenproject.org; sstabellini@kernel.org; julien@xen.org > Subject: Re: [PATCH 19/37] xen/x86: promote VIRTUAL_BUG_ON to ASSERT in > > On 23.09.2021 14:02, Wei Chen wrote: > > VIRTUAL_BUG_ON that is using in phys_to_nid is an empty macro. This > > results in two lines of error-checking code in phys_to_nid are not > > actually working. It also covers up two compilation errors: > > 1. error: ‘MAX_NUMNODES’ undeclared (first use in this function). > > This is because MAX_NUMNODES is defined in xen/numa.h. > > But asm/numa.h is a dependent file of xen/numa.h, we can't > > include xen/numa.h in asm/numa.h. This error has been fixed > > after we move phys_to_nid to xen/numa.h. > > This could easily be taken care of by move MAX_NUMNODES up ahead of > the asm/numa.h inclusion point. And then the change here would become > independent of the rest of the series (and could hence go in early). > > > 2. error: wrong type argument to unary exclamation mark. > > This is becuase, 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 in VIRTUAL_BUG_ON to enable the two > > lines of error-checking code. > > May I suggest to drop VIRTUAL_BUG_ON() and instead use ASSERT() > directly? > Sure! > Jan
diff --git a/xen/include/xen/numa.h b/xen/include/xen/numa.h index 51391a2440..1978e2be1b 100644 --- a/xen/include/xen/numa.h +++ b/xen/include/xen/numa.h @@ -38,7 +38,7 @@ struct node { extern int compute_hash_shift(struct node *nodes, int numnodes, nodeid_t *nodeids); -#define VIRTUAL_BUG_ON(x) +#define VIRTUAL_BUG_ON(x) ASSERT(!(x)) extern void numa_add_cpu(int cpu); extern void numa_init_array(void); @@ -75,7 +75,7 @@ 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); nid = memnodemap[paddr_to_pdx(addr) >> memnode_shift]; - VIRTUAL_BUG_ON(nid >= MAX_NUMNODES || !node_data[nid]); + VIRTUAL_BUG_ON(nid >= MAX_NUMNODES || !node_data[nid].node_spanned_pages); return nid; }
VIRTUAL_BUG_ON that is using in phys_to_nid is an empty macro. This results in two lines of error-checking code in phys_to_nid are not actually working. It also covers up two compilation errors: 1. error: ‘MAX_NUMNODES’ undeclared (first use in this function). This is because MAX_NUMNODES is defined in xen/numa.h. But asm/numa.h is a dependent file of xen/numa.h, we can't include xen/numa.h in asm/numa.h. This error has been fixed after we move phys_to_nid to xen/numa.h. 2. error: wrong type argument to unary exclamation mark. This is becuase, 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 in VIRTUAL_BUG_ON to enable the two lines of error-checking code. And fix the the left compilation errors by replacing !node_data[nid] to !node_data[nid].node_spanned_pages. Because when node_spanned_pages is 0, this node has no memory. numa_scan_node will print warning message for such kind of nodes: "Firmware Bug or mis-configured hardware?". Even Xen allows to online such kind of nodes. I still think it's impossible for phys_to_nid to return a no memory node for a physical address. Signed-off-by: Wei Chen <wei.chen@arm.com> --- xen/include/xen/numa.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)