Re: [patch for-5.3 0/4] revert immediate fallback to remote hugepages

[Vlastimil Babka]

On 11/25/19 9:38 PM, David Rientjes wrote:
> On Mon, 25 Nov 2019, Michal Hocko wrote:
> 
>>> So my question would be: if we know the previous behavior that allowed 
>>> excessive swap and recalling into compaction was deemed harmful for the 
>>> local node, why do we now believe it cannot be harmful if done for all 
>>> system memory?
>>
>> I have to say that I got lost in your explanation. I have already
>> pointed this out in a previous email you didn't reply to. But the main
>> difference to previous __GFP_THISNODE behavior is that it is used along
>> with __GFP_NORETRY and that reduces the overall effort of the reclaim
>> AFAIU. If that is not the case then please be _explicit_ why.
>>
> 
> I'm referring to the second allocation in alloc_pages_vma() after the 
> patch:
> 
>  			/*
>  			 * If hugepage allocations are configured to always
>  			 * synchronous compact or the vma has been madvised
>  			 * to prefer hugepage backing, retry allowing remote
> -			 * memory as well.
> +			 * memory with both reclaim and compact as well.
>  			 */
>  			if (!page && (gfp & __GFP_DIRECT_RECLAIM))
>  				page = __alloc_pages_node(hpage_node,
> - 						gfp | __GFP_NORETRY, order);
> +							gfp, order);
> 
> So we now do not have __GFP_NORETRY nor __GFP_THISNODE so this bypasses 
> all the precautionary logic in the page allocator that avoids excessive 
> swap: it is free to continue looping, swapping, and thrashing, trying to 
> allocate hugepages if all memory is fragmented.

Well, it's not completely free to do all that, there's other
reclaim/compaction logic that terminates the attempts. If it's
insufficient for some workloads, I would like to know, with hard data. I
think the past observations of thrashing with __GFP_THISNODE do not
prove the reclaim/compaction logic is wrong, see below.

> Qemu uses MADV_HUGEPAGE so this allocation *will* be attempted for 
> Andrea's workload.  The swap storms were reported for the same allocation 
> but with __GFP_THISNODE so it only occurred for local fragmentation and 
> low-on-memory conditions for the local node in the past.  This is now 
> opened up for all nodes.

I think it's also possible to explain the thrashing with __GFP_THISNODE
without the premise of local fragmentation and suboptimal
reclaim/compact decisions. THP __GFP_THISNODE allocations might simply
put too much pressure on the local node with a workload that might be
sized for the whole system. It's the same problem as with the node
reclaim mode. Even if there's no bad fragmentation and THP allocations
succeed without much trouble, they fill the node and cause reclaim (both
kswapd and direct). Fallback order-0 allocations are not restricted, so
they will use all nodes. The new THP retry without __GFP_THISNODE is
also not restricted, so it's very much possible it will not cause this
thrashing with a properly sized workload for the whole system.

> So the question is: what prevents the exact same issue from happening 
> again for Andrea's usecase if all memory on the system is fragmented?  I'm 
> assuming that if this were tested under such conditions that the swap 
> storms would be much worse.

We will only know if Andrea tests it. But if his workloads were fine
with just the initial __GFP_THISNODE reverts, they should be still fine
after this patch?