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

[Wei Liu <wei.liu2@citrix.com>]

On Fri, Jan 23, 2015 at 01:03:16PM +0000, Jan Beulich wrote:
> >>> On 23.01.15 at 12:13, <wei.liu2@citrix.com> 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.)
> 

No problem. I will fix this.

> > +        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))?
> 

This is from Elena's original patch. I missed this one. Will fix.

> > +
> > +            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.
> 

Fixed.

> > +                    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.
> 

Inner braces removed.

Wei.

> Jan