Re: [PATCH v3 2/7] libxc: Plumb Xen with vNUMA topology for domain.

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Tue, Nov 19, 2013 at 09:37:25AM +0100, Dario Faggioli wrote:
> On lun, 2013-11-18 at 15:24 -0500, Elena Ufimtseva wrote:
> > Per-domain vNUMA topology initialization.
> > domctl hypercall is used to set vNUMA topology
> > per domU during domain build time.
> > 
> > Signed-off-by: Elena Ufimtseva <ufimtseva@gmail.com>
> > 
> Ok, this one below should either go under its own "---"
> 
> > Changes since v2:
> > * modified error logic;
> > ---
> Or here.
> 
> I mean, something like the following:
> 
> "
>   bla bla changelog bal bla
> 
>   Signed-off-by: Elena...
>   ---
>   Changes since v2:
>    * xxx
>    * yyyy
>   ---
>   <diffstat>
> "
> 
> This way the "Changes since" part will be automatically kept out of the
> final committed changelog by the git commands committers use to commit
> the patch. ISTR this happens for everything that's below "---". If you
> put it _inside_ the actual changelog, they'll have to remove it by hand,
> which is annoying.
> 
> > diff --git a/tools/libxc/xc_domain.c b/tools/libxc/xc_domain.c
> > index 1ccafc5..198694f 100644
> 
> > +/* Plumbs Xen with vNUMA topology */
> > +int xc_domain_setvnuma(xc_interface *xch,
> > +                        uint32_t domid,
> > +                        uint16_t nr_vnodes,
> > +                        uint16_t nr_vcpus,
> > +                        vmemrange_t *vmemrange,
> > +                        unsigned int *vdistance,
> > +                        unsigned int *vcpu_to_vnode,
> > +                        unsigned int *vnode_to_pnode)
> > +{
> > +    int rc;
> > +    DECLARE_DOMCTL;
> > +    DECLARE_HYPERCALL_BOUNCE(vmemrange, sizeof(*vmemrange) * nr_vnodes,
> > +                                    XC_HYPERCALL_BUFFER_BOUNCE_BOTH);
> > +    DECLARE_HYPERCALL_BOUNCE(vdistance, sizeof(*vdistance) *
> > +                                    nr_vnodes * nr_vnodes,
> > +                                    XC_HYPERCALL_BUFFER_BOUNCE_BOTH);
> > +    DECLARE_HYPERCALL_BOUNCE(vcpu_to_vnode, sizeof(*vcpu_to_vnode) * nr_vcpus,
> > +                                    XC_HYPERCALL_BUFFER_BOUNCE_BOTH);
> > +    DECLARE_HYPERCALL_BOUNCE(vnode_to_pnode, sizeof(*vnode_to_pnode) *
> > +                                    nr_vnodes,
> > +                                    XC_HYPERCALL_BUFFER_BOUNCE_BOTH);
> >
> So, last round I suggested that these bounce buffers should have been
> _BOUNCE_IN, as information only flow toward Xen, and nothing gets copied
> back to xc (and neither it is from xc to the application).
> 
> However, Elena reported the system crashing (actually, what is it that
> was exactly happening, Elena?) if doing so.
> 
> I gave it another look, but I don't think I see a possible reason for
> that... Anyone? IanC? Andrew?
> 
> > +    if ( nr_vnodes == 0 ) {
> > +        errno = EINVAL;
> > +        return -1;
> > +    }
> > +
> > +    if ( vdistance == NULL || vcpu_to_vnode == NULL ||
> > +         vmemrange == NULL || vnode_to_pnode == NULL ) {
> > +        PERROR("Incorrect parameters for XEN_DOMCTL_setvnumainfo.\n");
> > +        errno = EINVAL;
> > +        return -1;
> > +    }
> > +    
> I think this line above has some spurious spaces.
> 
> > +    if ( xc_hypercall_bounce_pre(xch, vmemrange)      ||
> > +         xc_hypercall_bounce_pre(xch, vdistance)      ||
> > +         xc_hypercall_bounce_pre(xch, vcpu_to_vnode)  ||
> > +         xc_hypercall_bounce_pre(xch, vnode_to_pnode) ) {
> > +        PERROR("Could not bounce buffer for xc_domain_setvnuma.\n");
> > +        errno = EFAULT;
> > +        return -1;
> > +    }
> > +    
> And this one too (spurious spaces).
> 
> Also, while the setting of errno in the first two if-s is fine, in this
> case I think you should avoid doing it, as we rely on
> xc_hypercall_bounce_pre to have done so.
> 
> > +    set_xen_guest_handle(domctl.u.vnuma.vmemrange, vmemrange);
> > +    set_xen_guest_handle(domctl.u.vnuma.vdistance, vdistance);
> > +    set_xen_guest_handle(domctl.u.vnuma.vcpu_to_vnode, vcpu_to_vnode);
> > +    set_xen_guest_handle(domctl.u.vnuma.vnode_to_pnode, vnode_to_pnode);
> > +    
>        ^ and here
> 
> > +    domctl.cmd = XEN_DOMCTL_setvnumainfo;
> > +    domctl.domain = (domid_t)domid;
> > +    domctl.u.vnuma.nr_vnodes = nr_vnodes;
> > +    domctl.u.vnuma.__pad = 0;
> > +    
>        ^ here
> > +    rc = do_domctl(xch, &domctl);
> > +    
>        ^ here
> 
> > +    xc_hypercall_bounce_post(xch, vmemrange);
> > +    xc_hypercall_bounce_post(xch, vdistance);
> > +    xc_hypercall_bounce_post(xch, vcpu_to_vnode);
> > +    xc_hypercall_bounce_post(xch, vnode_to_pnode);
> > +    
>        ^ and here too.
> 
> I double checked that this isn't an issue with my MUA by importing the
> patch in this e-mail and I confirm it is not, it looks like there are a
> few trailing whitespaces around.
> 
> I went checking a couple of the other patches too, and found something
> similar. Can you fix this?
> 
> For the Linux patches, checkpatch is usually of great help. For Xen
> ones, I don't think we have anything like it (yet? :-D). However, for
> this specific problem (trailing whitespaces), even just looking at the
> patch with `git show' should make the issue evident. For instance, here
> I see them as big red rectangles.
> 

Or in vim you can have

syn match ErrorLeadSpace /^ \+/      " highlight any leading spaces
syn match ErrorTailSpace / \+$/      " highlight any trailing spaces
syn match Error80        /\%>80v.\+/ " highlight anything past 80

hi Error80 ctermbg=red
hi ErrorLeadSpace ctermbg=red
hi ErrorTailSpace ctermbg=red


And that will nicely point it to you.

> Regards,
> Dario
> 
> -- 
> <<This happens because I choose it to happen!>> (Raistlin Majere)
> -----------------------------------------------------------------
> Dario Faggioli, Ph.D, http://about.me/dario.faggioli
> Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)
> 



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