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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 13.06.17 at 20:39, <boris.ostrovsky@xxxxxxxxxx> wrote:
> On 06/13/2017 04:19 AM, Jan Beulich wrote:
>>>>> On 12.06.17 at 23:28, <dario.faggioli@xxxxxxxxxx> 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@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.
>> 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?

Yes, please (no matter who of you two does so).

Jan


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