From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dario Faggioli Subject: Re: [PATCH 3/7] xen: rework locking for dump of scheduler info (debug-key r) Date: Tue, 17 Mar 2015 11:14:10 +0000 Message-ID: <1426590848.32500.63.camel@citrix.com> References: <20150316165642.10279.86684.stgit@Solace.station> <20150316170509.10279.79362.stgit@Solace.station> <55081607020000780006AAAE@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============2136734145815311325==" Return-path: In-Reply-To: <55081607020000780006AAAE@mail.emea.novell.com> Content-Language: en-US List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: "JBeulich@suse.com" Cc: "Keir (Xen.org)" , "xumengpanda@gmail.com" , George Dunlap , "xen-devel@lists.xen.org" List-Id: xen-devel@lists.xenproject.org --===============2136734145815311325== Content-Language: en-US Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="=-3iapH2auzhP7WFETEa4y" --=-3iapH2auzhP7WFETEa4y Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Tue, 2015-03-17 at 10:54 +0000, Jan Beulich wrote: > >>> On 16.03.15 at 18:05, wrote: > > such as it is taken care of by the various schedulers, rather > > than happening in schedule.c. In fact, it is the schedulers > > that know better which locks are necessary for the specific > > dumping operations. > >=20 > > While there, fix a few style issues (indentation, trailing > > whitespace, parentheses and blank line after var declarations) > >=20 > > Signed-off-by: Dario Faggioli > > Cc: George Dunlap > > Cc: Meng Xu > > Cc: Jan Beulich > > Cc: Keir Fraser > > --- > > xen/common/sched_credit.c | 42 ++++++++++++++++++++++++++++++++++++= ++++-- > > xen/common/sched_credit2.c | 40 ++++++++++++++++++++++++++++++++----= ---- > > xen/common/sched_rt.c | 7 +++++-- > > xen/common/schedule.c | 5 ++--- > > 4 files changed, 79 insertions(+), 15 deletions(-) >=20 > Is it really correct that sched_sedf.c doesn't need any change here? >=20 Good point. I feel like mentioning though that SEDF is broken in many ways, and I (and I suspect George is in a similar situation) really don't have the bandwidth to fix it. Therefore, I really think we should start a discussion on how to deprecate/get rid of it. That being said, this is simple enough a case that I certainly can have a quick look. > > --- a/xen/common/sched_credit.c > > +++ b/xen/common/sched_credit.c > > @@ -26,6 +26,23 @@ > > =20 > > =20 > > /* > > + * Locking: > > + * - Scheduler-lock (a.k.a. runqueue lock): > > + * + is per-runqueue, and there is one runqueue per-cpu; > > + * + serializes all runqueue manipulation operations; > > + * - Private data lock (a.k.a. private scheduler lock): > > + * + serializes accesses to the scheduler global state (weight, > > + * credit, balance_credit, etc); > > + * + serializes updates to the domains' scheduling parameters. > > + * > > + * Ordering is "private lock always comes first": > > + * + if we need both locks, we must acquire the private > > + * scheduler lock for first; > > + * + if we already own a runqueue lock, we must never acquire > > + * the private scheduler lock. > > + */ >=20 > And this is Credit1 specific? >=20 It is similar (although with some differences) in Credit2, but there is already a comment there, similar to this, stating it. RTDS, as of now, only use _one_ lock, so not much to say about ordering. :-D > Regardless of that, even if that's just reflecting current state, isn't > acquiring a private lock around a generic lock backwards? >=20 It sounds like so, I agree, when describing it in English. But then, looking at the code, things make sense, IMO. > Finally, as said in different contexts earlier, I think unconditionally > acquiring locks in dumping routines isn't the best practice. At least > in non-debug builds I think these should be try-locks only, skipping > the dumping when a lock is busy. >=20 That makes sense. It also looks, if not completely orthogonal, something that can be done in a follow-up patch wrt this series. I.e., we first move the locking logic where it belongs, and then we rework it toward using _trylock(), does this make sense? Regards, Dario --=-3iapH2auzhP7WFETEa4y 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 iEYEABECAAYFAlUIDIAACgkQk4XaBE3IOsSSCgCfYiWOlbmBpqNWtmnh/mQeDu7l xFMAn11qzXXAZ/6+Y6SdHXMWi5m/DbzS =uJAo -----END PGP SIGNATURE----- --=-3iapH2auzhP7WFETEa4y-- --===============2136734145815311325== 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 --===============2136734145815311325==--