[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 |