From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dario Faggioli Subject: Re: [PATCH] xen: recalculate per-cpupool credits when updating timeslice Date: Tue, 2 Feb 2016 10:53:02 +0100 Message-ID: <1454406782.9227.27.camel@citrix.com> References: <1454062908-32013-1-git-send-email-jgross@suse.com> <56AB511402000078000CC59C@suse.com> <56AB45FE.5010500@suse.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============6914681613023875494==" Return-path: In-Reply-To: <56AB45FE.5010500@suse.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: Juergen Gross , Jan Beulich Cc: george.dunlap@eu.citrix.com, xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org --===============6914681613023875494== Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="=-sCpnTIRYvpVf4Tc7b+NY" --=-sCpnTIRYvpVf4Tc7b+NY Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Fri, 2016-01-29 at 11:59 +0100, Juergen Gross wrote: > On 29/01/16 11:46, Jan Beulich wrote: > > > > > On 29.01.16 at 11:21, wrote: > > > --- a/xen/common/sched_credit.c > > > +++ b/xen/common/sched_credit.c > > > @@ -1086,12 +1086,19 @@ csched_dom_cntl( > > > =C2=A0static inline void > > > =C2=A0__csched_set_tslice(struct csched_private *prv, unsigned > > > timeslice) > > > =C2=A0{ > > > +=C2=A0=C2=A0=C2=A0=C2=A0unsigned long flags; > > > + > > > +=C2=A0=C2=A0=C2=A0=C2=A0spin_lock_irqsave(&prv->lock, flags); > > > + > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0prv->tslice_ms =3D timeslice; > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0prv->ticks_per_tslice =3D CSCHED_TICKS_= PER_TSLICE; > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if ( prv->tslice_ms < prv->ticks_per_ts= lice ) > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0prv->ticks_per_= tslice =3D 1; > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0prv->tick_period_us =3D prv->tslice_ms = * 1000 / prv- > > > >ticks_per_tslice; > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0prv->credits_per_tslice =3D CSCHED_CRED= ITS_PER_MSEC * prv- > > > >tslice_ms; > > > +=C2=A0=C2=A0=C2=A0=C2=A0prv->credit =3D prv->credits_per_tslice * pr= v->ncpus; > > > + > > > +=C2=A0=C2=A0=C2=A0=C2=A0spin_unlock_irqrestore(&prv->lock, flags); > > > =C2=A0} > >=20 > > The added locking, which has no reason give for in the description > > at all, puzzles me: I can see it being needed (and having been > > missing) when called from csched_sys_cntl(), but it's not clear to > > me why it would be needed when called from csched_init(). Yet > > csched_sys_cntl() subsequently als updates prv->ratelimit_us, > > and hence the lock would perhaps better be taken there? >=20 > The locking is needed to protect against csched_alloc_pdata() and > csched_free_pdata(). prv->credit could be permananently wrong > without the lock, while prv->ratelimit_us can't be modified > concurrently in a wrong way (it could be modified by two concurrent > calls of csched_sys_cntl(), but even with locking one of both > calls would be the winner, same applies to the case with no lock). >=20 > OTOH I don't mind moving the lock to csched_sys_cntl(). Dario, > George, any preferences? >=20 Yes, I think having the lock in csched_sys_cntl() would be preferable. In any case, since the lack of locking and lack of recalculation look like two pretty independent existing bugs to me, can we have either: =C2=A0a. two patches; =C2=A0b. one patch but with both the issues described in the changelog. My preference going to a. 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) --=-sCpnTIRYvpVf4Tc7b+NY 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 v1 iEYEABECAAYFAlawfH4ACgkQk4XaBE3IOsST6wCfV3noPrvCKRHM3RBXivrYOMu1 dWUAniUNS61EyJ6UAE5VwYzbGLNYD+d1 =Bwf8 -----END PGP SIGNATURE----- --=-sCpnTIRYvpVf4Tc7b+NY-- --===============6914681613023875494== 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 --===============6914681613023875494==--