[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
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
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |