From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dario Faggioli Subject: Re: [PATCH v6 for Xen 4.7 1/4] xen: enable per-VCPU parameter settings for RTDS scheduler Date: Mon, 7 Mar 2016 18:53:01 +0100 Message-ID: <1457373181.3102.74.camel@citrix.com> References: <1457286958-5427-1-git-send-email-lichong659@gmail.com> <1457286958-5427-2-git-send-email-lichong659@gmail.com> <56DD894802000078000D9F84@prv-mh.provo.novell.com> <56DDBCFF02000078000DA1A0@prv-mh.provo.novell.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============6823928770880592182==" Return-path: In-Reply-To: <56DDBCFF02000078000DA1A0@prv-mh.provo.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xen.org Sender: "Xen-devel" To: Jan Beulich , Chong Li Cc: Chong Li , Sisu Xi , George Dunlap , xen-devel , Meng Xu , Dagaen Golomb List-Id: xen-devel@lists.xenproject.org --===============6823928770880592182== Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="=-oDRlr+UOc7EW/CmzprYC" --=-oDRlr+UOc7EW/CmzprYC Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Mon, 2016-03-07 at 09:40 -0700, Jan Beulich wrote: > > > > On 07.03.16 at 17:28, wrote: > > On Mon, Mar 7, 2016 at 6:59 AM, Jan Beulich > > wrote: > > >=C2=A0 > > > > @@ -1163,6 +1173,96 @@ rt_dom_cntl( > > > >=C2=A0 > > > > +=C2=A0=C2=A0=C2=A0=C2=A0case XEN_DOMCTL_SCHEDOP_getvcpuinfo: > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if ( guest_handle_= is_null(op->u.v.vcpus) ) > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0{ > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0rc =3D -EINVAL; > > > Perhaps rather -EFAULT? But then again - what is this check good > > > for > > > (considering that it doesn't cover other obviously bad handle > > > values)? > > Dario suggested this in the last post, because vcpus is a handle > > and > > needs to be validated. > > Well, as said - the handle being non-null doesn't make it a valid > handle. Any validation can be left to copy_{to,from}_guest*() > unless you mean to give a null handle some special meaning. >=20 IIRC, I was looking at how=C2=A0XEN_SYSCTL_pcitopoinfo is handled, for reference, and that has some guest_handle_is_null()=3D=3D>EINVAL sainity checking (in xen/common/sysctl.c), which, when I thought about it, made sense to me. My reasoning was, sort of: =C2=A01. if the handle is NULL, no point getting into the somewhat=C2=A0 =C2=A0 =C2=A0 complicated logic of the while, =C2=A02. more accurate error reporting: as being passed a NULL handler=C2= =A0 =C2=A0 =C2=A0 looked something we could identify and call invalid, rather t= han=C2=A0 =C2=A0 =C2=A0 waiting for the copy to fault. In any event, I've no problem at all with this being dropped. > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0{ > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0rc =3D -EINVAL; > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0break; > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0} > > > > + > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0spin_lock_irqsave(&prv->lock, flags); > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0svc =3D rt_vcpu(d->vcpu[local_sched.vcpuid]); > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0local_sched.s.rtds.budget =3D svc->budget / > > > > MICROSECS(1); > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0local_sched.s.rtds.period =3D svc->period / > > > > MICROSECS(1); > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0spin_unlock_irqrestore(&prv->lock, flags); > > > > + > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0if ( __copy_to_guest_offset(op->u.v.vcpus, index, > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0&local_sched, 1) ) > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0{ > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0rc =3D -EFAULT; > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0break; > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0} > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0if ( (++index > 0x3f) && hypercall_preempt_check() > > > > ) > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0break; > > > So how is the caller going to be able to reliably read all vCPU- > > > s' > > > information for a guest with more than 64 vCPU-s? > > In libxc, we re-issue hypercall if the current one is preempted. > And with the current code - how does libxc know? (And anyway, > this should only be a last resort, if the hypervisor can't by itself > arrange for a continuation. If done this way, having a code > comment referring to the required caller behavior would seem to > be an absolute must.) >=20 I definitely agree on commenting. About the structure of the code, as said above, I do like how=C2=A0XEN_SYSCTL_pcitopoinfo ended up being handled, I think it is a great fit for this specific case and, comparing at both this and previous version, I do think this one is (bugs apart) looking better. I'm sure I said this --long ago-- when discussing v4 (and maybe even previous versions), as well as more recently, when reviewing v5, and that's why Chong (finally! :-D) did it. So, with the comment in place (and with bugs fixed :-)), are you (Jan) ok with this being done this way? > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0} > > > > + > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if ( !rc && (op->u= .v.nr_vcpus !=3D index) ) > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0op->u.v.nr_vcpus =3D index; > > > I don't think the right side of the && is really necessary / > > > useful. > > The right side is to check whether the vcpus array is fully > > processed. > > When it is true and no error occurs (rc =3D=3D 0), we > > update op->u.v.nr_vcpus, which is returned to libxc, and helps xc > > function figuring out how many un-processed vcpus should > > be taken care of in the next hypercall. > Just consider what the contents of op->u.v.nr_vcpus is after > this piece of code was executed, once with the full conditional, > and another time with the right side of the && omitted. >=20 BTW, Chong, I'm not sure this has to do with what Jan is saying, but looking again at=C2=A0XEN_SYSCTL_pcitopoinfo, it looks to me you're missing copying nr_vcpus back up to the guest (which is actually what makes libxc knows whether all vcpus have been processed or now). Regards, Dario --=20 <> (Raistlin Majere) ----------------------------------------------------------------- Dario Faggioli, Ph.D, http://about.me/dario.faggioli Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK) --=-oDRlr+UOc7EW/CmzprYC Content-Type: application/pgp-signature; name="signature.asc" Content-Description: This is a digitally signed message part Content-Transfer-Encoding: 7bit -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iEYEABECAAYFAlbdv/0ACgkQk4XaBE3IOsRzygCeJC+4gPQsWSgERS1Bforbsr2F 1I4An3oTFDhkxNnquJle/lK7XxxZ0IV3 =Tvmh -----END PGP SIGNATURE----- --=-oDRlr+UOc7EW/CmzprYC-- --===============6823928770880592182== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KWGVuLWRldmVs IG1haWxpbmcgbGlzdApYZW4tZGV2ZWxAbGlzdHMueGVuLm9yZwpodHRwOi8vbGlzdHMueGVuLm9y Zy94ZW4tZGV2ZWwK --===============6823928770880592182==--