Re: [PATCH v6 2/6] xen/x86: move generically usable NUMA code from x86 to common

[Jan Beulich <jbeulich@xxxxxxxx>]

On 11.10.2022 13:17, Wei Chen wrote:
> v5 -> v6:
>  1. Replace numa_scan_node to numa_process_nodes in commit log.
>  2. Limit the scope of page_num_node, vnuma and page of numa_setup
>     function.
>  3. Use memset to init page_num_node instead of for_each_online_node.
>  4. Use %u instead of %d for nodeid_t and j in numa_setup print
>     messages.

The only instances of nodeid_t typed variable uses that I was able to
find are in dump_numa(). I guess you can leave them that way, but
strictly speaking %u isn't correct to use for nodeid_t (as it promotes
to int, not to unsigned int).

>  5. Use min(PADDR_BITS, BITS_PER_LONG - 1) to calculate the shift
>     when only one node is in the system.

This change needs mentioning / justifying in the description.

> +static void cf_check dump_numa(unsigned char key)
> +{
> +    s_time_t now = NOW();
> +    unsigned int i, j, n;
> +    struct domain *d;
> +
> +    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() */
> +        if ( phys_to_nid(pa) != i )
> +            printk("phys_to_nid(%"PRIpaddr") -> %u should be %u\n",
> +                   pa, phys_to_nid(pa), i);
> +    }
> +
> +    j = cpumask_first(&cpu_online_map);
> +    n = 0;
> +    for_each_online_cpu ( i )
> +    {
> +        if ( i != j + n || cpu_to_node[j] != cpu_to_node[i] )
> +        {
> +            if ( n > 1 )
> +                printk("CPU%u...%u -> NODE%u\n", j, j + n - 1, 
> cpu_to_node[j]);
> +            else
> +                printk("CPU%u -> NODE%u\n", j, cpu_to_node[j]);
> +            j = i;
> +            n = 1;
> +        }
> +        else
> +            ++n;
> +    }
> +    if ( n > 1 )
> +        printk("CPU%u...%u -> NODE%u\n", j, j + n - 1, cpu_to_node[j]);
> +    else
> +        printk("CPU%u -> NODE%u\n", j, cpu_to_node[j]);
> +
> +    rcu_read_lock(&domlist_read_lock);
> +
> +    printk("Memory location of each domain:\n");
> +    for_each_domain ( d )
> +    {
> +        const struct page_info *page;
> +        unsigned int page_num_node[MAX_NUMNODES];
> +        const struct vnuma_info *vnuma;
> +
> +        process_pending_softirqs();
> +
> +        printk("Domain %u (total: %u):\n", d->domain_id, 
> domain_tot_pages(d));

Perhaps switch to using %pd here?

> +        memset(page_num_node, 0, sizeof(unsigned int) * MAX_NUMNODES);

Simply (and less fragile) sizeof(page_num_node)?

Jan