From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dario Faggioli Subject: Re: [Help] Trigger Watchdog when adding an IPI in vcpu_wake Date: Thu, 22 Sep 2016 11:12:15 +0200 Message-ID: <1474535535.4393.339.camel@citrix.com> References: <20160913085437.GA14514@linux-gk3p> <1473766217.6339.103.camel@citrix.com> <20160914104417.GA25572@linux-gk3p> <20160917033051.GA16347@vultr.guest> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============7761161628743990666==" Return-path: In-Reply-To: <20160917033051.GA16347@vultr.guest> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xen.org Sender: "Xen-devel" To: Wei Yang , Wei Yang Cc: george.dunlap@eu.citrix.com, xuquan8@huawei.com, mengxu@cis.upenn.edu, liuxiaojian6@huawei.com, xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org --===============7761161628743990666== Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="=-DccgZlCtfaj143Xzp2tQ" --=-DccgZlCtfaj143Xzp2tQ Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Sat, 2016-09-17 at 03:30 +0000, Wei Yang wrote: > Dario, >=20 Hey, hi again, and sorry for the in getting back at this particular part of the conversation. > Just get chance to look into this. This is interesting and I am > trying to > understand the problem you want to solve first. >=20 :-) > vcpu_wake() is a _special_ case who has to turn off the irq, because > SCHED_OP(wake, v) would not only enqueue the vcpu but also tickle > pcpu to pick > up the queued vcpu. And the tickling part needd the irq to be turned > off. >=20 _Almost_ correct. However, the problem is more that vcpu_wake() can happen in response to an IRQ, and when you grab a spinlock in IRQ context, you need to disable IRQs. There is a good explanation of why, here: > I don't get the this part. Why we have to turn off the irq for > tickling pcpu? >=20 We don't. The point here is that, in Xen, we wants spinlocks to either _always_ be taken with IRQs disabled or _always_ be taken with IRQs enabled. Well, since we now take the scheduler's runqueue lock in vcpu_wake() (and as said that must be done with IRQs disabled), this implies that the scheduler's runqueue lock needs to always be taken with IRQs disabled, even when leaving them enabled would not actually be an issue, for being consistent with the locking rule. So, if we manage to avoid taking the scheduler's runqueue lock in vcpu_wake= (), we will manage to put ourself in the opposite situation, wrt the lockin= g consistency rule, i.e., we can _always_ take the scheduler's runqueue loc= k with IRQs enabled. This is, broadly speaking, the problem we wanted to solve, and how we thoug= ht about solving it. > And by enabling irq in schedule handlers benefits the performance? or > ? The > motivation behind this is? >=20 As I said to you about your modification, here too it is not super-easy=20 to tell in advance whether, and if yes, by how much, we'll see a boost in performance. In this case, the idea is that, in general, keeping IRQs disabled is bad. It is bad for concurrency, it is bad for latency in both scheduling and when it comes to reacting to external event. And it has been profiled that the scheduler is one of the component that keeps the IRQs disabled for long chunks of time. I honestly did expect things to improve a bit, especially on certain workloads, but of course, as usual, benchmarks will tell. :-) > Didn't look into your code yet.=20 > Ok.=C2=A0 Even if/when you look at it, bear in mind it's still only a stub. > From the description from Andrew, I comes with > several questions: > 1. which per-cpu list you want to queue the vcpu? v->processor? > ISTR having thought, and even given a try, to both v->processor (where v is the waking vcpu) and the current cpu (where for 'current cpu' I meant the cpu where the wake-up is occurring). I think I decided to use the current cpu (mostly for the same reasons why I don't think there is much advantage in what you are trying to do :-P) > 2. SCHEDULE_WAKE_SOFTIRQ would do runqueue insert and tickle? > It would call what is now vcpu_wake(). So, yes, for most scheduler that means inserting and tickling, but it's really schedulere specific. > 3. purpose is so that schedule softirq could run with irq enabled > Purpose is that scheduling functions can be executed with IRQs enabled, yes. > 4. the schedule softirq would do the jobs currently in vcpu_wake()? >=20 Err... not sure what you mean (but maybe I've already answered at point 2.?). > Took the per-cpu branch as an example, I see several commits. The top > 6 are > related ones, right? >=20 The top 4 (I told you it's a stub :-P). Regards, Dario --=20 <> (Raistlin Majere) ----------------------------------------------------------------- Dario Faggioli, Ph.D, http://about.me/dario.faggioli Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK) --=-DccgZlCtfaj143Xzp2tQ 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 iQIcBAABCAAGBQJX46BwAAoJEBZCeImluHPuJ/4P/iAaFyoAIJbCfJV+4zcvzVTM ZKpHKkPhjXTzw427Kc8ZbUeIfOX28YLI/zGMDi/Wvi+wCSZYveNtAX8RKQofKK/h kNEOq/5A/t+WGqu/DeupjfLBUgEsScoMh1qNY4RqcrFpXzwcpmOAFJEyaUotdUpL jVwh5YOtYSCsed0sK7No/CVhwrAq9g+hOZNH1IlkKubWuhaVlyHOeGVDIV7Aqs3Y BYmBILPTAum7P/msPE120cnjaf8dQmm7Y4xayDmEAyO0kPpyNrYTP9xzND8jaegA /vQ1oBPpM3nhKdWDYZ0mVt1KJvkxhLs8AlMk1Y2Om0EoZ1/Q0KJOw83AsSyy9VN9 Z3BeOK6kGGO945C9WG7Nc45vCMerCOekyLNG6erV2xkIZAIm2MpiaH65osVvEh90 6WGV9sbWKlqZrtdvO1RJ/MkyDs62ZbYfoIbRAHqdbj3mdvVnlhvTEAeFhenta7Im CjX2medgJYi8hjB+KAcLXCDmfMmDuHd/OCw61pPhlm0rp/Oixa0q/oKqts05XOEL 6da8ux4aGa/DfAYEKmubaz4qd5lxUqQHS5LhCgdtvagBK0jYEnXO4sPekFBrvwyu PSoF7rAjPyRTSnog0OpzsGnMBqXenZYLhkcZI8V6mFdhHdy6F4uQiaGSA5yiGSag +PUHzLNphwBiQCDLyQ5d =/IyV -----END PGP SIGNATURE----- --=-DccgZlCtfaj143Xzp2tQ-- --===============7761161628743990666== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KWGVuLWRldmVs IG1haWxpbmcgbGlzdApYZW4tZGV2ZWxAbGlzdHMueGVuLm9yZwpodHRwczovL2xpc3RzLnhlbi5v cmcveGVuLWRldmVsCg== --===============7761161628743990666==--