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

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

On 05/11/2017 11:48 AM, Dario Faggioli wrote:
> 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?).

Right.

I didn't realize that do_tasklet() also schedules softirq. So you are
suggesting something along the lines of

        do_tasklet();

        if ( !softirq_pending(smp_processor_id() && !scrub_free_pages() )
            (*pm_idle)();

        do_softirq();


>
>> 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.

Nevermind that. I was thinking we enter idle_loop() based on
cpu_is_haltable().

-boris

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

Attachment: signature.asc
Description: OpenPGP digital signature

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