From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dario Faggioli Subject: Re: [PATCH v3 4/9] mm: Scrub memory from idle loop Date: Thu, 11 May 2017 12:26:15 +0200 Message-ID: <1494498375.7393.5.camel@citrix.com> References: <1492184258-3277-1-git-send-email-boris.ostrovsky@oracle.com> <1492184258-3277-5-git-send-email-boris.ostrovsky@oracle.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============6958185701205904731==" Return-path: In-Reply-To: <1492184258-3277-5-git-send-email-boris.ostrovsky@oracle.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xen.org Sender: "Xen-devel" To: Boris Ostrovsky , xen-devel@lists.xen.org Cc: sstabellini@kernel.org, wei.liu2@citrix.com, George.Dunlap@eu.citrix.com, andrew.cooper3@citrix.com, ian.jackson@eu.citrix.com, tim@xen.org, jbeulich@suse.com List-Id: xen-devel@lists.xenproject.org --===============6958185701205904731== Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="=-l+/Gc8GOSdHRo5MRZNUv" --=-l+/Gc8GOSdHRo5MRZNUv Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Fri, 2017-04-14 at 11:37 -0400, Boris Ostrovsky wrote: > Instead of scrubbing pages during guest destruction (from > free_heap_pages()) do this opportunistically, from the idle loop. >=20 > Signed-off-by: Boris Ostrovsky > --- > Changes in v3: > * If memory-only nodes exist, select the closest one for scrubbing > * Don't scrub from idle loop until we reach SYS_STATE_active. >=20 > =C2=A0xen/arch/arm/domain.c=C2=A0=C2=A0=C2=A0|=C2=A0=C2=A0=C2=A013 ++++-- > =C2=A0xen/arch/x86/domain.c=C2=A0=C2=A0=C2=A0|=C2=A0=C2=A0=C2=A0=C2=A03 += - > =C2=A0xen/common/page_alloc.c |=C2=A0=C2=A0=C2=A098 > +++++++++++++++++++++++++++++++++++++++++----- > =C2=A0xen/include/xen/mm.h=C2=A0=C2=A0=C2=A0=C2=A0|=C2=A0=C2=A0=C2=A0=C2= =A01 + > =C2=A04 files changed, 98 insertions(+), 17 deletions(-) >=20 > diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c > index 76310ed..38d6331 100644 > --- a/xen/arch/arm/domain.c > +++ b/xen/arch/arm/domain.c > @@ -46,13 +46,16 @@ void idle_loop(void) > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if ( cpu_is_offline= (smp_processor_id()) ) > =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=A0stop_cpu(); > =C2=A0 > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0local_irq_disable(); > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if ( cpu_is_haltable(smp= _processor_id()) ) > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if ( !scrub_free_pages()= ) > =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= dsb(sy); > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= wfi(); > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= local_irq_disable(); > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= if ( cpu_is_haltable(smp_processor_id()) ) > +=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=A0dsb(sy); > +=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=A0wfi(); > +=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= local_irq_enable(); > =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=A0local_irq_enable(); > =C2=A0 > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0do_tasklet(); > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0do_softirq(); > diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c > index 90e2b1f..a5f62b5 100644 > --- a/xen/arch/x86/domain.c > +++ b/xen/arch/x86/domain.c > @@ -118,7 +118,8 @@ static void idle_loop(void) > =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=A0if ( cpu_is_offline= (smp_processor_id()) ) > =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=A0play_dead(); > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0(*pm_idle)(); > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if ( !scrub_free_pages()= ) > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= (*pm_idle)(); > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0do_tasklet(); > This means that, if we got here to run a tasklet (as in, if the idle vCPU has been forced into execution, because there were a vCPU context tasklet wanting to run), we will (potentially) do some scrubbing first. Is this on purpose, and, in any case, ideal? vCPU context tasklets are not terribly common, but I still don't think it is (ideal). Not sure how to address this, though. What (the variants of) pm_idle() uses for deciding whether or not to actually go to sleep is cpu_is_haltable(), which checks per_cpu(tasklet_work_to_do, cpu): /* * Used by idle loop to decide whether there is work to do: * (1) Run softirqs; or (2) Play dead; or (3) Run tasklets. */ #define cpu_is_haltable(cpu) \ (!softirq_pending(cpu) && \ cpu_online(cpu) && \ !per_cpu(tasklet_work_to_do, cpu)) Pulling it out/adding a call to it (cpu_is_haltable()) is ugly, and probably not what we want=C2=A0(e.g., it's always called with IRQs disabled= , while they're on here). Maybe we can test tasklet_work_to_do, before calling scrub_free_pages() (also ugly, IMO). Or, if scrub_free_pages() is, and always will be, called only from here, within the idle loop, test tasklet_work_to_do inside, similarly to what it does already for pending softirqs... Regards, Dario --=20 <> (Raistlin Majere) ----------------------------------------------------------------- Dario Faggioli, Ph.D, http://about.me/dario.faggioli Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK) --=-l+/Gc8GOSdHRo5MRZNUv 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 iQIcBAABCAAGBQJZFDxIAAoJEBZCeImluHPuDTUP/00UBE1p8knoRk/JhF1rAa1s 5KQ6hyqhz0vL7tajkYQNidxCSEfnMX8rkdTqgX36WHfPSvsHxN9oBqvbKWnKUNFi v80Ga6HUZthQMNYgQ35jmcJcb74azqIjMT8Zv1Cgz78G4jdz7hWtq7M8AAdwCtkB K77xpm1XkhN9NZi38bjQjpTtNFrhHe7XzYvw+zrhvRUfSmVSGyoAe0f1GM/tG8OC supfvjpfvgYvZbfv23vtkZtzZGb10y01EUK9mLZmlg2ZIHnEa9p6DsEM80hpazfX rqH0qmf3yFIhUsk0YabNa3Fh5pJG8yRbYNNY2JBRTfUP8nKHc8DlbNwqgjXwbuG+ 9GJBvRVNS8y4JiDegs72kAmXem05qUOwwWkoTn51SdI/ZfFMSlwnGmtOw7qOTRbI r0t1BPSFitavHGd6iDSMaBpz2b0Njd7MLEcKMaAAdxUgJffcTm2IB/IKKHZ+b2e+ s7ScLpQko7CUKIUGIs7/H1tuk0KW4zSS6DZEAM3a6T7RRZRLLvTEXEiySs5bUbbI aHnUdjJkAp14WufnERp1ovVg/MiucRt1eI+qrIKP1xjVvXOh7H9ryH6T32p8HxnL C4zYR7s/T1mJqtP6jfUJczeQFn1/pgk0+WEJf6XwOKC4jar18Nhy6BxzWt5nFDbT 1RDCw9nyp+/0G46CMfL0 =a4ky -----END PGP SIGNATURE----- --=-l+/Gc8GOSdHRo5MRZNUv-- --===============6958185701205904731== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KWGVuLWRldmVs IG1haWxpbmcgbGlzdApYZW4tZGV2ZWxAbGlzdHMueGVuLm9yZwpodHRwczovL2xpc3RzLnhlbi5v cmcveGVuLWRldmVsCg== --===============6958185701205904731==--