Re: [Xen-devel] [PATCH v4 01/21] xen: dump vNUMA information with debug key "u"

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 23.01.15 at 12:13, <wei.liu2@xxxxxxxxxx> wrote:
> @@ -408,6 +413,68 @@ static void dump_numa(unsigned char key)
>  
>          for_each_online_node ( i )
>              printk("    Node %u: %u\n", i, page_num_node[i]);
> +
> +        read_lock(&d->vnuma_rwlock);

I'm sorry, but no - on v3 I specifically said "don't you need to try to
acquire d->vnuma_rwlock for reading, and skip any printing if the
acquire attempt fails". I.e. this should read_trylock(). (Yes, there
are pre-existing bad examples [including the acquiring of
d->page_alloc_lock in this function], but I think we should stop this
practice.)

> +        if ( !d->vnuma )
> +        {
> +            read_unlock(&d->vnuma_rwlock);
> +            continue;
> +        }
> +
> +        vnuma = d->vnuma;
> +        printk("     %u vnodes, %u vcpus, guest physical layout:\n",
> +               vnuma->nr_vnodes, d->max_vcpus);
> +        for ( i = 0; i < vnuma->nr_vnodes; i++ )
> +        {
> +            unsigned int start_cpu = ~0U;
> +
> +            err = snprintf(keyhandler_scratch, 12, "%3u",
> +                    vnuma->vnode_to_pnode[i]);
> +            if ( err < 0 || vnuma->vnode_to_pnode[i] == NUMA_NO_NODE )
> +                strlcpy(keyhandler_scratch, "???", 3);

If you really want 3 question marks to appear, the number needs to
be > 3. Is there any reason not to pass the correct one, i.e.
ARRAY_SIZE(keyhandler_scratch) (or sizeof(keyhandler_scratch))?

> +
> +            printk("       %3u: pnode %s,", i, keyhandler_scratch);
> +
> +            printk(" vcpus ");
> +
> +            for ( j = 0; j < d->max_vcpus; j++ )
> +            {
> +                if ( !(j & 0x3f) )
> +                    process_pending_softirqs();
> +
> +                if ( vnuma->vcpu_to_vnode[j] == i )
> +                {
> +                    if ( start_cpu == ~0U )
> +                    {
> +                        printk("%d", j);
> +                        start_cpu = j;
> +                    }
> +                } else if ( start_cpu != ~0U ) {

Coding style.

> +                    if ( j - 1 != start_cpu )
> +                        printk("-%d ", j - 1);
> +                    else
> +                        printk(" ");
> +                    start_cpu = ~0U;
> +                }
> +            }
> +
> +            if ( start_cpu != ~0U  && start_cpu != j - 1 )
> +                printk("-%d", j - 1);
> +
> +            printk("\n");
> +
> +            for ( j = 0; j < vnuma->nr_vmemranges; j++ )
> +            {
> +                if ( vnuma->vmemrange[j].nid == i )
> +                {
> +                    printk("           %016"PRIx64" - %016"PRIx64"\n",
> +                           vnuma->vmemrange[j].start,
> +                           vnuma->vmemrange[j].end);
> +                }
> +            }

At least the inner (belonging to the if()) braces should be dropped
as being unnecessary.

Jan


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