From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dario Faggioli Subject: Re: [PATCH] xen:rtds:Clear __RTDS_depleted bit once replenish vcpu Date: Tue, 25 Oct 2016 11:20:51 +0200 Message-ID: <1477387251.27407.53.camel@citrix.com> References: <1477102322-4162-1-git-send-email-mengxu@cis.upenn.edu> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0440367271570679008==" Return-path: Received: from mail6.bemta5.messagelabs.com ([195.245.231.135]) by lists.xenproject.org with esmtp (Exim 4.84_2) (envelope-from ) id 1byxv0-0000Ts-7B for xen-devel@lists.xenproject.org; Tue, 25 Oct 2016 09:21:14 +0000 In-Reply-To: <1477102322-4162-1-git-send-email-mengxu@cis.upenn.edu> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xen.org Sender: "Xen-devel" To: Meng Xu , xen-devel@lists.xenproject.org Cc: Wei Liu , Dagaen Golomb , George Dunlap , Haoran Li , Linh Thi Xuan Phan , Meng Xu , Tianyang Chen List-Id: xen-devel@lists.xenproject.org --===============0440367271570679008== Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="=-krVKDWUI7lml+4kJqlJ4" --=-krVKDWUI7lml+4kJqlJ4 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Fri, 2016-10-21 at 22:12 -0400, Meng Xu wrote: > We should clear the __RTDS_depleted bit once a VCPU budget is > replenished. > Because repl_timer_handler may be called after rt_schedule > but before rt_context_saved, the VCPU may be not on CPU or on queue > when the VCPU is the middle of context switch > Yes, this makes sense, and is a good fix. I actually would prefer the subject to become (something like): "xen: rtds: always clear the flag when replenishing a depleted vcpu" whith that changed: > Signed-off-by: Meng Xu > Acked-by: Dario Faggioli This being said, I hope you see the value of having split the patches --especially patches like this-- in such a way that each one deals with one specific issue. It doesn't matter if they end up being very small, in terms of changed lines. In fact, the behavioral issue being dealt with in here is rather subtle, and most important "self-contained", i.e., independent from any other issue that we may or may not have already found. Also, consider that, if it turns out that this patch is wrong, i.e., it does not really fix the problem and/or introduce other ones, we will be able to revert it. If the hunk below was part of a more complex patch, that would have been both more difficult and less neat. Another example: > --- a/xen/common/sched_rt.c > +++ b/xen/common/sched_rt.c > @@ -1488,8 +1488,8 @@ static void repl_timer_handler(void *data){ > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0if ( svc->cur_deadline > next_on_runq->cur_deadline ) > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0runq_tickle(ops, next_on_runq); > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0} > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0else if ( vcpu_on_q(svc= ) && > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0__test_and_clear_bit(__RTDS_depleted, = &svc->flags)=20 > ) > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0else if ( __test_and_cl= ear_bit(__RTDS_depleted, &svc->flags)=20 > && > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0vcpu_on_q(svc) ) > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0runq_tickle(ops, svc); > =C2=A0 The previous patch was dropping the 'else', the reason for which was not really easy to see to me. I went looking at the commit message, but it was not really clear itself, and actually contained the description of two problems and two fixes! :-O So, again, for things like these, 1 issue =3D=3D> 1 patch. About the other patch, I think I understand the situation, but I don't like the fix much. I'm looking into it and thinking what could be an alternative solution. I'll let you know. And finally, independently from these patches, I was thinking whether it would not be worth dealing with the case budget=3D=3Dperiod as a specia= l case. I mean, if that is the case, there's no need to program the timer, etc, we could just identify the situation and act accordingly (e.g., in burn_budget()). This is just an idea, though, based on the fact that people may want to set budget=3D=3Dperiod for some domain/vcpu (like the "special" ones, dom0= or driver domains), and if there are a few of them, we avoid the overhead of timers firing very very close to re-scheduling points, etc.=C2=A0 On the other hand, special cases make the code uglier, and often harder to follow. So I'm saying it may (in my opinion) be worth trying to write a patch, but it's not guaranteed that we actually want to take it... It's just hard to tell, before actually seeing the code. This is, of course, just my idea, with which you can well disagree. :-) 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) --=-krVKDWUI7lml+4kJqlJ4 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 iQIcBAABCAAGBQJYDyP4AAoJEBZCeImluHPueucP/RGZXNgCIh5tO0RbwL+5MDgH LWwNfTHPL538/Kb58ao0y6XwNLoKRy2ol39QUe0nv5a521J3GnDAlKn5F+40N8GI q4Vw5b8uKOlo71h1txfSVIAs9eVANnOtMfnu6fVb6QtI+fESLx2LJZc+OpezPsJ6 O/dLxrwMlFR6PKzlM5a/l+1MxYpELNs4LTV3GgxHnBxruxSqTxXs9VfxyyeR5W22 T3D4bnYsjsq2dbLjpb7WiWtbzbpXyPhvIdJ2xW/IbLKvkgE/Tbh0EyJ4p5uBROap e9RlqvKL5zw4CJiIRuCluOIs7hOWXyBI45bPIAgYo/z6imL1pX0EAl/ZUGVK354G wNU0dND4Qr5MbsuhHHoWkfE6iKtFg6rgTuITSphz4d5k+hct9VESEOleYsq13vNZ fkSr2nKoTEMGWyJnfmY5v/5cb+fG+whTg+k417Y1hZ2Bj7ltnvPLK45Naw5ozWBq muPGLrLg27qpSg2uaeWcAUGs/UndCPeDAO197xXKx9c0rYwXyjq4TfJlHunYeGJs ayyKBM/MMTbWEjEJZaehESnDDGDwAEwvS4+3SIr9gAzMn9xXCbETRjqo3Cd7u2fn 8r4C+x4DE0H5fTTRy82R236UmAb8fscYj1qXEF27P07Fl0lPUToANVSBJXPtDUQS E2H4DCXNmhUMbym3X/c3 =Xy8w -----END PGP SIGNATURE----- --=-krVKDWUI7lml+4kJqlJ4-- --===============0440367271570679008== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KWGVuLWRldmVs IG1haWxpbmcgbGlzdApYZW4tZGV2ZWxAbGlzdHMueGVuLm9yZwpodHRwczovL2xpc3RzLnhlbi5v cmcveGVuLWRldmVsCg== --===============0440367271570679008==--