From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dario Faggioli Subject: Re: [PATCH 09/16] xen: sched: close potential races when switching scheduler to CPUs Date: Wed, 6 Apr 2016 18:21:10 +0200 Message-ID: <1459959670.3166.100.camel@citrix.com> References: <20160318185524.8117.74837.stgit@Solace.station> <20160318190505.8117.89778.stgit@Solace.station> <56F3DA34.4040007@citrix.com> <1459877839.3166.84.camel@citrix.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============6856892461424287677==" Return-path: Received: from mail6.bemta6.messagelabs.com ([85.158.143.247]) by lists.xenproject.org with esmtp (Exim 4.84_2) (envelope-from ) id 1anqCm-0004lm-T4 for xen-devel@lists.xenproject.org; Wed, 06 Apr 2016 16:21:20 +0000 In-Reply-To: <1459877839.3166.84.camel@citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xen.org Sender: "Xen-devel" To: George Dunlap , xen-devel@lists.xenproject.org Cc: George Dunlap , Tianyang Chen , Josh Whitehead , Meng Xu , Robert VanVossen List-Id: xen-devel@lists.xenproject.org --===============6856892461424287677== Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="=-ULXVi9jkah38xvT+dF4a" --=-ULXVi9jkah38xvT+dF4a Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Tue, 2016-04-05 at 19:37 +0200, Dario Faggioli wrote: > On Thu, 2016-03-24 at 12:14 +0000, George Dunlap wrote: > > On 18/03/16 19:05, Dario Faggioli wrote: > > So I think there should be no problem with: > > 1. Grabbing the pcpu schedule lock in schedule_cpu_switch() > > 2. Grabbing prv->lock in csched2_switch_sched() > > 3. Setting the per_cpu schedule lock as the very last thing in > > csched2_switch_sched() > > 4. Releasing the (old) pcpu schedule lock in schedule_cpu_switch(). > >=20 > > What do you think? > >=20 > I think it should work. We'll be doing the scheduler lock > manipulation > protected by the old and wrong (as it's the one of another scheduler) > per-cpu/runq lock, and the correct global private lock. It would look > like the ordering between the two locks is the wrong one, in Credit2, > but it's not because of the fact that the per-runq lock is the other > scheduler's one. >=20 > Tricky, but everything is in here! :-/ >=20 I've done it as you suggest above. The new .switch_sched hook is still there, and still does look the same. But I do indeed like the final look of the code better, and it appears to be working ok. Have a look. ;-) > > As an aside -- it seems to me that as soon as we change the > > scheduler > > lock, there's a risk that something else may come along and try to > > grab > > it / access the data.=C2=A0=C2=A0Does that mean we really ought to use = memory > > barriers to make sure that the lock is written only after all > > changes > > to > > the scheduler data have been appropriately made? > >=20 > Yes, if it were only for this code, I think you're right, barriers > would be necessary. I once again think this is actually safe, because > it's serialized elsewhere, but thinking more about it, I can well add > both barriers (and a comment). >=20 And I've added smp_mb()-s too. > > > This also means that, in Credit2 and RTDS, we can get rid > > > of the code that was doing the scheduler lock remapping > > > in csched2_free_pdata() and rt_free_pdata(), and of their > > > triggering ASSERT-s. > > Right -- so to put it a different way, *all* schedulers must now > > set > > the > > locking scheme they wish to use, even if they want to use the > > default > > per-cpu locks.=C2=A0=C2=A0 > >=20 > Exactly. >=20 > >=20 > > I think that means we have to do that for arinc653 too, > > right? > >=20 > Mmm... right, I'll have a look at that. >=20 And, finally, I did have a look at this too, and I actually don't think ARINC needs any of this. In fact, ARINC brings the idea of "doing its own locking" much further than the other schedulers we have. They have their lock and they use it in such a way that they don't even care to what {v,p}cpu_schedule_lock() and friends points to. As an example, check=C2=A0a653sched_do_schedule(). It's called from schedule(), right after taking the runqueue lock, with=C2=A0pcpu_schedule_lock_irq(), and yet it does this: =C2=A0spin_lock_irqsave(&sched_priv->lock, flags); So, I actually better _not_ add anything to this series that re-maps sd->schedule_lock to point to their sched_priv->lock, or we'd deadlock! I'm not sure the design behind all this is the best possible one, but that's a different issue, to be dealt with with another series in another moment. :-) In any case, I've added Robert and Josh. Regards, Dario --=20 <> (Raistlin Majere) ----------------------------------------------------------------- Dario Faggioli, Ph.D, http://about.me/dario.faggioli Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK) --=-ULXVi9jkah38xvT+dF4a 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 iEYEABECAAYFAlcFN3YACgkQk4XaBE3IOsTzywCghIroUTEx/4cjRe35u+el+dn3 vIIAn0UqiVakbULigGg1mHWlS5m2a26c =eq3x -----END PGP SIGNATURE----- --=-ULXVi9jkah38xvT+dF4a-- --===============6856892461424287677== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KWGVuLWRldmVs IG1haWxpbmcgbGlzdApYZW4tZGV2ZWxAbGlzdHMueGVuLm9yZwpodHRwOi8vbGlzdHMueGVuLm9y Zy94ZW4tZGV2ZWwK --===============6856892461424287677==--