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

[Boris Ostrovsky <boris.ostrovsky@oracle.com>]

On 06/13/2017 04:19 AM, Jan Beulich wrote:
>>>> On 12.06.17 at 23:28, <dario.faggioli@citrix.com> wrote:
>> 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@oracle.com> 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.
> That's a valid point, but would then call for the re-ordering to
> be done in a separate commit with proper explanation.
>
>> And in fact, there's an unwritten (AFAICT) requirement that every
>> implementation of pm_idle should not actually sleep if there are
>> tasklets pending.
> Unwritten or not - that check before actually going to sleep is
> quite obviously required, as it needs to be done with interrupts
> already disabled (i.e. can't be done _only_ here).
>
>> 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?
> First of all I'm not sure there's anything problematic here. But with
> no explanation given at all, the change also isn't obviously fine, as
> it does alter behavior. If there's indeed nothing that can affect
> what do_tasklet() would do and that might happen while we're in
> the low level idle handler, then fine. But this needs to be proven.
>
>> 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().
> Right, so something along these lines will need to go into the commit
> message.


So would you then prefer to separate this into two patches, with the
first just moving do_tasklet() above sleeping?

-boris


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