Re: [Xen-devel] [PATCH v11 3/4] vnuma hook to debug-keys u

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 05.09.14 at 06:06, <ufimtseva@xxxxxxxxx> wrote:
> Add debug-keys hook to display vnuma topology.
> 
> Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> 
> Signed-off-by: Elena Ufimtseva <ufimtseva@xxxxxxxxx>

I was about to apply this but apart from the tags above being
mis-ordered, I think the code here has become stale.

> --- a/xen/arch/x86/numa.c
> +++ b/xen/arch/x86/numa.c
> @@ -357,10 +357,11 @@ EXPORT_SYMBOL(node_data);
>  static void dump_numa(unsigned char key)
>  {
>      s_time_t now = NOW();
> -    int i;
> +    int i, j, err, n;
>      struct domain *d;
>      struct page_info *page;
>      unsigned int page_num_node[MAX_NUMNODES];
> +    uint64_t mem;

Please move these new variables into the for() scope below, and
have j and n be unsigned..

> @@ -402,6 +403,38 @@ static void dump_numa(unsigned char key)
>  
>          for_each_online_node ( i )
>              printk("    Node %u: %u\n", i, page_num_node[i]);
> +
> +        if ( !d->vnuma )
> +            continue;
> +
> +        printk("   %u vnodes, %u vcpus\n", d->vnuma->nr_vnodes, 
> d->max_vcpus);

Is the vCPU count really relevant here?

> +        for ( i = 0; i < d->vnuma->nr_vnodes; i++ )

So you iterate over nr_vnodes ...

> +        {
> +            err = snprintf(keyhandler_scratch, 12, "%u",
> +                           d->vnuma->vnode_to_pnode[i]);
> +            if ( err < 0 || d->vnuma->vnode_to_pnode[i] == NUMA_NO_NODE )
> +                snprintf(keyhandler_scratch, 3, "???");
> +
> +            printk("        vnode %3u - pnode %s,", i, keyhandler_scratch);

... and print the vnode here ...

> +            mem = d->vnuma->vmemrange[i].end - d->vnuma->vmemrange[i].start;

but index vmemrange[] here.

> +            printk(" %"PRIu64" MB, ", mem >> 20);
> +
> +            printk("vcpu nrs: ");

What is "nrs" here? Do you mean "vnodes"? (If at all, the printing of
the vCPU count should go here I think.)

And finally you're not read_trylock()-ing the respective lock, so you
risk races with updates to the info.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel