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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

Jan


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