From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dario Faggioli Subject: Re: [PATCH 3/9] xen: sched: make locking for {insert, remove}_vcpu consistent Date: Tue, 29 Sep 2015 23:40:45 +0200 Message-ID: <1443562845.3276.134.camel@citrix.com> References: <20150929164726.17589.96920.stgit@Solace.station> <20150929165549.17589.76223.stgit@Solace.station> <560ACAD5.8040405@citrix.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0260846943383304099==" Return-path: Received: from mail6.bemta14.messagelabs.com ([193.109.254.103]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1Zh2dp-0004Gw-1M for xen-devel@lists.xenproject.org; Tue, 29 Sep 2015 21:40:53 +0000 In-Reply-To: <560ACAD5.8040405@citrix.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: Andrew Cooper , xen-devel@lists.xenproject.org Cc: George Dunlap , Meng Xu List-Id: xen-devel@lists.xenproject.org --===============0260846943383304099== Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="=-C/fkbCqzpcfb60un9IEi" --=-C/fkbCqzpcfb60un9IEi Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Tue, 2015-09-29 at 18:31 +0100, Andrew Cooper wrote: > On 29/09/15 17:55, Dario Faggioli wrote: > > The insert_vcpu() scheduler hook is called with an > > inconsistent locking strategy. In fact, it is sometimes > > invoked while holding the runqueue lock and sometimes > > when that is not the case. > >=20 > > In other words, some call sites seems to imply that > > locking should be handled in the callers, in schedule.c > > --e.g., in schedule_cpu_switch(), which acquires the > > runqueue lock before calling the hook; others that > > specific schedulers should be responsible for locking > > themselves --e.g., in sched_move_domain(), which does > > not acquire any lock for calling the hook. > >=20 > > The right thing to do seems to always defer locking to > > the specific schedulers, as it's them that know what, how > > and when it is best to lock (as in: runqueue locks, vs. > > private scheduler locks, vs. both, etc.) > >=20 > > This patch, therefore: > > - removes any locking around insert_vcpu() from > > generic code (schedule.c); > > - add the _proper_ locking in the hook implementations, > > depending on the scheduler (for instance, credit2 > > does that already, credit1 and RTDS need to grab > > the runqueue lock while manipulating runqueues). > >=20 > > In case of credit1, remove_vcpu() handling needs some > > fixing remove_vcpu() too, i.e.: > > - it manipulates runqueues, so the runqueue lock must > > be acquired; > > - *_lock_irq() is enough, there is no need to do > > _irqsave() >=20 > Nothing in any of generic scheduling code should need interrupts > disabled at all. >=20 That's a really, really, really interesting point. I think I see what you mean. However, currently, pretty much **all** scheduling related locks are acquired via _irq or _irqsave primitives, and that is true for schedule.c and for all the sched_*.c files. > One of the problem-areas identified by Jenny during the ticketlock > performance work was that the SCHEDULE_SOFTIRQ was a large consumer > of > time with interrupts disabled. > Right, and I am very much up for investigating whether this can improve. However, this seems to me the topic for a different series. > Is the use of _lock_irq() here to cover another issue expecting > interrupts to be disabled, or could it be replaced with a plain > spin_lock()? >=20 As said, it is probably the case that spin_lock() would be ok, here as well as elsewhere. This is being done like this in this patch for consistency, as that is what happens **everywhere** else in scheduling code. In fact, I haven't tried, but it may well be the case that, converting only one (or a subset) of locks to non _irq* variants, we'd make check_lock() complain. So, can we just allow this patch to follow suit, and then overhaul and change/fix (if it reveals feasible) all locking at once, in a dedicated series? This seems the best approach to me... > Also, a style nit... > > > diff --git a/xen/common/sched_credit.c b/xen/common/sched_credit.c > > index a1945ac..557efaa 100644 > > @@ -935,15 +946,18 @@ csched_vcpu_remove(const struct scheduler > > *ops, struct vcpu *vc) > > vcpu_unpause(svc->vcpu); > > } > > =20 > > + lock =3D vcpu_schedule_lock_irq(vc); > > + > > if ( __vcpu_on_runq(svc) ) > > __runq_remove(svc); > > =20 > > - spin_lock_irqsave(&(prv->lock), flags); > > + spin_lock(&(prv->lock)); >=20 > Please drop the superfluous brackets as you are already changing the > line. >=20 This, I can do for sure. :-) 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) --=-C/fkbCqzpcfb60un9IEi 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 iEYEABECAAYFAlYLBV0ACgkQk4XaBE3IOsT7pwCfSlXWItx8UOt9QpJYZyXgSYxj zZ8An04MYQ1GJ3Mp6mIYVTTvtLxXc0MY =uqOy -----END PGP SIGNATURE----- --=-C/fkbCqzpcfb60un9IEi-- --===============0260846943383304099== 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 --===============0260846943383304099==--