From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dario Faggioli Subject: Re: [PATCH v3 for Xen 4.6 1/4] xen: enable per-VCPU parameter settings for RTDS scheduler Date: Tue, 7 Jul 2015 16:55:53 +0200 Message-ID: <1436280953.22672.119.camel@citrix.com> References: <1435545899-22751-1-git-send-email-chong.li@wustl.edu> <1435545899-22751-2-git-send-email-chong.li@wustl.edu> <559BB101020000780008D371@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============6449790542632522244==" Return-path: In-Reply-To: <559BB101020000780008D371@mail.emea.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: Chong Li , Sisu Xi , george.dunlap@eu.citrix.com, xen-devel@lists.xen.org, Meng Xu , Chong Li , dgolomb@seas.upenn.edu List-Id: xen-devel@lists.xenproject.org --===============6449790542632522244== Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="=-XzJwqa1OdKYnPRcfwFCB" --=-XzJwqa1OdKYnPRcfwFCB Content-Type: multipart/mixed; boundary="=-/9Ra74dH/amd+IvcNQ9S" --=-/9Ra74dH/amd+IvcNQ9S Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Tue, 2015-07-07 at 09:59 +0100, Jan Beulich wrote: > >>> On 29.06.15 at 04:44, wrote: > > --- a/xen/common/Makefile > > +++ b/xen/common/Makefile > > @@ -31,7 +31,6 @@ obj-y +=3D rbtree.o > > obj-y +=3D rcupdate.o > > obj-y +=3D sched_credit.o > > obj-y +=3D sched_credit2.o > > -obj-y +=3D sched_sedf.o > > obj-y +=3D sched_arinc653.o > > obj-y +=3D sched_rt.o > > obj-y +=3D schedule.o >=20 > Stray change. Or perhaps the file doesn't build anymore, in which case > you should instead have stated that the patch is dependent upon the > series removing SEDF. >=20 > > @@ -1157,8 +1158,75 @@ rt_dom_cntl( > > list_for_each( iter, &sdom->vcpu ) > > { > > struct rt_vcpu * svc =3D list_entry(iter, struct rt_vcpu, = sdom_elem); > > - svc->period =3D MICROSECS(op->u.rtds.period); /* transfer = to nanosec */ > > - svc->budget =3D MICROSECS(op->u.rtds.budget); > > + svc->period =3D MICROSECS(op->u.d.rtds.period); /* transfe= r to nanosec */ > > + svc->budget =3D MICROSECS(op->u.d.rtds.budget); > > + } > > + spin_unlock_irqrestore(&prv->lock, flags); > > + break; > > + case XEN_DOMCTL_SCHEDOP_getvcpuinfo: > > + spin_lock_irqsave(&prv->lock, flags); > > + for( index =3D 0; index < op->u.v.nr_vcpus; index++ ) >=20 > Coding style (more further down). >=20 > > + { > > + if ( copy_from_guest_offset(&local_sched, > > + op->u.v.vcpus, index, 1) ) >=20 > Indentation. >=20 > > + { > > + rc =3D -EFAULT; > > + break; > > + } > > + if ( local_sched.vcpuid >=3D d->max_vcpus > > + || d->vcpu[local_sched.vcpuid] =3D=3D NULL ) >=20 > || belongs at the end of the first line. Indentation. >=20 > > + { > > + rc =3D -EINVAL; > > + break; > > + } > > + svc =3D rt_vcpu(d->vcpu[local_sched.vcpuid]); > > + > > + local_sched.vcpuid =3D svc->vcpu->vcpu_id; >=20 > Why? If at all, this should be an ASSERT(). >=20 > > + local_sched.s.rtds.budget =3D svc->budget / MICROSECS(1); > > + local_sched.s.rtds.period =3D svc->period / MICROSECS(1); > > + if( index >=3D op->u.v.nr_vcpus ) /* not enough guest buff= er*/ >=20 > Impossible due to the containing loop's condition. >=20 > > + { > > + rc =3D -ENOBUFS; > > + break; > > + } > > + if ( copy_to_guest_offset(op->u.v.vcpus, index, >=20 > __copy_to_guest_offset() >=20 > > + case XEN_DOMCTL_SCHEDOP_putvcpuinfo: > > + spin_lock_irqsave(&prv->lock, flags); > > + for( index =3D 0; index < op->u.v.nr_vcpus; index++ ) > > + { > > + if ( copy_from_guest_offset(&local_sched, > > + op->u.v.vcpus, index, 1) ) > > + { > > + rc =3D -EFAULT; > > + break; > > + } > > + if ( local_sched.vcpuid >=3D d->max_vcpus > > + || d->vcpu[local_sched.vcpuid] =3D=3D NULL ) > > + { > > + rc =3D -EINVAL; > > + break; > > + } > > + svc =3D rt_vcpu(d->vcpu[local_sched.vcpuid]); > > + svc->period =3D MICROSECS(local_sched.s.rtds.period); > > + svc->budget =3D MICROSECS(local_sched.s.rtds.budget); >=20 > Are all input values valid here? >=20 > > --- a/xen/common/schedule.c > > +++ b/xen/common/schedule.c > > @@ -65,7 +65,6 @@ DEFINE_PER_CPU(struct schedule_data, schedule_data); > > DEFINE_PER_CPU(struct scheduler *, scheduler); > > =20 > > static const struct scheduler *schedulers[] =3D { > > - &sched_sedf_def, > > &sched_credit_def, > > &sched_credit2_def, > > &sched_arinc653_def, >=20 > See above. >=20 > > @@ -1054,7 +1053,9 @@ long sched_adjust(struct domain *d, struct xen_do= mctl_scheduler_op *op) > > =20 > > if ( (op->sched_id !=3D DOM2OP(d)->sched_id) || > > ((op->cmd !=3D XEN_DOMCTL_SCHEDOP_putinfo) && > > - (op->cmd !=3D XEN_DOMCTL_SCHEDOP_getinfo)) ) > > + (op->cmd !=3D XEN_DOMCTL_SCHEDOP_getinfo) && > > + (op->cmd !=3D XEN_DOMCTL_SCHEDOP_putvcpuinfo) && > > + (op->cmd !=3D XEN_DOMCTL_SCHEDOP_getvcpuinfo)) ) > > return -EINVAL; >=20 > Convert to switch() please. >=20 > > --- a/xen/include/public/domctl.h > > +++ b/xen/include/public/domctl.h > > @@ -330,31 +330,59 @@ DEFINE_XEN_GUEST_HANDLE(xen_domctl_max_vcpus_t); > > #define XEN_SCHEDULER_ARINC653 7 > > #define XEN_SCHEDULER_RTDS 8 > > =20 > > +typedef struct xen_domctl_sched_sedf { > > + uint64_aligned_t period; > > + uint64_aligned_t slice; > > + uint64_aligned_t latency; > > + uint32_t extratime; > > + uint32_t weight; > > +} xen_domctl_sched_sedf_t; >=20 > Indentation. >=20 > > +typedef union xen_domctl_schedparam { > > + xen_domctl_sched_sedf_t sedf; > > + xen_domctl_sched_credit_t credit; > > + xen_domctl_sched_credit2_t credit2; > > + xen_domctl_sched_rtds_t rtds; > > +} xen_domctl_schedparam_t; >=20 > I don't see the need for this extra wrapper type. Nor do I see the > need for the typedef here and above - they're generally only > created if you want to also define a matching guest handle type. >=20 > > +typedef struct xen_domctl_schedparam_vcpu { > > + union { > > + xen_domctl_sched_credit_t credit; > > + xen_domctl_sched_credit2_t credit2; > > + xen_domctl_sched_rtds_t rtds; > > + } s; > > + uint16_t vcpuid; >=20 > Explicit padding please. >=20 > > +} xen_domctl_schedparam_vcpu_t; > > +DEFINE_XEN_GUEST_HANDLE(xen_domctl_schedparam_vcpu_t); > > + > > /* Set or get info? */ > > #define XEN_DOMCTL_SCHEDOP_putinfo 0 > > #define XEN_DOMCTL_SCHEDOP_getinfo 1 > > +#define XEN_DOMCTL_SCHEDOP_putvcpuinfo 2 > > +#define XEN_DOMCTL_SCHEDOP_getvcpuinfo 3 > > struct xen_domctl_scheduler_op { > > uint32_t sched_id; /* XEN_SCHEDULER_* */ > > uint32_t cmd; /* XEN_DOMCTL_SCHEDOP_* */ > > union { > > - struct xen_domctl_sched_sedf { > > - uint64_aligned_t period; > > - uint64_aligned_t slice; > > - uint64_aligned_t latency; > > - uint32_t extratime; > > - uint32_t weight; > > - } sedf; > > - struct xen_domctl_sched_credit { > > - uint16_t weight; > > - uint16_t cap; > > - } credit; > > - struct xen_domctl_sched_credit2 { > > - uint16_t weight; > > - } credit2; > > - struct xen_domctl_sched_rtds { > > - uint32_t period; > > - uint32_t budget; > > - } rtds; > > + xen_domctl_schedparam_t d; >=20 > With this type gone I'm not even sure we need to wrap this in > another union; not doing so would eliminate some of the other > changes in this patch. >=20 So, Jan, just to be sure, do you mean (apart from the explicit padding) something like this (attached, also)? diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h index bc45ea5..8210ecb 100644 --- a/xen/include/public/domctl.h +++ b/xen/include/public/domctl.h @@ -330,31 +330,56 @@ DEFINE_XEN_GUEST_HANDLE(xen_domctl_max_vcpus_t); #define XEN_SCHEDULER_ARINC653 7 #define XEN_SCHEDULER_RTDS 8 =20 +struct xen_domctl_sched_sedf { + uint64_aligned_t period; + uint64_aligned_t slice; + uint64_aligned_t latency; + uint32_t extratime; + uint32_t weight; +}; + +struct xen_domctl_sched_credit { + uint16_t weight; + uint16_t cap; +}; + +struct xen_domctl_sched_credit2 { + uint16_t weight; +}; + +struct xen_domctl_sched_rtds { + uint32_t period; + uint32_t budget; +}; + +typedef struct xen_domctl_schedparam_vcpu { + union { + struct xen_domctl_sched_sedf sedf; + struct xen_domctl_sched_credit credit; + struct xen_domctl_sched_credit2 credit2; + struct xen_domctl_sched_rtds rtds; + } s; + uint16_t vcpuid; +} xen_domctl_schedparam_vcpu_t; +DEFINE_XEN_GUEST_HANDLE(xen_domctl_schedparam_vcpu_t); + /* Set or get info? */ #define XEN_DOMCTL_SCHEDOP_putinfo 0 #define XEN_DOMCTL_SCHEDOP_getinfo 1 +#define XEN_DOMCTL_SCHEDOP_putvcpuinfo 2 +#define XEN_DOMCTL_SCHEDOP_getvcpuinfo 3 struct xen_domctl_scheduler_op { uint32_t sched_id; /* XEN_SCHEDULER_* */ uint32_t cmd; /* XEN_DOMCTL_SCHEDOP_* */ union { - struct xen_domctl_sched_sedf { - uint64_aligned_t period; - uint64_aligned_t slice; - uint64_aligned_t latency; - uint32_t extratime; - uint32_t weight; - } sedf; - struct xen_domctl_sched_credit { - uint16_t weight; - uint16_t cap; - } credit; - struct xen_domctl_sched_credit2 { - uint16_t weight; - } credit2; - struct xen_domctl_sched_rtds { - uint32_t period; - uint32_t budget; - } rtds; + struct xen_domctl_sched_sedf sedf; + struct xen_domctl_sched_credit credit; + struct xen_domctl_sched_credit2 credit2; + struct xen_domctl_sched_rtds rtds; + struct { + XEN_GUEST_HANDLE_64(xen_domctl_schedparam_vcpu_t) vcpus; + uint16_t nr_vcpus; + } v; } u; }; typedef struct xen_domctl_scheduler_op xen_domctl_scheduler_op_t; If yes, I'd be fine with it too (George?) Thanks and Regards, Dario --=20 <> (Raistlin Majere) ----------------------------------------------------------------- Dario Faggioli, Ph.D, http://about.me/dario.faggioli Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK) --=-/9Ra74dH/amd+IvcNQ9S Content-Disposition: attachment; filename="rtds-domctl.patch" Content-Type: text/x-patch; name="rtds-domctl.patch"; charset="UTF-8" Content-Transfer-Encoding: base64 ZGlmZiAtLWdpdCBhL3hlbi9pbmNsdWRlL3B1YmxpYy9kb21jdGwuaCBiL3hlbi9pbmNsdWRlL3B1 YmxpYy9kb21jdGwuaAppbmRleCBiYzQ1ZWE1Li44MjEwZWNiIDEwMDY0NAotLS0gYS94ZW4vaW5j bHVkZS9wdWJsaWMvZG9tY3RsLmgKKysrIGIveGVuL2luY2x1ZGUvcHVibGljL2RvbWN0bC5oCkBA IC0zMzAsMzEgKzMzMCw1NiBAQCBERUZJTkVfWEVOX0dVRVNUX0hBTkRMRSh4ZW5fZG9tY3RsX21h eF92Y3B1c190KTsKICNkZWZpbmUgWEVOX1NDSEVEVUxFUl9BUklOQzY1MyA3CiAjZGVmaW5lIFhF Tl9TQ0hFRFVMRVJfUlREUyAgICAgOAogCitzdHJ1Y3QgeGVuX2RvbWN0bF9zY2hlZF9zZWRmIHsK KyAgICB1aW50NjRfYWxpZ25lZF90IHBlcmlvZDsKKyAgICB1aW50NjRfYWxpZ25lZF90IHNsaWNl OworICAgIHVpbnQ2NF9hbGlnbmVkX3QgbGF0ZW5jeTsKKyAgICB1aW50MzJfdCBleHRyYXRpbWU7 CisgICAgdWludDMyX3Qgd2VpZ2h0OworfTsKKworc3RydWN0IHhlbl9kb21jdGxfc2NoZWRfY3Jl ZGl0IHsKKyAgICB1aW50MTZfdCB3ZWlnaHQ7CisgICAgdWludDE2X3QgY2FwOworfTsKKworc3Ry dWN0IHhlbl9kb21jdGxfc2NoZWRfY3JlZGl0MiB7CisgICAgdWludDE2X3Qgd2VpZ2h0OworfTsK Kworc3RydWN0IHhlbl9kb21jdGxfc2NoZWRfcnRkcyB7CisgICAgdWludDMyX3QgcGVyaW9kOwor ICAgIHVpbnQzMl90IGJ1ZGdldDsKK307CisKK3R5cGVkZWYgc3RydWN0IHhlbl9kb21jdGxfc2No ZWRwYXJhbV92Y3B1IHsKKyAgICB1bmlvbiB7CisgICAgICAgIHN0cnVjdCB4ZW5fZG9tY3RsX3Nj aGVkX3NlZGYgc2VkZjsKKyAgICAgICAgc3RydWN0IHhlbl9kb21jdGxfc2NoZWRfY3JlZGl0IGNy ZWRpdDsKKyAgICAgICAgc3RydWN0IHhlbl9kb21jdGxfc2NoZWRfY3JlZGl0MiBjcmVkaXQyOwor ICAgICAgICBzdHJ1Y3QgeGVuX2RvbWN0bF9zY2hlZF9ydGRzIHJ0ZHM7CisgICAgfSBzOworICAg IHVpbnQxNl90IHZjcHVpZDsKK30geGVuX2RvbWN0bF9zY2hlZHBhcmFtX3ZjcHVfdDsKK0RFRklO RV9YRU5fR1VFU1RfSEFORExFKHhlbl9kb21jdGxfc2NoZWRwYXJhbV92Y3B1X3QpOworCiAvKiBT ZXQgb3IgZ2V0IGluZm8/ICovCiAjZGVmaW5lIFhFTl9ET01DVExfU0NIRURPUF9wdXRpbmZvIDAK ICNkZWZpbmUgWEVOX0RPTUNUTF9TQ0hFRE9QX2dldGluZm8gMQorI2RlZmluZSBYRU5fRE9NQ1RM X1NDSEVET1BfcHV0dmNwdWluZm8gMgorI2RlZmluZSBYRU5fRE9NQ1RMX1NDSEVET1BfZ2V0dmNw dWluZm8gMwogc3RydWN0IHhlbl9kb21jdGxfc2NoZWR1bGVyX29wIHsKICAgICB1aW50MzJfdCBz Y2hlZF9pZDsgIC8qIFhFTl9TQ0hFRFVMRVJfKiAqLwogICAgIHVpbnQzMl90IGNtZDsgICAgICAg LyogWEVOX0RPTUNUTF9TQ0hFRE9QXyogKi8KICAgICB1bmlvbiB7Ci0gICAgICAgIHN0cnVjdCB4 ZW5fZG9tY3RsX3NjaGVkX3NlZGYgewotICAgICAgICAgICAgdWludDY0X2FsaWduZWRfdCBwZXJp b2Q7Ci0gICAgICAgICAgICB1aW50NjRfYWxpZ25lZF90IHNsaWNlOwotICAgICAgICAgICAgdWlu dDY0X2FsaWduZWRfdCBsYXRlbmN5OwotICAgICAgICAgICAgdWludDMyX3QgZXh0cmF0aW1lOwot ICAgICAgICAgICAgdWludDMyX3Qgd2VpZ2h0OwotICAgICAgICB9IHNlZGY7Ci0gICAgICAgIHN0 cnVjdCB4ZW5fZG9tY3RsX3NjaGVkX2NyZWRpdCB7Ci0gICAgICAgICAgICB1aW50MTZfdCB3ZWln aHQ7Ci0gICAgICAgICAgICB1aW50MTZfdCBjYXA7Ci0gICAgICAgIH0gY3JlZGl0OwotICAgICAg ICBzdHJ1Y3QgeGVuX2RvbWN0bF9zY2hlZF9jcmVkaXQyIHsKLSAgICAgICAgICAgIHVpbnQxNl90 IHdlaWdodDsKLSAgICAgICAgfSBjcmVkaXQyOwotICAgICAgICBzdHJ1Y3QgeGVuX2RvbWN0bF9z Y2hlZF9ydGRzIHsKLSAgICAgICAgICAgIHVpbnQzMl90IHBlcmlvZDsKLSAgICAgICAgICAgIHVp bnQzMl90IGJ1ZGdldDsKLSAgICAgICAgfSBydGRzOworICAgICAgICBzdHJ1Y3QgeGVuX2RvbWN0 bF9zY2hlZF9zZWRmIHNlZGY7CisgICAgICAgIHN0cnVjdCB4ZW5fZG9tY3RsX3NjaGVkX2NyZWRp dCBjcmVkaXQ7CisgICAgICAgIHN0cnVjdCB4ZW5fZG9tY3RsX3NjaGVkX2NyZWRpdDIgY3JlZGl0 MjsKKyAgICAgICAgc3RydWN0IHhlbl9kb21jdGxfc2NoZWRfcnRkcyBydGRzOworICAgICAgICBz dHJ1Y3QgeworICAgICAgICAgICAgWEVOX0dVRVNUX0hBTkRMRV82NCh4ZW5fZG9tY3RsX3NjaGVk cGFyYW1fdmNwdV90KSB2Y3B1czsKKyAgICAgICAgICAgIHVpbnQxNl90IG5yX3ZjcHVzOworICAg ICAgICB9IHY7CiAgICAgfSB1OwogfTsKIHR5cGVkZWYgc3RydWN0IHhlbl9kb21jdGxfc2NoZWR1 bGVyX29wIHhlbl9kb21jdGxfc2NoZWR1bGVyX29wX3Q7Cg== --=-/9Ra74dH/amd+IvcNQ9S-- --=-XzJwqa1OdKYnPRcfwFCB Content-Type: application/pgp-signature; name="signature.asc" Content-Description: This is a digitally signed message part -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iEYEABECAAYFAlWb6HkACgkQk4XaBE3IOsQ0gQCePv9DtVNxBDwJThJ0ahN4YW04 6lsAn2YbghWnaZYr0STlbTtMbXJWKmqD =/2Xq -----END PGP SIGNATURE----- --=-XzJwqa1OdKYnPRcfwFCB-- --===============6449790542632522244== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel --===============6449790542632522244==--