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

[Dario Faggioli <dario.faggioli@citrix.com>]

[-- Attachment #1.1: Type: text/plain, Size: 5322 bytes --]

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.

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)


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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