[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



On Tue, Nov 19, 2013 at 9:03 AM, Konrad Rzeszutek Wilk
<konrad.wilk@xxxxxxxxxx> wrote:
> 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@xxxxxxxxx>
>> >
>> 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

True )

checkpatch also works with --types TRAILING_WHITESPACE or others options.

Ill clean these.


>
>
> 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@xxxxxxxxxxxxx
>> http://lists.xen.org/xen-devel
>



-- 
Elena

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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.