On Wed, 13 Nov 2019, Mel Gorman wrote: > > > The whole point of the Vlastimil's patch is to have an optimistic local > > > node allocation first and the full gfp context one in the fallback path. > > > If our full gfp context doesn't really work well then we can revisit > > > that of course but that should happen at alloc_hugepage_direct_gfpmask > > > level. > > > > Since the patch reverts the precaution put into the page allocator to not > > attempt reclaim if the allocation order is significantly large and the > > return value from compaction specifies it is unlikely to succed on its > > own, I believe Vlastimil's patch will cause the same regression that > > Andrea saw is the whole host is low on memory and/or significantly > > fragmented. So the suggestion was that he test this change to make sure > > we aren't introducing a regression for his workload. > > TLDR: I do not have evidence that Vlastimil's patch causes more swapping > but more information is needed from Andrea on exactly how he's > testing this. It's not clear to me what was originally tested > and whether memory just had to be full or whether it had to be > fragmented. If fragmented, then we have to agree on what an > appropriate mechanism is for fragmenting memory. Hypothetical > kernel modules that don't exist do not count. > > I put together a testcase whereby a virtual machine is deployed, started > and then time how long it takes to run memhog on 80% of the guests > physical memory. I varied how large the virtual machine is and ran it on > a 2-socket machine so that the smaller tests would be single node and > the larger tests would span both nodes. Before each startup, a large > file is read to fill the memory with pagecache. > First, thanks very much for the follow-up and considerable amount of time testing and benchmarking this. I, like you, do not have a reliable test case that will reproduce the issue that Andrea initially reported over a year ago. I believe in the discussion that repeatedly referred to swap storms that, with the __GFP_THISNODE policy, we were not thrashing because the local node was low on memory due to page cache. How memory is filled with page cache will naturally effect how it can be reclaimed when compaction fails locally, I don't know if it's an accurate representation of the initial problem. I also don't recall details about the swapfile or exactly where we were contending while trying to fault local hugepages. My concern, and it's only a concern at this point and not a regression report because we don't have a result from Andrea, is that the result of this patch is that the second allocation in alloc_pages_vma() enables the exact same allocation policy that Andrea reported was a problem earlier if __GFP_DIRECT_RECLAIM is set, which it will be as a result of qemu's use of MADV_HUGEPAGE. That reclaim and compaction is now done over the entire system and not isolated only to the local node so there are two plausible outcomes: (1) the remote note is not fragmented and we can easily fault a remote hugepage or (2) we thrash and cause swap storms remotely as well as locally. (1) is the outcome that Andrea is seeking based on the initial reverts: that much we know. So my concern is that if the *system* is fragmented that we have now introduced a much more significant swap storm that will result in a much more serious regression. 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? The patch subverts the precaution put into place in the page allocator to specifically not do this excessive reclaim and recall into compaction dance and I believe restores the previous bad behavior if remote memory is similarly fragmented. (What prevents this??) Andrea was able to test this on several kernel versions with a fragmented local node so I *assume* it would not be difficult to measure the extent to which this patch can become harmful if all memory is fragmented. I'm hoping that we can quantify that potentially negative impact before opening users up to the possibility. As you said, it behaves better on some systems and workloads and worse on others and we both agree more information is needed. I think asking Andrea to test and quantify the change with a fragmented system would help us to make a more informed decision and not add a potential regression to 5.5 or whichever kernel this would be merged in.
