Re: [Xen-devel] [PATCH v4 4/8] mm: Scrub memory from idle loop

[Dario Faggioli <dario.faggioli@xxxxxxxxxx>]

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, <boris.ostrovsky@xxxxxxxxxx> wrote:
> > > 
> > > Instead of scrubbing pages during guest destruction (from
> > > free_heap_pages()) do this opportunistically, from the idle loop.
> > 
> > This is too brief for my taste. In particular the re-ordering ...
> > 
> > > --- a/xen/arch/x86/domain.c
> > > +++ b/xen/arch/x86/domain.c
> > > @@ -118,8 +118,9 @@ static void idle_loop(void)
> > >      {
> > >          if ( cpu_is_offline(smp_processor_id()) )
> > >              play_dead();
> > > -        (*pm_idle)();
> > >          do_tasklet();
> > > +        if ( cpu_is_haltable(smp_processor_id()) &&
> > > !scrub_free_pages() )
> > > +            (*pm_idle)();
> > >          do_softirq();
> > 
> > ... here (and its correctness / safety) needs an explanation. Not
> > processing tasklets right after idle wakeup is a not obviously
> > correct change. 
>
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  
    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.
> 
> Are you asking for a comment here (i.e. the explanation given by
> Dario
> (added)  in
> https://lists.xenproject.org/archives/html/xen-devel/2017-05/msg01215
> .html)
> or are you saying something is wrong?
> 
...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
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)

Attachment: signature.asc
Description: This is a digitally signed message part

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel