From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dario Faggioli Subject: Re: [PATCH v2 for Xen 4.6 1/4] xen: enabling XL to set per-VCPU parameters of a domain for RTDS scheduler Date: Thu, 4 Jun 2015 17:27:53 +0200 Message-ID: <1433431673.6291.27.camel@citrix.com> References: <1432598752-20826-1-git-send-email-chong.li@wustl.edu> <55645421020000780007DCB8@mail.emea.novell.com> <5565C96F020000780007E27D@mail.emea.novell.com> <1432904488.5077.30.camel@citrix.com> <55688260020000780007F088@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============5363781470244007925==" Return-path: In-Reply-To: <55688260020000780007F088@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 , xen-devel , Meng Xu , Chong Li , Dagaen Golomb List-Id: xen-devel@lists.xenproject.org --===============5363781470244007925== Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="=-omidLnSgozjFfGi/toYX" --=-omidLnSgozjFfGi/toYX Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Fri, 2015-05-29 at 14:14 +0100, Jan Beulich wrote: > >>> On 29.05.15 at 15:01, wrote: > > On Wed, 2015-05-27 at 12:41 +0100, Jan Beulich wrote: > >> >>> On 26.05.15 at 19:18, wrote: > >> > On Tue, May 26, 2015 at 4:08 AM, Jan Beulich wro= te: > >> >>>>> On 26.05.15 at 02:05, wrote: > >> >>> + > >> >>> + switch ( op->cmd ) > >> >>> + { > >> >>> + case XEN_DOMCTL_SCHEDOP_getvcpuinfo: > >> >>> + spin_lock_irqsave(&prv->lock, flags); > >> >>> + list_for_each( iter, &sdom->vcpu ) > >> >>> + { > >> >>> + svc =3D list_entry(iter, struct rt_vcpu, sdom_elem); > >> >>> + vcpuid =3D svc->vcpu->vcpu_id; > >> >>> + > >> >>> + local_sched.budget =3D svc->budget / MICROSECS(1); > >> >>> + local_sched.period =3D svc->period / MICROSECS(1); > >> >>> + if ( copy_to_guest_offset(op->u.rtds.vcpus, vcpuid, > >> >> > >> >> What makes you think that the caller has provided enough slots? > >> >=20 > >> > This code works in a similar way as the one for XEN_SYSCTL_numainfo. > >> > So if there is no enough slot in guest space, en error code is > >> > returned. > >>=20 > >> I don't think I saw any place you returned an error here. The > >> only error being returned is -EFAULT when the copy-out fails. > >>=20 > > Look again at XEN_SYSCTL_numainfo, in particular, at this part: > >=20 > > if ( ni->num_nodes < num_nodes ) > > { > > ret =3D -ENOBUFS; > > i =3D num_nodes; > > } > > else > > i =3D 0 > >=20 > > Which, as you'll appreciate if you check from where ni->num_nodes and > > num_nodes come from, if where the boundary checking we're asking for > > happens. > >=20 > > Note how the 'i' variable is used in the rest of the block, and you'll > > see what we mean, and can do something similar. >=20 > I think this was targeted at Chong rather than me (while I was > listed as To, and Chong only on Cc)? >=20 It was indeed targeted at him. :-) I replied to your email to re-use and quote the points you made, but did not think about changing what my MUA does by default when hitting 'Reply'... I never do that, actually, but I now see how it could be a good idea to do so... I'll try to remember and fit that in my workflow. > >> >>> +/* Adjust scheduling parameter for the vcpus of a given domain. *= / > >> >>> +long sched_adjust_vcpu( > >> >>> + struct domain *d, > >> >>> + struct xen_domctl_scheduler_vcpu_op *op) > >> >>> +{ > >> >>> + long ret; > >> >> > >> >> I see no reason for this and the function return type to be "long". > >> >=20 > >> > The reason is stated in the begining. > >>=20 > >> In the beginning of what? I don't see any such, and it would also be > >> odd for there to be an explanation of why a particular type was chosen= . > >> > > As Chong he's saying elsewhere, he's moslty following suit. Should we > > consider a pre-patch making both sched_adjust() and > > sched_adjust_global() retunr int? >=20 > Perhaps. But the main point here is that people copying existing code > should sanity check what they copy (so to not spread oddities or even > bugs). >=20 Agreed, but in this case, he'd have ended up with: long sched_adjust(...) int sched_adjust_vcpu(...) long sched_adjust_global(...) So I think I see why Chong just went with 'long', that was my point. Then of course, one could have spotted that it's the two existing ones that are wrong/suboptimal, but that's more our responsibility than Chong's fault, IMO (which does not mean that we should not point the thing out and fix/ask to fix). In any case, as far as this series is concerned, there should be no need for a new function like this any longer. For "fixing" _adjust and _adjust_global, I'll take care of it. Regards, Dario --=20 <> (Raistlin Majere) ----------------------------------------------------------------- Dario Faggioli, Ph.D, http://about.me/dario.faggioli Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK) --=-omidLnSgozjFfGi/toYX 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 iEYEABECAAYFAlVwbnkACgkQk4XaBE3IOsRQgQCeMO/6LFLj7SoKf5sZXaOTN3MP 6UgAnRMv2Kcr2e72lLyhf2LWnD4kvvmC =GuW7 -----END PGP SIGNATURE----- --=-omidLnSgozjFfGi/toYX-- --===============5363781470244007925== 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 --===============5363781470244007925==--