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

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

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. Nor is it immediately clear why this needs to be
> pulled ahead for your purposes.

Are you asking for a comment here (i.e. the explanation given by Dario
(added)  in
https://lists.xenproject.org/archives/html/xen-devel/2017-05/msg01215.html)
or are you saying something is wrong?


>
>> +        if ( node_need_scrub[node] )
>> +        {
>> +            if ( !get_node )
>> +                return node;
>> +
>> +            dist = __node_distance(local_node, node);
>> +            if ( dist < shortest || closest == NUMA_NO_NODE )
>> +            {
>> +                if ( !node_test_and_set(node, node_scrubbing) )
>> +                {
>> +                    if ( closest != NUMA_NO_NODE )
>> +                        node_clear(closest, node_scrubbing);
> You call this function with no locks held, yet you temporarily set a
> bit in node_scrubbing which you then may clear again here. That'll
> prevent another idle CPU to do scrubbing on this node afaict,
> which, while not a bug, is not optimal. Therefore I think for this
> second part of the function you actually want to acquire the heap
> lock.

I actually specifically didn't want to take the heap lock here since we
will be calling this routine quite often and in most cases no scrubbing
will be needed.


-boris

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