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

[Dario Faggioli <dario.faggioli@xxxxxxxxxx>]

On Thu, 2017-05-11 at 10:19 -0400, Boris Ostrovsky wrote:
> > > 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)
> > >      {
> > >          if ( cpu_is_offline(smp_processor_id()) )
> > >              play_dead();
> > > -        (*pm_idle)();
> > > +        if ( !scrub_free_pages() )
> > > +            (*pm_idle)();
> > >          do_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.
> > 
> We can move do_tasklet() above scrub_free_pages(). And new tasklet
> after
> that would result in a softirq being set so we'd do an early exit
> from
> scrub_free_pages().
> 
How early?

In fact, right now, if there is one tasklet queued, this is what
happens:

 tasklet_schedule(t)
   tasklet_enqueue(t)
     test_and_set_bit(_TASKLET_enqueued, tasklet_work_to_do);
     raise_softirq(SCHEDULE_SOFTIRQ);
 schedule()
   set_bit(_TASKLET_scheduled, tasklet_work_to_do)
   tasklet_work_scheduled = 1;
   do_schedule(tasklet_work_scheduled)
     csched_schedule(tasklet_work_to_do)
       snext = CSCHED_VCPU(idle_vcpu[cpu]);
 idle_loop()
   (*pm_idle)()
     if ( !cpu_is_haltable() ) return;
   do_tasklet() /* runs tasklet t */
     clear_bit(_TASKLET_enqueued, work_to_do); /* list_empty(list) == true */
     raise_softirq(SCHEDULE_SOFTIRQ);
   do_softirq()
 schedule()
   clear_bit(_TASKLET_scheduled, tasklet_work);
   tasklet_work_scheduled = 0;
   do_schedule(tasklet_work_scheduled)
     csched_schedule(tasklet_work_to_do)
       snext = CSCHED_VCPU(idle_vcpu[cpu]);
 idle_loop()
   (*pm_idle)()
     if ( !cpu_is_haltable() )
 ...

If we move do_tasklet up, as you suggest, this is what happens:

 tasklet_schedule(t)
   tasklet_enqueue(t)
     test_and_set_bit(_TASKLET_enqueued, tasklet_work_to_do);
     raise_softirq(SCHEDULE_SOFTIRQ);
 schedule()
   set_bit(_TASKLET_scheduled, tasklet_work_to_do)
   tasklet_work_scheduled = 1;
   do_schedule(tasklet_work_scheduled)
     csched_schedule(tasklet_work_to_do)
       snext = CSCHED_VCPU(idle_vcpu[cpu]);
 idle_loop()
   do_tasklet() /* runs tasklet t */
     clear_bit(_TASKLET_enqueued, work_to_do); /* list_empty(list) == true */
     raise_softirq(SCHEDULE_SOFTIRQ);
   if ( !scrub_free_pages() )
     //do some scrubbing, but softirq_pending() is true, so return 1
   do_softirq()
 schedule()
   clear_bit(_TASKLET_scheduled, tasklet_work);
   tasklet_work_scheduled = 0;
   do_schedule(tasklet_work_scheduled)
     csched_schedule(tasklet_work_to_do)
       snext = CSCHED_VCPU(idle_vcpu[cpu]);
 idle_loop()
   if ( !scrub_free_pages() )
     //do the scrubbing, returns 0, so we enter the if
     (*pm_idle)()
       if ( !cpu_is_haltable() )
 ...

IOW (provided I'm understanding your code right, of course), I still
see it happening that we switched to idle *not* because the system was
idle, but for running a tasklet, and yet we end up doing at least some
scrubbing (like if the system were idle).

Which still feels wrong to me.

If there's more than one tasklet queued (or another one, or more,
is/are queued before the one t is processed), it's even worse, because
we go through the whole schedule()->idle_loop()->do_tasklet() again and
again, and at each step we do a bit of scrubbing, before going back to
schedule().

It probably would be at least a bit better, if scrub_free_pages() would
check for softirqs() _before_ starting any scrubbing (which I don't
think it does, right now, am I right?).

> OTOH since, as you say, we only get to idle loop() if no tasklet is
> pending (cpu_is_haltable() test) then would even that be needed?
> 
Err... sorry, not getting. It's the other way round. One of the reasons
why we end up executing idle_loop(), is that there is at least a
tasklet pending.

Where we only get to if there's nothing pending is to calling
(*pm_idle)().

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