From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dario Faggioli Subject: Re: [PATCH v4 4/8] mm: Scrub memory from idle loop Date: Mon, 12 Jun 2017 23:28:36 +0200 Message-ID: <1497302916.26212.30.camel@citrix.com> References: <1495209040-11101-1-git-send-email-boris.ostrovsky@oracle.com> <1495209040-11101-5-git-send-email-boris.ostrovsky@oracle.com> <593E68040200007800161CD1@prv-mh.provo.novell.com> <321c791e-d732-1730-d529-35ad9506b9a9@oracle.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1858895578111411840==" Return-path: In-Reply-To: <321c791e-d732-1730-d529-35ad9506b9a9@oracle.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xen.org Sender: "Xen-devel" To: Boris Ostrovsky , Jan Beulich Cc: sstabellini@kernel.org, wei.liu2@citrix.com, George.Dunlap@eu.citrix.com, ian.jackson@eu.citrix.com, tim@xen.org, xen-devel@lists.xen.org, andrew.cooper3@citrix.com List-Id: xen-devel@lists.xenproject.org --===============1858895578111411840== Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="=-X8e57Ug2HsyCokNaHs2s" --=-X8e57Ug2HsyCokNaHs2s Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Mon, 2017-06-12 at 13:01 -0400, Boris Ostrovsky wrote: > On 06/12/2017 04:08 AM, Jan Beulich wrote: > > > > > On 19.05.17 at 17:50, wrote: > > >=20 > > > Instead of scrubbing pages during guest destruction (from > > > free_heap_pages()) do this opportunistically, from the idle loop. > >=20 > > This is too brief for my taste. In particular the re-ordering ... > >=20 > > > --- a/xen/arch/x86/domain.c > > > +++ b/xen/arch/x86/domain.c > > > @@ -118,8 +118,9 @@ 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_off= line(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=A0=C2=A0do_tasklet(); > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if ( cpu_is_haltable= (smp_processor_id()) && > > > !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_softirq(); > >=20 > > ... here (and its correctness / safety) needs an explanation. Not > > processing tasklets right after idle wakeup is a not obviously > > correct change.=20 > Well, one can also see things the other way round, though. I.e.: considering that do_tasklet() is here for when we force the idle vcpu into execution right because we want to process tasklets, doing that only after having tried to sleep is not obviously correct. And in fact, there's an unwritten (AFAICT) requirement that every implementation of pm_idle should not actually sleep if there are tasklets pending. Truth is, IMO, that we may be here for two reasons: 1) going to sleep or 2) running tasklet, and the only think we can do is guessing (and ordering the call according to such guess) and checking whether we guessed right or wrong. That is: - guess it's 1. Check whether it's really 1. If it is, go ahead with =20 =C2=A0 it; if not, go for 2; - guess it's 2. Check whether it's really 2. If it is, go ahead with it, if not, go for 1; Now scrubbing is kind of a third reason why we may be here, and doing as done in the code above (although I'm not super happy of the final look of the result either), should make all the use cases happy. Also, what's the scenario where you think this may be problematic? AFAICT, tasklets are vcpu context, or softirq context. If some softirq context tasklet work is scheduled for a CPU while it is sleeping, TASKLET_SOFTIRQ is raised, and the call to do_softirq() --which still happens right after the wakeup-- will take care of it. If some vcpu context work is scheduled, SCHEDULE_SOFTIRQ is raised. do_softirq() will call the scheduler, which will see that there is vcpu tasklet work to do, and hence confirm in execution the idle vcpu, which will get to execute do_tasklet(). Anyway... > > Nor is it immediately clear why this needs to be > > pulled ahead for your purposes. >=20 > Are you asking for a comment here (i.e. the explanation given by > Dario > (added)=C2=A0=C2=A0in > https://lists.xenproject.org/archives/html/xen-devel/2017-05/msg01215 > .html) > or are you saying something is wrong? >=20 ...If it's more commenting that's being asked, either in the code or in the changelog, that would indeed improve things, I agree. Regards, Dario --=20 <> (Raistlin Majere) ----------------------------------------------------------------- Dario Faggioli, Ph.D, http://about.me/dario.faggioli Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK) --=-X8e57Ug2HsyCokNaHs2s 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 iQIcBAABCAAGBQJZPweEAAoJEBZCeImluHPuuXUP/338Tds0dCSc8+i3OqoIgXUd Mw6lcT85x0e8wV3B4SfA7mzr2DzjIjM06NRZvx46mWXtl73LaC+HqrkbH0KqOJjF +QHMUR8YgeyNHGcW3KeBLcPuldwSQQTpK3xXGBA2OG0YDfy93LzWAI49I/iLH7Gu y648dGcYnjNG4K3NroicsRqsL0yRZMDTLFGNMZx0RNvllMoWWHTzoS0lHLFaB4vm czqdmFrabD7Jx1F4jc3XlC/sftBo4JNMKjEP1OWajLzzSMsI+k7k7vAhHBkoyJhx qNInLsCQw0rDHq86oEyGP+489mALEro2Ygs/cy061yJCsMy2jkMbGZXoaVOMULhI R7GxR6brykrfk/X7rWx5kOh2hM7byzWDbWhyRfZuMhHlmHLmCjmDlE0Kok9080NI qivfzpLG2vqCx8VUi1/UGHqqVGwRb1wJZgCy2s+a8qGdaF3JVt3QnurgMlkYi5PY 0Hjp3DmibG/CoLq4SEH998yklOH375njksuXLLutM9p6nUX8mOTQbKv5+vU9Vkw9 ZCImfRkhrXs1Ow7x30JejlR1OttKMAzWqGNUvh1qFDG5WNe6kZv7hJhLbVpQVI2d vSJsAQXX8pDtQ3/BceKQSwZolLx+nkgWECMZpeP2AHZAoBpDwOMRaVLoRDhz8j6Q zxnZjfS8AtSqnaCFV/LL =mSFq -----END PGP SIGNATURE----- --=-X8e57Ug2HsyCokNaHs2s-- --===============1858895578111411840== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KWGVuLWRldmVs IG1haWxpbmcgbGlzdApYZW4tZGV2ZWxAbGlzdHMueGVuLm9yZwpodHRwczovL2xpc3RzLnhlbi5v cmcveGVuLWRldmVsCg== --===============1858895578111411840==--