|
[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
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |