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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 13.06.17 at 20:20, <boris.ostrovsky@xxxxxxxxxx> wrote:

>>>>> +        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.
>> I'm not convinced - memory-only nodes shouldn't be that common,
>> so in a presumably large share of cases we don't even need to
>> enter this second part of the function. And if a debatable choice
>> is being made, giving the reason in a short comment would help
>> reviewers judge for themselves whether indeed the chosen
>> approach is the lesser of two (or more) possible evils.
> 
> I realize that CPU-less nodes are rare but if we are on such a system we
> will always be adding pressure on the heap lock. Especially on systems
> with many CPUs.
> 
> Even if a CPU unnecessarily loses its turn to scrub because another
> processor held the node and decided not to scrub it the first CPU can
> come back next time when it wakes up from sleep.
> 
> Another alternative could be adding a local lock, just for this routine.

If no accesses elsewhere are affected, that's certainly an option.
But no matter what route you go, please reason about it in the
commit message.

Jan


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