From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dario Faggioli Subject: Re: [PATCH] xen: idle_loop: either deal with tasklets or go idle Date: Fri, 16 Jun 2017 15:34:07 +0200 Message-ID: <1497620047.30417.7.camel@citrix.com> References: <149745892779.20244.4770433880444010417.stgit@Solace.fritz.box> <149745919711.20244.17843343131079129783.stgit@Solace.fritz.box> <5943B8F202000078001635CC@prv-mh.provo.novell.com> <1497609841.30417.5.camel@citrix.com> <5943E16D02000078001636DC@prv-mh.provo.novell.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============6844149660999598030==" Return-path: Received: from mail6.bemta6.messagelabs.com ([193.109.254.103]) by lists.xenproject.org with esmtp (Exim 4.84_2) (envelope-from ) id 1dLrOf-0006wA-Me for xen-devel@lists.xenproject.org; Fri, 16 Jun 2017 13:34:45 +0000 In-Reply-To: <5943E16D02000078001636DC@prv-mh.provo.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xen.org Sender: "Xen-devel" To: Jan Beulich Cc: Andrew Cooper , Julien Grall , Stefano Stabellini , BorisOstrovsky , xen-devel@lists.xenproject.org List-Id: xen-devel@lists.xenproject.org --===============6844149660999598030== Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="=-j66XQ7w6rHPMqiiAQZ93" --=-j66XQ7w6rHPMqiiAQZ93 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Fri, 2017-06-16 at 05:47 -0600, Jan Beulich wrote: > > On Fri, 2017-06-16 at 02:54 -0600, Jan Beulich wrote: > > > > > > On 14.06.17 at 18:53, wrote: > > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0unsigned long *work_to_do =3D &per_cp= u(tasklet_work_to_do, > > > > cpu); > > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0struct list_head *list =3D &per_cpu(t= asklet_list, cpu); > > > > =C2=A0 > > > > -=C2=A0=C2=A0=C2=A0=C2=A0/* > > > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0* Work must be enqueued *and* schedu= led. Otherwise there > > > > is > > > > no work to > > > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0* do, and/or scheduler needs to run = to update idle vcpu > > > > priority. > > > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0*/ > > > > -=C2=A0=C2=A0=C2=A0=C2=A0if ( likely(*work_to_do !=3D > > > > (TASKLET_enqueued|TASKLET_scheduled)) ) > > > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0return; > > >=20 > > > Perhaps it also wouldn't hurt to convert this to an ASSERT() too. > > >=20 > >=20 > > Nope, I can't. It's a best effort check, and *work_to_do (which is > > per_cpu(tasklet_work_to_do,cpu)) can change, and the assert may > > fail. >=20 > How that? TASKLET_enqueued can only be cleared by do_tasklet() > itself, and I don't think nesting invocations of the function can or > should occur. TASKLET_scheduled is only being cleared when > schedule() observes that bit set without TASKLET_enqueued also > set. IOW there may be races in setting of the bits, but since we > expect the caller to have done this check already, I think an > ASSERT() would be quite fine here. >=20 Ok, makes sense. I will add the ASSERT() (with something like what you wrote here as a comment). > > > Wouldn't cpu_is_haltable() then also better use this new > > > function? > > >=20 > >=20 > > Mmm... Perhaps. It's certainly less code chrun. > >=20 > > ARM code would then contain two invocations of cpu_is_haltable() > > (the > > first happens with IRQ enabled, so a second one with IRQ disabled > > is > > necessary). But that is *exactly* the same thing we do on x86 > > (they're > > just in different functions in that case). > >=20 > > So, I reworked the patch according to these suggestions, and you > > can > > look at it below. >=20 > I'm confused: You've added further uses of cpu_is_haltable() > where the cheaper tasklet_work_to_do() would do. > Indeed. Sorry! Fact is, I've read your comment backwards, i.e., as you were saying something like "wouldn't cpu_is_haltable() better be used here, instead of this new function?" And it's not that your wording is ambiguous, it's me that, apparently, can't read English! :-/ I'll rework the patch again... > Of course that suggestion > of mine was more than 1:1 replacement - the implied question was > whether cpu_is_haltable() simply checking tasklet_work_to_do to > be non-zero isn't too lax (and wouldn't better be > !tasklet_work_to_do()). >=20 Now, to try to answer the real question... Let's assume that, on cpu x, we are about to check cpu_is_haltable(), while, on cpu y, tasklet_schedule_on_cpu(x) is being called, and manages to set _TASKLET_enqueued in *work_to_do. I.e., in current code: CPU x CPU y | | cpu_is_haltable(x) tasklet_schedule_on_cpu(x) |!softirq_pending(x) =3D=3D true tasklet_enqueue() |cpu_online(x) =3D=3D true test_and_set(TASKLET_enqueued, | work_to_do) |!work_to_do =3D=3D FALSE So we don't go to sleep, and we stay in the idle loop for the next iteration. At which point, CPU y will have raised SCHEDULE_SOFTIRQ on x, schedule (still on x) will set TASKLET_scheduled, and we'll call do_tasklet(). Basically, right now, we risk spinning for the time that passes between TASKLET_enqueued being set and SCHEDULE_SOFTIRQ being raised and reaching cpu x. This should be a very short window, and, considering how the TASKLET_* flags are handled, this looks the correct behavior to me. If we use !tasklet_work_to_do() in cpu_is_haltable(): =C2=A0CPU x=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=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 CPU y =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=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=A0cpu_is_haltable(x)=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 tasklet_schedule_on_cpu(x) =C2=A0|!softirq_pending(x) =3D=3D true=C2=A0=C2=A0=C2=A0=C2=A0 tasklet_e= nqueue() =C2=A0|cpu_online(x) =3D=3D true=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0 test_and_set(TASKLET_enqueued, =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=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=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0work_to_do) =C2=A0|!(work_to_do =3D=3D TASKLET_enqueued+ TASKLET_scheduled) =3D=3D TRUE Which means we'd go to sleep... just for (most likely) be woken up very very soon by SCHEDULE_SOFTIRQ being thrown at us (cpu x) by cpu y. Am I overlooking anything? And is this (this time) what you were asking? Assuming answers are 'no' and 'yes', I think I'd leave cpu_is_haltable() as it is (perhaps adding a brief comment on why it's ok/better to check work_to_do directly, instead than calling tasklet_work_to_do()). Sorry again for the misunderstanding, Dario --=20 <> (Raistlin Majere) ----------------------------------------------------------------- Dario Faggioli, Ph.D, http://about.me/dario.faggioli Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK) --=-j66XQ7w6rHPMqiiAQZ93 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 iQIcBAABCAAGBQJZQ95QAAoJEBZCeImluHPu+NcP/jPLsb5EvhpY5PeXq+DGZXz6 WHEC32qOPLcCZrQXKiyKmnd6ynvzmMDtdlUdIiGgxRGdpoR5MaRcppJU0uDar+Vp 3UdYsSE/XmG6n22fjNUarkEMUzExZMUlZojnpRRq4Yol29P1AehrDFrqH5xISmYO PCO8+Mkvp/4QCGRB8inUx4cmO70E0ABvUg9UAA1L70G/jCYrqlcfqfuX+fZwYrzA mATGZ8OJwLLBLD3NGyykFrg/xFcEL6lPMcdnw214n+7WvsPTXIH4oDhlyuwpFKUC XoTE7/IzYzA98HyYSgic5wus/qAOEVMiwBD1cN5mvQdlAbrYXAIVWR5oO1hrFUyP qEiG6w2oc3YWpncBCUuz6BG38YgG8uapiDiXpkGI0PkofeRc+BP7xfi21BPlA0m8 nAChM49juwMHyTJZ84si/21YlEGFKM78C0mCB6MnRuuYxJszpj/LJxcKhFavUZLj vv4mBhWslP848l37npcWz5PIZ2KPIy2JBO+orBrYy/NmZPRx28Pq+hJtwv0xtXdv NF/JIUX6H3WqDV83d/7VGMq5CNZx2GgNU32fbbXmgN+9yiywntGesQadnRtKliNF UWZehxrL0ychJUseIaDiHIlfDqohOg0Rlsp8uijV7d2XUDKeDdV+OcZW8FFDRCx8 RNTbXvCONv3EJJJT35VM =uQyc -----END PGP SIGNATURE----- --=-j66XQ7w6rHPMqiiAQZ93-- --===============6844149660999598030== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KWGVuLWRldmVs IG1haWxpbmcgbGlzdApYZW4tZGV2ZWxAbGlzdHMueGVuLm9yZwpodHRwczovL2xpc3RzLnhlbi5v cmcveGVuLWRldmVsCg== --===============6844149660999598030==--