On 02.09.2022 05:31, Wei Chen wrote:
> --- /dev/null
> +++ b/xen/common/numa.c
> @@ -0,0 +1,442 @@
> +/*
> + * Generic VM initialization for NUMA setups.
> + * Copyright 2002,2003 Andi Kleen, SuSE Labs.
> + * Adapted for Xen: Ryan Harper <ry...@us.ibm.com>
> + */
> +
> +#include <xen/init.h>
> +#include <xen/keyhandler.h>
> +#include <xen/mm.h>
> +#include <xen/nodemask.h>
> +#include <xen/numa.h>
> +#include <xen/param.h>
> +#include <xen/sched.h>
> +#include <xen/softirq.h>
> +
> +struct node_data __ro_after_init node_data[MAX_NUMNODES];
> +
> +/* Mapping from pdx to node id */
> +unsigned int __ro_after_init memnode_shift;
> +unsigned long __ro_after_init memnodemapsize;
> +uint8_t *__ro_after_init memnodemap;
> +static uint8_t __ro_after_init _memnodemap[64];
These last two want to use nodeid_t instead of uint8_t. Originally
the latter used typeof(*memnodemap) for (I think - iirc it was me who
made it that way) this reason: That way correcting memnodemap's type
would have propagated without the need for further adjustments.
> +nodeid_t __read_mostly cpu_to_node[NR_CPUS] = {
> + [0 ... NR_CPUS-1] = NUMA_NO_NODE
> +};
> +
> +cpumask_t __read_mostly node_to_cpumask[MAX_NUMNODES];
> +
> +nodemask_t __read_mostly node_online_map = { { [0] = 1UL } };
> +
> +bool __read_mostly numa_off;
The v3 review discussing this possibly becoming __ro_after_init didn't
really finish (you didn't reply to my latest request there), but you
also didn't change the attribute. Please clarify.
> +static int __init populate_memnodemap(const struct node *nodes,
> + unsigned int numnodes, unsigned int
> shift,
> + nodeid_t *nodeids)
Can't this be pointer-to-const, and then also in the caller?
> +static unsigned int __init extract_lsb_from_nodes(const struct node *nodes,
> + nodeid_t numnodes)
> +{
> + unsigned int i;
> + nodeid_t nodes_used = 0;
This once again is a variable which doesn't really hold a node ID (but
instead is a counter), and hence would better be unsigned int (see
./CODING_STYLE).
> + unsigned long spdx, epdx;
> + unsigned long bitfield = 0, memtop = 0;
> +
> + for ( i = 0; i < numnodes; i++ )
> + {
> + spdx = paddr_to_pdx(nodes[i].start);
> + epdx = paddr_to_pdx(nodes[i].end - 1) + 1;
> + if ( spdx >= epdx )
> + continue;
> + bitfield |= spdx;
> + nodes_used++;
> + if ( epdx > memtop )
> + memtop = epdx;
> + }
> + if ( nodes_used <= 1 )
> + i = BITS_PER_LONG - 1;
> + else
> + i = find_first_bit(&bitfield, sizeof(unsigned long)*8);
Please add the missing blanks around * .
> + memnodemapsize = (memtop >> i) + 1;
> + return i;
Please add the missing blank line before the (main) return statement
of the function.
> +int __init compute_hash_shift(const struct node *nodes,
> + nodeid_t numnodes, nodeid_t *nodeids)
While I agree that nodeid_t can hold all necessary values, I still
don't think a cound should be expressed by nodeid_t. As above - see
./CODING_STYLE.
> +{
> + unsigned int shift;
> +
> + shift = extract_lsb_from_nodes(nodes, numnodes);
> + if ( memnodemapsize <= ARRAY_SIZE(_memnodemap) )
> + memnodemap = _memnodemap;
> + else if ( allocate_cachealigned_memnodemap() )
> + return -1;
> + printk(KERN_DEBUG "NUMA: Using %d for the hash shift.\n", shift);
This wants to be %u now. I'd also like to ask to drop the full stop
at this occasion.
> + if ( populate_memnodemap(nodes, numnodes, shift, nodeids) != 1 )
> + {
> + printk(KERN_INFO "Your memory is not aligned you need to "
> + "rebuild your hypervisor with a bigger NODEMAPSIZE "
> + "shift=%d\n", shift);
Again %u please.
> +/* initialize NODE_DATA given nodeid and start/end */
> +void __init setup_node_bootmem(nodeid_t nodeid, paddr_t start, paddr_t end)
Please capitalize the first letter of the comment (see ./CODING_STYLE).
> +void __init numa_init_array(void)
> +{
> + unsigned int i;
> + nodeid_t rr;
> +
> + /*
> + * There are unfortunately some poorly designed mainboards around
> + * that only connect memory to a single CPU. This breaks the 1:1
> cpu->node
> + * mapping. To avoid this fill in the mapping for all possible
> + * CPUs, as the number of CPUs is not known yet.
> + * We round robin the existing nodes.
> + */
Along with the style correction re-flowing of the text would have been
nice, such the lines aren't wrapped seemingly randomly without utilizing
permitted line length.
> +void __init numa_initmem_init(unsigned long start_pfn, unsigned long end_pfn)
> +{
> + unsigned int i;
> + paddr_t start = pfn_to_paddr(start_pfn);
> + paddr_t end = pfn_to_paddr(end_pfn);
> +
> +#ifdef CONFIG_NUMA_EMU
> + if ( numa_fake && !numa_emulation(start_pfn, end_pfn) )
> + return;
> +#endif
> +
> +#ifdef CONFIG_NUMA
> + if ( !numa_off && !numa_scan_nodes(start, end) )
> + return;
> +#endif
> +
> + printk(KERN_INFO "%s\n",
> + numa_off ? "NUMA turned off" : "No NUMA configuration found");
> +
> + printk(KERN_INFO "Faking a node at %"PRIpaddr"-%"PRIpaddr"\n",
> + start, end);
> + /* setup dummy node covering all memory */
Please again capitalize the first character of the comment.
> +static void cf_check dump_numa(unsigned char key)
> +{
> + s_time_t now = NOW();
> + unsigned int i, j, n;
> + struct domain *d;
> + struct page_info *page;
Along with the various other style corrections perhaps add const here?
> + unsigned int page_num_node[MAX_NUMNODES];
> + const struct vnuma_info *vnuma;
> +
> + printk("'%c' pressed -> dumping numa info (now = %"PRI_stime")\n", key,
> + now);
> +
> + for_each_online_node ( i )
> + {
> + paddr_t pa = pfn_to_paddr(node_start_pfn(i) + 1);
> +
> + printk("NODE%u start->%lu size->%lu free->%lu\n",
> + i, node_start_pfn(i), node_spanned_pages(i),
> + avail_node_heap_pages(i));
> + /* sanity check phys_to_nid() */
First char of comment again.
Jan
