On Tue 25-09-18 13:20:08, Mel Gorman wrote:

On Tue, Sep 25, 2018 at 02:03:25PM +0200, Michal Hocko wrote:

...

From: Andrea Arcangeli aarcange@redhat.com

THP allocation might be really disruptive when allocated on NUMA system with the local node full or hard to reclaim. Stefan has posted an allocation stall report on 4.12 based SLES kernel which suggests the same issue:

[245513.362669] kvm: page allocation stalls for 194572ms, order:9, mode:0x4740ca(__GFP_HIGHMEM|__GFP_IO|__GFP_FS|__GFP_COMP|__GFP_NOMEMALLOC|__GFP_HARDWALL|__GFP_THISNODE|__GFP_MOVABLE|__GFP_DIRECT_RECLAIM), nodemask=(null) [245513.363983] kvm cpuset=/ mems_allowed=0-1 [245513.364604] CPU: 10 PID: 84752 Comm: kvm Tainted: G W 4.12.0+98-ph <a href="/view.php?id=1" title="[geschlossen] Integration Ramdisk" class="resolved">0000001</a> SLE15 (unreleased) [245513.365258] Hardware name: Supermicro SYS-1029P-WTRT/X11DDW-NT, BIOS 2.0 12/05/2017 [245513.365905] Call Trace: [245513.366535] dump_stack+0x5c/0x84 [245513.367148] warn_alloc+0xe0/0x180 [245513.367769] __alloc_pages_slowpath+0x820/0xc90 [245513.368406] ? __slab_free+0xa9/0x2f0 [245513.369048] ? __slab_free+0xa9/0x2f0 [245513.369671] __alloc_pages_nodemask+0x1cc/0x210 [245513.370300] alloc_pages_vma+0x1e5/0x280 [245513.370921] do_huge_pmd_wp_page+0x83f/0xf00 [245513.371554] ? set_huge_zero_page.isra.52.part.53+0x9b/0xb0 [245513.372184] ? do_huge_pmd_anonymous_page+0x631/0x6d0 [245513.372812] __handle_mm_fault+0x93d/0x1060 [245513.373439] handle_mm_fault+0xc6/0x1b0 [245513.374042] __do_page_fault+0x230/0x430 [245513.374679] ? get_vtime_delta+0x13/0xb0 [245513.375411] do_page_fault+0x2a/0x70 [245513.376145] ? page_fault+0x65/0x80 [245513.376882] page_fault+0x7b/0x80 [...] [245513.382056] Mem-Info: [245513.382634] active_anon:126315487 inactive_anon:1612476 isolated_anon:5 active_file:60183 inactive_file:245285 isolated_file:0 unevictable:15657 dirty:286 writeback:1 unstable:0 slab_reclaimable:75543 slab_unreclaimable:2509111 mapped:81814 shmem:31764 pagetables:370616 bounce:0 free:32294031 free_pcp:6233 free_cma:0 [245513.386615] Node 0 active_anon:254680388kB inactive_anon:1112760kB active_file:240648kB inactive_file:981168kB unevictable:13368kB isolated(anon):0kB isolated(file):0kB mapped:280240kB dirty:1144kB writeback:0kB shmem:95832kB shmem_thp: 0kB shmem_pmdmapped: 0kB anon_thp: 81225728kB writeback_tmp:0kB unstable:0kB all_unreclaimable? no [245513.388650] Node 1 active_anon:250583072kB inactive_anon:5337144kB active_file:84kB inactive_file:0kB unevictable:49260kB isolated(anon):20kB isolated(file):0kB mapped:47016kB dirty:0kB writeback:4kB shmem:31224kB shmem_thp: 0kB shmem_pmdmapped: 0kB anon_thp: 31897600kB writeback_tmp:0kB unstable:0kB all_unreclaimable? no

The defrag mode is "madvise" and from the above report it is clear that the THP has been allocated for MADV_HUGEPAGA vma.

Andrea has identified that the main source of the problem is __GFP_THISNODE usage:

: The problem is that direct compaction combined with the NUMA : __GFP_THISNODE logic in mempolicy.c is telling reclaim to swap very : hard the local node, instead of failing the allocation if there's no : THP available in the local node. : : Such logic was ok until __GFP_THISNODE was added to the THP allocation : path even with MPOL_DEFAULT. : : The idea behind the __GFP_THISNODE addition, is that it is better to : provide local memory in PAGE_SIZE units than to use remote NUMA THP : backed memory. That largely depends on the remote latency though, on : threadrippers for example the overhead is relatively low in my : experience. : : The combination of __GFP_THISNODE and __GFP_DIRECT_RECLAIM results in : extremely slow qemu startup with vfio, if the VM is larger than the : size of one host NUMA node. This is because it will try very hard to : unsuccessfully swapout get_user_pages pinned pages as result of the : __GFP_THISNODE being set, instead of falling back to PAGE_SIZE : allocations and instead of trying to allocate THP on other nodes (it : would be even worse without vfio type1 GUP pins of course, except it'd : be swapping heavily instead).

Fix this by removing __GFP_THISNODE for THP requests which are requesting the direct reclaim. This effectivelly reverts 5265047ac301 on the grounds that the zone/node reclaim was known to be disruptive due to premature reclaim when there was memory free. While it made sense at the time for HPC workloads without NUMA awareness on rare machines, it was ultimately harmful in the majority of cases. The existing behaviour is similiar, if not as widespare as it applies to a corner case but crucially, it cannot be tuned around like zone_reclaim_mode can. The default behaviour should always be to cause the least harm for the common case.

If there are specialised use cases out there that want zone_reclaim_mode in specific cases, then it can be built on top. Longterm we should consider a memory policy which allows for the node reclaim like behavior for the specific memory ranges which would allow a

[1] http://lkml.kernel.org/r/20180820032204.9591-1-aarcange@redhat.com

[mhocko@suse.com: rewrote the changelog based on the one from Andrea] Fixes: 5265047ac301 ("mm, thp: really limit transparent hugepage allocation to local node") Cc: Zi Yan zi.yan@cs.rutgers.edu Cc: stable # 4.1+ Reported-by: Stefan Priebe s.priebe@profihost.ag Debugged-by: Andrea Arcangeli aarcange@redhat.com Reported-by: Alex Williamson alex.williamson@redhat.com Signed-off-by: Andrea Arcangeli aarcange@redhat.com Signed-off-by: Michal Hocko mhocko@suse.com

Reviewed-by: Mel Gorman mgorman@techsingularity.net

Both patches look correct to me but I'm responding to this one because it's the fix. The change makes sense and moves further away from the severe stalling behaviour we used to see with both THP and zone reclaim mode.

I put together a basic experiment with usemem configured to reference a buffer multiple times that is 80% the size of main memory on a 2-socket box with symmetric node sizes and defrag set to "always". The defrag setting is not the default but it would be functionally similar to accessing a buffer with madvise(MADV_HUGEPAGE). Usemem is configured to reference the buffer multiple times and while it's not an interesting workload, it would be expected to complete reasonably quickly as it fits within memory. The results were;

usemem vanilla noreclaim-v1 Amean Elapsd-1 42.78 ( 0.00%) 26.87 ( 37.18%) Amean Elapsd-3 27.55 ( 0.00%) 7.44 ( 73.00%) Amean Elapsd-4 5.72 ( 0.00%) 5.69 ( 0.45%)

This shows the elapsed time in seconds for 1 thread, 3 threads and 4 threads referencing buffers 80% the size of memory. With the patches applied, it's 37.18% faster for the single thread and 73% faster with two threads. Note that 4 threads showing little difference does not indicate the problem is related to thread counts. It's simply the case that 4 threads gets spread so their workload mostly fits in one node.

The overall view from /proc/vmstats is more startling

                     4.19.0-rc1  4.19.0-rc1
                        vanillanoreclaim-v1r1

Minor Faults 35593425 708164 Major Faults 484088 36 Swap Ins 3772837 0 Swap Outs 3932295 0

Massive amounts of swap in/out without the patch

Direct pages scanned 6013214 0 Kswapd pages scanned 0 0 Kswapd pages reclaimed 0 0 Direct pages reclaimed 4033009 0

Lots of reclaim activity without the patch

Kswapd efficiency 100% 100% Kswapd velocity 0.000 0.000 Direct efficiency 67% 100% Direct velocity 11191.956 0.000

Mostly from direct reclaim context as you'd expect without the patch.

Page writes by reclaim 3932314.000 0.000 Page writes file 19 0 Page writes anon 3932295 0 Page reclaim immediate 42336 0

Writes from reclaim context is never good but the patch eliminates it.

We should never have default behaviour to thrash the system for such a basic workload. If zone reclaim mode behaviour is ever desired but on a single task instead of a global basis then the sensible option is to build a mempolicy that enforces that behaviour.

Thanks a lot for numbers Mel!

Hello David,

On Thu, Oct 04, 2018 at 01:16:32PM -0700, David Rientjes wrote:

There are ways to address this without introducing regressions for existing users of MADV_HUGEPAGE: introduce an madvise() mode to accept remote thp allocations, which users of this library would never set, or fix memory compaction so that it does not incur substantial allocation latency when it will likely fail.

These librarians needs to call a new MADV_ and the current MADV_HUGEPAGE should not be affected because the new MADV_ will require some capbility (i.e. root privilege).

qemu was the first user of MADV_HUGEPAGE and I don't think it's fair to break it and require change to it to run at higher privilege to retain the direct compaction behavior of MADV_HUGEPAGE.

The new behavior you ask to retain in MADV_HUGEPAGE, generated the same misbehavior to VM as mlock could have done too, so it can't just be given by default without any privilege whatsoever.

Ok you could mitigate the breakage that MADV_HUGEPAGE could have generated (before the recent fix) by isolating malicious or inefficient programs with memcg, but by default in a multiuser system without cgroups the global disruption provided before the fix (i.e. the pathological THP behavior) is not warranted. memcg shouldn't be mandatory to avoid a process to affect the VM in such a strong way (i.e. all other processes who happened to be allocated in the node where the THP allocation triggered, being trashed in swap like if all memory of all other nodes was not completely free).

Not only that, it's not only about malicious processes it's also excessively inefficient for processes that just don't fit in a local node and use MADV_HUGEPAGE. Your processes all fit in the local node for sure if they're happy about it. This was reported as a "pathological THP regression" after all in a workload that couldn't swap at all because of the iommu gup persistent refcount pins.

The alternative patch I posted which still invoked direct reclaim locally, and falled back to NUMA-local PAGE_SIZEd allocations instead of falling back to NUMA-remote THP, could have been given without privilege, but it still won't swapout as this patch would have done, so it still won't go as far as the MADV_HUGEPAGE behavior had before the fix (for the lib users that strongly needed local memory).

Overall I think the call about the default behavior of MADV_HUGEPAGE is still between removing __GFP_THISNODE if gfp_flags can reclaim (the fix in -mm), or by changing direct compaction to only call compaction and not reclaim (i.e. __GFP_COMPACT_ONLY) when __GFP_THISNODE is set.

To go beyond that some privilege is needed and a new MADV_ flag can require privilege or return error if there's not enough privilege. So the lib with 100's users can try to use that new flag first, show an error in stderr (maybe under debug), and fallback to MADV_HUGEPAGE if the app hasn't enough privilege. The alternative is to add a new mem policy less strict than MPOL_BIND to achieve what you need on top of MADV_HUGEPAGE (which also would require some privilege of course as all mbinds). I assume you already evaluated the preferred and local mbinds and it's not a perfect fit?

If we keep this as a new MADV_HUGEPAGE_FORCE_LOCAL flag, you could still add a THP sysfs/sysctl control to lift the privilege requirement marking it as insecure setting in docs (mm/transparent_hugepage/madv_hugepage_force_local=0|1 forced to 0 by default). This would be on the same lines of other sysctl that increase the max number of files open and such things (perhaps a sysctl would be better in fact for tuning in /etc/sysctl.conf).

Note there was still some improvement left possible in my __GFP_COMPACT_ONLY patch alternative. Notably if the watermarks for the local node shown the local node not to have enough real "free" PAGE_SIZEd pages to succeed the PAGE_SIZEd local THP allocation if compaction failed, we should have relaxed __GFP_THISNODE and tried to allocate THP from the NUMA-remote nodes before falling back to PAGE_SIZEd allocations. That also won't require any new privilege.

To get the same behavior as before though you would need to drop all caches with echo 3 >drop_caches before the app starts (and it still won't swap anon memory which previous MADV_HUGEPAGE behavior would have, but the whole point is that the previous MADV_HUGEPAGE behavior would have backfired for the lib too if the app was allocating so much RAM from the local node that it required swapouts of local anon memory).

Thanks, Andrea

Hello,

On Thu, Oct 04, 2018 at 04:05:26PM -0700, David Rientjes wrote:

The source of the problem needs to be addressed: memory compaction. We regress because we lose __GFP_NORETRY and pointlessly try reclaim, but

I commented in detail about the __GFP_NORETRY topic in the other email so I will skip the discussion about __GFP_NORETRY in the context of this answer except for the comment at the end of the email to the actual code that implements __GFP_NORETRY.

But that's a memory compaction issue, not a thp gfp mask issue; the reclaim issue is responded to below.

Actually memory compaction has no issues whatsoever with __GFP_THISNODE regardless of __GFP_NORETRY.

This patch causes an even worse regression if all system memory is fragmented such that thp cannot be allocated because it tries to stress compaction on remote nodes, which ends up unsuccessfully, not just the local node.

On Haswell, when all memory is fragmented (not just the local node as I obtained by 13.9% regression result), the patch results in a fault latency regression of 40.9% for MADV_HUGEPAGE region of 8GB. This is because it is thrashing both nodes pointlessly instead of just failing for __GFP_THISNODE.

There's no I/O involved at the very least on compaction, nor we drop any cache or shrink any slab by mistake by just invoking compaction. Even when you hit the worst case "all nodes are 100% fragmented" scenario that generates the 40% increased allocation latency, all other tasks running in the local node will keep running fine, and they won't be pushed away forcefully into swap with all their kernel cache depleted, which is a mlock/mbind privileged behavior that the app using the MADV_HUGEPAGE lib should not ever been able to inflict on other processes running in the node from different users (users as in uid).

Furthermore when you incur the worst case latency after that there's compact deferred logic skipping compaction next time around if all nodes were so fragmented to the point of guaranteed failure. While there's nothing stopping reclaim to run every time COMPACT_SKIPPED is returned just because compaction keeps succeeding as reclaim keeps pushing more 2M amounts into swap from the local nodes.

I don't doubt with 1024 nodes things can get pretty bad when they're all 100% fragmented, __GFP_THISNODE would win in such case, but then what you're asking then is the __GFP_COMPACT_ONLY behavior. That will solve it.

What we'd need probably regardless of how we solve this bug (because not all compaction invocations are THP invocations... and we can't keep making special cases and optimizations tailored for THP or we end up in that same 40% higher latency for large skbs and other stuff) is a more sophisticated COMPACT_DEFERRED logic where you can track when remote compaction failed. Then you wait many more times before trying a global compaction. It could be achieved with just a compact_deferred counter in the zone/pgdat (wherever it fits best).

Overall I don't think the bug we're dealing with and the slowdown of compaction on the remote nodes are comparable, also considering the latter will still happen regardless if you've large skbs or other drivers allocating large amounts of memory as an optimization.

So the end result is that the patch regresses access latency forever by 13.9% when the local node is fragmented because it is accessing remote thp vs local pages of the native page size, and regresses fault latency of 40.9% when the system is fully fragmented. The only time that fault latency is improved is when remote memory is not fully fragmented, but then you must incur the remote access latency.

You get THP however which will reduce the TLB miss cost and maximize TLB usage, so it depends on the app if that 13.9% cost is actually offseted by the THP benefit or not.

It entirely depends if large part of the workload mostly fits in in-socket CPU cache. The more the in-socket/node CPU cache pays off, the more remote-THP also pays off. There would be definitely workloads that would run faster, not slower, with the remote THP instead of local PAGE_SIZEd memory. The benefit of THP is also larger for the guest loads than for host loads, so it depends on that too.

We agree about the latency issue with a ton of RAM and thousands of nodes, but again that can be mitigated with a NUMA friendly COMPACT_DEFERRED logic NUMA aware. Even without such NUMA-aware-compact_deferred logic improvement, the worst case of the remote compaction behavior still doesn't look nearly as bad as this bug by thinking about it. And it only is a concern for extremely large NUMA systems (which may run the risk of running in other solubility issues in other places if random workloads are applied to it and all nodes are low on memory and fully fragmented which is far from common scenario on those large systems), while the bug we fixed was hurting badly all very common 2 nodes installs with workloads that are common and should run fine.

Direct reclaim doesn't make much sense for thp allocations if compaction has failed, even for MADV_HUGEPAGE. I've discounted Mel's results because he is using thp defrag set to "always", which includes __GFP_NORETRY but the default option and anything else other than "always" does not use __GFP_NORETRY like the page allocator believes it does:

            /*
             * Checks for costly allocations with __GFP_NORETRY, which
             * includes THP page fault allocations
             */
            if (costly_order && (gfp_mask & __GFP_NORETRY)) {
                    /*
                     * If compaction is deferred for high-order allocations,
                     * it is because sync compaction recently failed. If
                     * this is the case and the caller requested a THP
                     * allocation, we do not want to heavily disrupt the
                     * system, so we fail the allocation instead of entering
                     * direct reclaim.
                     */
                    if (compact_result == COMPACT_DEFERRED)
                            goto nopage;

So he is avoiding the cost of reclaim, which you are not, specifically because he is using defrag == "always". __GFP_NORETRY should be included for any thp allocation and it's a regression that it doesn't.

Compaction doesn't fail, it returns COMPACT_SKIPPED and it asks to do a run of reclam to generate those 2M of PAGE_SIZEd memory required to move a 2M piece into the newly freely PAGE_SIZEd fragments. So __GFP_NORETRY never jumps to "nopage" and it never gets deferred either.

For the record, I didn't trace it literally with gdb to validate my theory of why forcefully adding __GFP_NORETRY didn't move the needle, so feel free to do more investigations in that area if you see any pitfall in the theory.

Thanks, Andrea

On Fri, 5 Oct 2018, Mel Gorman wrote:

...

This causes, on average, a 13.9% access latency regression on Haswell, and the regression would likely be more severe on Naples and Rome.

That assumes that fragmentation prevents easy allocation which may very well be the case. While it would be great that compaction or the page allocator could be further improved to deal with fragmentation, it's outside the scope of this patch.

Hi Mel,

The regression that Andrea is working on, correct me if I'm wrong, is heavy reclaim and swapping activity that is trying to desperately allocate local hugepages when the local node is fragmented based on advice provided by MADV_HUGEPAGE.

Why is it ever appropriate to do heavy reclaim and swap activity to allocate a transparent hugepage? This is exactly what the __GFP_NORETRY check for high-order allocations is attempting to avoid, and it explicitly states that it is for thp faults. The fact that we lost __GFP_NORERY for thp allocations for all settings, including the default setting, other than yours (setting of "always") is what I'm focusing on. There is no guarantee that this activity will free an entire pageblock or that it is even worthwhile.

Why is thp memory ever being allocated without __GFP_NORETRY as the page allocator expects?

That aside, removing __GFP_THISNODE can make the fault latency much worse if remote notes are fragmented and/or reclaim has the inability to free contiguous memory, which it likely cannot. This is where I measured over 40% fault latency regression from Linus's tree with this patch on a fragmnented system where order-9 memory is neither available from node 0 or node 1 on Haswell.

...

There exist libraries that allow the .text segment of processes to be remapped to memory backed by transparent hugepages and use MADV_HUGEPAGE to stress local compaction to defragment node local memory for hugepages at startup.

That is taking advantage of a co-incidence of the implementation. MADV_HUGEPAGE is *advice* that huge pages be used, not what the locality is. A hint for strong locality preferences should be separate advice (madvise) or a separate memory policy. Doing that is outside the context of this patch but nothing stops you introducing such a policy or madvise, whichever you think would be best for the libraries to consume (I'm only aware of libhugetlbfs but there might be others).

The behavior that MADV_HUGEPAGE specifies is certainly not clearly defined, unfortunately. The way that an application writer may read it, as we have, is that it will make a stronger attempt at allocating a hugepage at fault. This actually works quite well when the allocation correctly has __GFP_NORETRY, as it's supposed to, and compaction is MIGRATE_ASYNC.

So rather than focusing on what MADV_HUGEPAGE has meant over the past 2+ years of kernels that we have implemented based on, or what it meant prior to that, is a fundamental question of the purpose of direct reclaim and swap activity that had always been precluded before __GFP_NORETRY was removed in a thp allocation. I don't think anybody in this thread wants 14% remote access latency regression if we allocate remotely or 40% fault latency regression when remote nodes are fragmented as well.

Removing __GFP_THISNODE only helps when remote memory is not fragmented, otherwise it multiplies the problem as I've shown.

The numbers that you provide while using the non-default option to mimick MADV_HUGEPAGE mappings but also use __GFP_NORETRY makes the actual source of the problem quite easy to identify: there is an inconsistency in the thp gfp mask and the page allocator implementation.

...

The cost, including the statistics Mel gathered, is acceptable for these processes: they are not concerned with startup cost, they are concerned only with optimal access latency while they are running.

Then such applications at startup have the option of setting zone_reclaim_mode during initialisation assuming a privileged helper can be created. That would be somewhat heavy handed and a longer-term solution would still be to create a proper memory policy of madvise flag for those libraries.

We *never* want to use zone_reclaim_mode for these allocations, that would be even worse, we do not want to reclaim because we have a very unlikely chance of making pageblocks free without the involvement of compaction. We want to trigger memory compaction with a well-bounded cost that MIGRATE_ASYNC provides and then fail.

[Sorry for being slow in responding but I was mostly offline last few days]

On Tue 09-10-18 10:48:25, Mel Gorman wrote: [...]

This goes back to my point that the MADV_HUGEPAGE hint should not make promises about locality and that introducing MADV_LOCAL for specialised libraries may be more appropriate with the initial semantic being how it treats MADV_HUGEPAGE regions.

I agree with your other points and not going to repeat them. I am not sure madvise s the best API for the purpose though. We are talking about memory policy here and there is an existing api for that so I would _prefer_ to reuse it for this purpose.

Sure we will likely need somethin in the compaction as well but we should start simple and go forward in smaller steps.

On Tue 09-10-18 14:00:34, Mel Gorman wrote:

On Tue, Oct 09, 2018 at 02:27:45PM +0200, Michal Hocko wrote:

...

[Sorry for being slow in responding but I was mostly offline last few days]

On Tue 09-10-18 10:48:25, Mel Gorman wrote: [...]

...

This goes back to my point that the MADV_HUGEPAGE hint should not make promises about locality and that introducing MADV_LOCAL for specialised libraries may be more appropriate with the initial semantic being how it treats MADV_HUGEPAGE regions.

I agree with your other points and not going to repeat them. I am not sure madvise s the best API for the purpose though. We are talking about memory policy here and there is an existing api for that so I would _prefer_ to reuse it for this purpose.

I flip-flopped on that one in my head multiple times on the basis of how strict it should be. Memory policies tend to be black or white -- bind here, interleave there, etc. It wasn't clear to me what the best policy would be to describe "allocate local as best as you can but allow fallbacks if necessary".

I was thinking about MPOL_NODE_PROXIMITY with the following semantic: - try hard to allocate from a local or very close numa node(s) even when that requires expensive operations like the memory reclaim/compaction before falling back to other more distant numa nodes.

Hence, I started leaning towards advise as it is really about advice that the kernel can ignore if necessary. That said, I don't feel as strongly about the "how" as I do about the fact that applications and libraries should not depend on side-effects of the MADV_HUGEPAGE implementation that relate to locality.

completely agreed on this.

On Tue, Oct 09, 2018 at 04:25:10PM +0200, Michal Hocko wrote:

On Tue 09-10-18 14:00:34, Mel Gorman wrote:

...

On Tue, Oct 09, 2018 at 02:27:45PM +0200, Michal Hocko wrote:

...

[Sorry for being slow in responding but I was mostly offline last few days]

On Tue 09-10-18 10:48:25, Mel Gorman wrote: [...]

...

This goes back to my point that the MADV_HUGEPAGE hint should not make promises about locality and that introducing MADV_LOCAL for specialised libraries may be more appropriate with the initial semantic being how it treats MADV_HUGEPAGE regions.

I agree with your other points and not going to repeat them. I am not sure madvise s the best API for the purpose though. We are talking about memory policy here and there is an existing api for that so I would _prefer_ to reuse it for this purpose.

I flip-flopped on that one in my head multiple times on the basis of how strict it should be. Memory policies tend to be black or white -- bind here, interleave there, etc. It wasn't clear to me what the best policy would be to describe "allocate local as best as you can but allow fallbacks if necessary".

MPOL_PREFERRED is not black and white. In fact I asked David earlier if MPOL_PREFERRED could check if it would already be a good fit for this. Still the point is it requires privilege (and for a good reason).

I was thinking about MPOL_NODE_PROXIMITY with the following semantic:

• try hard to allocate from a local or very close numa node(s) even when

that requires expensive operations like the memory reclaim/compaction before falling back to other more distant numa nodes.

If MPOL_PREFERRED can't work something like this could be added.

I think "madvise vs mbind" is more an issue of "no-permission vs permission" required. And if the processes ends up swapping out all other process with their memory already allocated in the node, I think some permission is correct to be required, in which case an mbind looks a better fit. MPOL_PREFERRED also looks a first candidate for investigation as it's already not black and white and allows spillover and may already do the right thing in fact if set on top of MADV_HUGEPAGE.

Thanks, Andrea

On Wed, 10 Oct 2018, David Rientjes wrote:

...

I think "madvise vs mbind" is more an issue of "no-permission vs permission" required. And if the processes ends up swapping out all other process with their memory already allocated in the node, I think some permission is correct to be required, in which case an mbind looks a better fit. MPOL_PREFERRED also looks a first candidate for investigation as it's already not black and white and allows spillover and may already do the right thing in fact if set on top of MADV_HUGEPAGE.

We would never want to thrash the local node for hugepages because there is no guarantee that any swapping is useful. On COMPACT_SKIPPED due to low memory, we have very clear evidence that pageblocks are already sufficiently fragmented by unmovable pages such that compaction itself, even with abundant free memory, fails to free an entire pageblock due to the allocator's preference to fragment pageblocks of fallback migratetypes over returning remote free memory.

As I've stated, we do not want to reclaim pointlessly when compaction is unable to access the freed memory or there is no guarantee it can free an entire pageblock. Doing so allows thrashing of the local node, or remote nodes if __GFP_THISNODE is removed, and the hugepage still cannot be allocated. If this proposed mbind() that requires permissions is geared to me as the user, I'm afraid the details of what leads to the thrashing are not well understood because I certainly would never use this.

At the risk of beating a dead horse that has already been beaten, what are the plans for this patch when the merge window opens? It would be rather unfortunate for us to start incurring a 14% increase in access latency and 40% increase in fault latency. Would it be possible to test with my patch[*] that does not try reclaim to address the thrashing issue? If that is satisfactory, I don't have a strong preference if it is done with a hardcoded pageblock_order and __GFP_NORETRY check or a new __GFP_COMPACT_ONLY flag.

I think the second issue of faulting remote thp by removing __GFP_THISNODE needs supporting evidence that shows some platforms benefit from this (and not with numa=fake on the command line :).

[*] https://marc.info/?l=linux-kernel&m=153903127717471

Hello Andrew,

On Mon, Oct 15, 2018 at 03:44:59PM -0700, Andrew Morton wrote:

On Mon, 15 Oct 2018 15:30:17 -0700 (PDT) David Rientjes rientjes@google.com wrote:

...

Would it be possible to test with my patch[*] that does not try reclaim to address the thrashing issue?

Yes please.

It'd also be great if a testcase reproducing the 40% higher access latency (with the one liner original fix) was available.

We don't have a testcase for David's 40% latency increase problem, but that's likely to only happen when the system is somewhat low on memory globally. So the measurement must be done when compaction starts failing globally on all zones, but before the system starts swapping. The more global fragmentation the larger will be the window between "compaction fails because all zones are too fragmented" and "there is still free PAGE_SIZEd memory available to reclaim without swapping it out". If I understood correctly, that is precisely the window where the 40% higher latency should materialize.

The workload that shows the badness in the upstream code is fairly trivial. Mel and Zi reproduced it too and I have two testcases that can reproduce it, one with device assignment and the other is just memhog. That's a massively larger window than the one where the 40% higher latency materializes.

When there's 75% or more of the RAM free (not even allocated as easily reclaimable pagecache) globally, you don't expect to hit heavy swapping.

The 40% THP allocation latency increase if you use MADV_HUGEPAGE in such window where all remote zones are fully fragmented is somehow lesser of a concern in my view (plus there's the compact deferred logic that should mitigate that scenario). Furthermore it is only a concern for page faults in MADV_HUGEPAGE ranges. If MADV_HUGEPAGE is set the userland allocation is long lived, so such higher allocation latency won't risk to hit short lived allocations that don't set MADV_HUGEPAGE (unless madvise=always, but that's not the default precisely because not all allocations are long lived).

If the MADV_HUGEPAGE using library was freely available it'd also be nice.

On Mon, Oct 15, 2018 at 03:44:59PM -0700, Andrew Morton wrote:

On Mon, 15 Oct 2018 15:30:17 -0700 (PDT) David Rientjes rientjes@google.com wrote:

...

At the risk of beating a dead horse that has already been beaten, what are the plans for this patch when the merge window opens?

I'll hold onto it until we've settled on something. Worst case, Andrea's original is easily backportable.

I consider this to be an unfortunate outcome. On the one hand, we have a problem that three people can trivially reproduce with known test cases and a patch shown to resolve the problem. Two of those three people work on distributions that are exposed to a large number of users. On the other, we have a problem that requires the system to be in a specific state and an unknown workload that suffers badly from the remote access penalties with a patch that has review concerns and has not been proven to resolve the trivial cases. In the case of distributions, the first patch addresses concerns with a common workload where on the other hand we have an internal workload of a single company that is affected -- which indirectly affects many users admittedly but only one entity directly.

At the absolute minimum, a test case for the "system fragmentation incurs access penalties for a workload" scenario that could both replicate the fragmentation and demonstrate the problem should have been available before the patch was rejected. With the test case, there would be a chance that others could analyse the problem and prototype some fixes. The test case was requested in the thread and never produced so even if someone were to prototype fixes, it would be dependant on a third party to test and produce data which is a time-consuming loop. Instead, we are more or less in limbo.

Hello,

On Tue, Oct 16, 2018 at 03:37:15PM -0700, Andrew Morton wrote:

we'll still make it into 4.19.1. Am reluctant to merge this while discussion, testing and possibly more development are ongoing.

I think there can be definitely more developments primarily to make the compact deferred logic NUMA aware. Instead of a global deferred logic, we should split it per zone per node so that it backs off exponentially with an higher cap in remote nodes. The current global "backoff" limit will still apply to the "local" zone compaction. Who would like to work on that?

However I don't think it's worth waiting for that, because it's not a trivial change.

Certainly we can't ship upstream in production with this bug, so if it doesn't get fixed upstream we'll fix it downstream first until the more developments are production ready. This was a severe regression compared to previous kernels that made important workloads unusable and it starts when __GFP_THISNODE was added to THP allocations under MADV_HUGEPAGE. It is not a significant risk to go to the previous behavior before __GFP_THISNODE was added, it worked like that for years.

This was simply an optimization to some lucky workloads that can fit in a single node, but it ended up breaking the VM for others that can't possibly fit in a single node, so going back is safe.

Thanks, Andrea

On Tue 16-10-18 16:16:43, Andrew Morton wrote:

On Tue, 16 Oct 2018 19:11:49 -0400 Andrea Arcangeli aarcange@redhat.com wrote:

...

This was a severe regression compared to previous kernels that made important workloads unusable and it starts when __GFP_THISNODE was added to THP allocations under MADV_HUGEPAGE. It is not a significant risk to go to the previous behavior before __GFP_THISNODE was added, it worked like that for years.@s1@s2@s1

5265047ac301 ("mm, thp: really limit transparent hugepage allocation to local node") was April 2015. That's a long time for a "severe regression" to go unnoticed?

Well, it gets some time to adopt changes in enterprise and we start seeing people reporting this issue. That is why I believe we should start with something really simple and stable tree backportable first and then build something more complex on top.

On Tue, 16 Oct 2018, Mel Gorman wrote:

I consider this to be an unfortunate outcome. On the one hand, we have a problem that three people can trivially reproduce with known test cases and a patch shown to resolve the problem. Two of those three people work on distributions that are exposed to a large number of users. On the other, we have a problem that requires the system to be in a specific state and an unknown workload that suffers badly from the remote access penalties with a patch that has review concerns and has not been proven to resolve the trivial cases.

The specific state is that remote memory is fragmented as well, this is not atypical. Removing __GFP_THISNODE to avoid thrashing a zone will only be beneficial when you can allocate remotely instead. When you cannot allocate remotely instead, you've made the problem much worse for something that should be __GFP_NORETRY in the first place (and was for years) and should never thrash.

I'm not interested in patches that require remote nodes to have an abundance of free or unfragmented memory to avoid regressing.

In the case of distributions, the first patch addresses concerns with a common workload where on the other hand we have an internal workload of a single company that is affected -- which indirectly affects many users admittedly but only one entity directly.

The alternative, which is my patch, hasn't been tested or shown why it cannot work. We continue to talk about order >= pageblock_order vs __GFP_COMPACTONLY.

I'd like to know, specifically:

- what measurable affect my patch has that is better solved with removing __GFP_THISNODE on systems where remote memory is also fragmented?

- what platforms benefit from remote access to hugepages vs accessing local small pages (I've asked this maybe 4 or 5 times now)?

- how is reclaiming (and possibly thrashing) memory helpful if compaction fails to free an entire pageblock due to slab fragmentation due to low on memory conditions and the page allocator preference to return node- local memory?

- how is reclaiming (and possibly thrashing) memory helpful if compaction cannot access the memory reclaimed because the freeing scanner has already passed by it, or the migration scanner has passed by it, since this reclaim is not targeted to pages it can find?

- what metrics can be introduced to the page allocator so that we can determine that reclaiming (and possibly thrashing) memory will result in a hugepage being allocated?

Until we have answers, especially for the last, there is no reason why thp allocations should not be __GFP_NORETRY including for MADV_HUGEPAGE regions. The implementation of memory compaction simply cannot guarantee that the cost is worthwhile.

On Mon, 22 Oct 2018, Zi Yan wrote:

Hi David,

Hi!

On 22 Oct 2018, at 17:04, David Rientjes wrote:

...

On Tue, 16 Oct 2018, Mel Gorman wrote:

...

I consider this to be an unfortunate outcome. On the one hand, we have a problem that three people can trivially reproduce with known test cases and a patch shown to resolve the problem. Two of those three people work on distributions that are exposed to a large number of users. On the other, we have a problem that requires the system to be in a specific state and an unknown workload that suffers badly from the remote access penalties with a patch that has review concerns and has not been proven to resolve the trivial cases.

The specific state is that remote memory is fragmented as well, this is not atypical. Removing __GFP_THISNODE to avoid thrashing a zone will only be beneficial when you can allocate remotely instead. When you cannot allocate remotely instead, you've made the problem much worse for something that should be __GFP_NORETRY in the first place (and was for years) and should never thrash.

I'm not interested in patches that require remote nodes to have an abundance of free or unfragmented memory to avoid regressing.

I just wonder what is the page allocation priority list in your environment, assuming all memory nodes are so fragmented that no huge pages can be obtained without compaction or reclaim.

Here is my version of that list, please let me know if it makes sense to you:

1. local huge pages: with compaction and/or page reclaim, you are willing

to pay the penalty of getting huge pages;

2. local base pages: since, in your system, remote data accesses have much

higher penalty than the extra TLB misses incurred by the base page size;

3. remote huge pages: at least it is better than remote base pages;

4. remote base pages: it performs worst in terms of locality and TLBs.

I have a ton of different platforms available. Consider a very basic access latency evaluation on Broadwell on a running production system: remote hugepage vs remote PAGE_SIZE pages had about 5% better access latency. Remote PAGE_SIZE pages vs local pages is a 12% degradation. On Naples, remote hugepage vs remote PAGE_SIZE had 2% better access latency intrasocket, no better access latency intersocket. Remote PAGE_SIZE pages vs local is a 16% degradation intrasocket and 38% degradation intersocket.

My list removes (3) from your list, but is otherwise unchanged. I remove (3) because 2-5% better access latency is nice, but we'd much rather fault local base pages and then let khugepaged collapse it into a local hugepage when fragmentation is improved or we have freed memory. That is where we can get a much better result, 41% better access latency on Broadwell and 52% better access latncy on Naples. I wouldn't trade that for 2-5% immediate remote hugepages.

It just so happens that prior to this patch, the implementation of the page allocator matches this preference.

In addition, to prioritize local base pages over remote pages, the original huge page allocation has to fail, then kernel can fall back to base page allocations. And you will never get remote huge pages any more if the local base page allocation fails, because there is no way back to huge page allocation after the fallback.

That is exactly what we want, we want khugepaged to collapse memory into local hugepages for the big improvement rather than persistently access a hugepage remotely; the win of the remote hugepage just isn't substantial enough, and the win of the local hugepage is just too great.

...

I'd like to know, specifically:

• what measurable affect my patch has that is better solved with removing __GFP_THISNODE on systems where remote memory is also fragmented?

• what platforms benefit from remote access to hugepages vs accessing local small pages (I've asked this maybe 4 or 5 times now)?

• how is reclaiming (and possibly thrashing) memory helpful if compaction fails to free an entire pageblock due to slab fragmentation due to low on memory conditions and the page allocator preference to return node- local memory?

• how is reclaiming (and possibly thrashing) memory helpful if compaction cannot access the memory reclaimed because the freeing scanner has already passed by it, or the migration scanner has passed by it, since this reclaim is not targeted to pages it can find?

• what metrics can be introduced to the page allocator so that we can determine that reclaiming (and possibly thrashing) memory will result in a hugepage being allocated?

The slab fragmentation and whether reclaim/compaction can help form huge pages seem to orthogonal to this patch, which tries to decide the priority between locality and huge pages.

It's not orthogonal to the problem being reported which requires local memory pressure. If there is no memory pressure, compaction often can succeed without reclaim because the freeing scanner can find target memory and the migration scanner can make a pageblock free. Under memory pressure, however, where Andrea is experiencing the thrashing of the local node, by this time it can be inferred that slab pages have already fallen bcak to MIGRATE_MOVABLE pageblocks. There is nothing preventing it under memory pressure because of the preference to return local memory over fragmenting pageblocks.

So the point of slab fragmentation, which typically exists locally when there is memory pressure, is that we cannot ascertain whether memory compaction even with reclaim will be successful. Not only because the freeing scanner cannot access reclaimed memory, but also because we have no feedback from compaction to determine whether the work will be useful. Thrashing the local node, migrating COMPACT_CLUSTER_MAX pages, finding one slab page sitting in the pageblock, and continuing is not a good use of the allocator's time. This is true of both MADV_HUGEPAGE and non-MADV_HUGEPAGE regions.

For reclaim to be considered, we should ensure that work is useful to compaction. That ability is non-existant. The worst case scenario is you thrash the local node and still cannot allocate a hugepage.

On Tue, Oct 23, 2018 at 08:57:45AM +0100, Mel Gorman wrote:

Note that I accept it's trivial to fragment memory in a harmful way. I've prototyped a test case yesterday that uses fio in the following way to fragment memory

o fio of many small files (64K) o create initial pages using writes that disable fallocate and create inodes on first open. This is massively inefficient from an IO perspective but it mixes slab and page cache allocations so all NUMA nodes get fragmented. o Size the page cache so that it's 150% the size of memory so it forces reclaim activity and new fio activity to further mix slab and page cache allocations o After initial write, run parallel readers to keep slab active and run this for the same length of time the initial writes took so fio has called stat() on the existing files and begun the read phase. This forces the slab and page cache pages to remain "live" and difficult to reclaim/compact. o Finally, start a workload that allocates THP after the warmup phase but while fio is still runnning to measure allocation success rate and latencies

The tests completed shortly after I wrote this mail so I can put some figures to the intuitions expressed in this mail. I'm truncating the reports for clarity but can upload the full data if necessary.

The target system is a 2-socket using E5-2670 v3 (Haswell). Base kernel is 4.19. The baseline is an unpatched kernel. relaxthisnode-v1r1 is patch 1 of Michal's series and does not include the second cleanup. noretry-v1r1 is David's alternative

global-dhp__workload_usemem-stress-numa-compact (no filesystem as this is the trivial case of allocating anonymous memory on a freshly booted system. Figures are elapsed time)

4.19.0 4.19.0 4.19.0 vanilla relaxthisnode-v1r1 noretry-v1r1 Amean System-1 14.16 ( 0.00%) 12.35 * 12.75%* 15.96 * -12.70%* Amean System-3 15.14 ( 0.00%) 9.83 * 35.08%* 11.00 * 27.34%* Amean System-4 9.88 ( 0.00%) 9.85 ( 0.25%) 9.80 ( 0.75%) Amean Elapsd-1 29.23 ( 0.00%) 26.16 * 10.50%* 33.81 * -15.70%* Amean Elapsd-3 25.67 ( 0.00%) 7.28 * 71.63%* 8.49 * 66.93%* Amean Elapsd-4 5.49 ( 0.00%) 5.53 ( -0.76%) 5.46 ( 0.49%)

The figures in () are the percentage gain/loss. If it's around *'s then the automation has guessed at the results are outside the noise.

System CPU usage is reduced by both as reported but Micha's gives a 10.5% gain and David's is a 15.7% loss. Boith appear to be outside the noise. While not included here, the vanilla kernel swaps heavily with a 56% reclaim efficiency (pages scanned vs pages reclaimed) and neither of the proposed patches swaps and it's all from direct reclaim activity. Michal's patch does not enter reclaim, David's enters reclaim but it's very light.

global-dhp__workload_thpfioscale-xfs (Uses fio to fragment memory and keep slab and page cache active while there is an attempt to allocate THP in parallel. No special madvise flags or tuning is applied. A dedicated test partition is used for fio and XFS was the target filesystem that is recreated on every test) thpfioscale Fault Latencies 4.19.0 4.19.0 4.19.0 vanilla relaxthisnode-v1r1 noretry-v1r1 Amean fault-base-5 1471.95 ( 0.00%) 1515.64 ( -2.97%) 1491.05 ( -1.30%) Amean fault-huge-5 0.00 ( 0.00%) 534.51 * -99.00%* 0.00 ( 0.00%)

thpfioscale Percentage Faults Huge 4.19.0 4.19.0 4.19.0 vanilla relaxthisnode-v1r1 noretry-v1r1 Percentage huge-5 0.00 ( 0.00%) 1.18 ( 100.00%) 0.00 ( 0.00%)

Both patches incur a slight hit to fault latency (measured in microseconds) but it's well within the noise. While not included here, the variance is massive (min 1052 microseconds, max 282348 microseconds in the vanilla kernel. Both patches reduce the worst-case scenarios. All kernels show terrible allocation success rates. Michal's had a 1.18% success rate but that's probably luck.

global-dhp__workload_thpfioscale-madvhugepage-xfs (Same as the last test but the THP allocation program uses MADV_HUGEPAGE)

thpfioscale Fault Latencies 4.19.0 4.19.0 4.19.0 vanilla relaxthisnode-v1r1 noretry-v1r1 Amean fault-base-5 6772.84 ( 0.00%) 10256.30 * -51.43%* 1574.45 * 76.75%* Amean fault-huge-5 2644.19 ( 0.00%) 5314.17 *-100.98%* 3517.89 ( -33.04%)

thpfioscale Percentage Faults Huge 4.19.0 4.19.0 4.19.0 vanilla relaxthisnode-v1r1 noretry-v1r1 Percentage huge-5 45.48 ( 0.00%) 95.09 ( 109.08%) 2.81 ( -93.81%

The first point of interest is that even with the vanilla kernel, the allocation fault latency is much higher than average reflecting that additional work is being done.

Next point of interest -- David's patch has much lower latency on average when allocating *base* pages showing and the vmstats (not included) show that compaction activity is reduced but not eliminated.

To balance this, Michal's patch has an 95% allocation success rate for THP versus 45% on the default kernel at the cost of higher fault latency. This is almost certainly a reflection that THPs are being allocated on remote nodes. This can be considered good or bad depending on whether THP is more important than locality. Note with David's patch that the allocation success rate drops to 2.81% showing that it's much less efficient at THP.

This demonstrates a very clear trade-off between allocation latency and allocation success rate for THP. Which one is better is workload dependent.

global-dhp__workload_thpfioscale-defrag-xfs (Same as global-dhp__workload_thpfioscale-xfs except that defrag is set to always) thpfioscale Fault Latencies 4.19.0 4.19.0 4.19.0 vanilla relaxthisnode-v1r1 noretry-v1r1 Amean fault-base-5 2678.60 ( 0.00%) 4442.14 * -65.84%* 1640.15 * 38.77%* Amean fault-huge-5 1324.61 ( 0.00%) 1460.08 ( -10.23%) 2358.23 ( -78.03%)

thpfioscale Percentage Faults Huge 4.19.0 4.19.0 4.19.0 vanilla relaxthisnode-v1r1 noretry-v1r1 Percentage huge-5 0.90 ( 0.00%) 0.40 ( -55.56%) 0.22 ( -75.93%)

The allocation latency is again higher in this case as greater effort is made to allocate the huge page. Michal's takes a hit as it's still trying to allocate the THP while David's gives up early. In all cases the allocation success rate is terrible.

So it should be reasonably clear that no approach is a universal win. Michal's wins at the trivial case which is what the original problem was and why it was pushed at all. David's in general has lower latency in general because it gives up quickly but the allocation success rate when MADV_HUGEPAGE specifically asks for huge pages is terrible. This may make it a non-starter for the virtualisation case that wants huge pages on the basis that if an application asks for huge pages, it presumably is willing to pay the cost to get them.

On Mon, 15 Oct 2018, Andrea Arcangeli wrote:

...

At the risk of beating a dead horse that has already been beaten, what are the plans for this patch when the merge window opens? It would be rather unfortunate for us to start incurring a 14% increase in access latency and 40% increase in fault latency. Would it be possible to test with my patch[*] that does not try reclaim to address the thrashing issue? If that is satisfactory, I don't have a strong preference if it is done with a hardcoded pageblock_order and __GFP_NORETRY check or a new __GFP_COMPACT_ONLY flag.

I don't like the pageblock size hardcoding inside the page allocator. __GFP_COMPACT_ONLY is fully runtime equivalent, but it at least let the caller choose the behavior, so it looks more flexible.

I'm not sure that I understand why the user would ever want to thrash their zone(s) for allocations of this order. The problem here is specifically related to an entire pageblock becoming freeable and the unlikeliness that reclaiming/swapping/thrashing will assist memory compaction in making that happen. For this reason, I think the order >= pageblock_order check is reasonable because it depends on the implementation of memory compaction.

Why do we need another gfp flag for thp allocations when they are made to be __GFP_NORETRY by default and it is very unlikely that reclaiming once and then retrying compaction is going to make an entire pageblock free?

I'd like to know (1) how continuous reclaim activity can make entire pageblocks freeable without thrashing and (2) the metrics that we can use to determine when it is worthwhile vs harmful. I don't believe (1) is ever helpful based on the implementation of memory compaction and we lack (2) since reclaim is not targeted to memory that compaction can use.

As long as compaction returns COMPACT_SKIPPED it's ok to keep doing reclaim and keep doing compaction, as long as compaction succeeds.

Compaction will operate on 32 pages at a time and declare success each time and then pick up where it left off the next time it is called in the hope that it "succeeds" 512/32=16 times in a row while constantly reclaiming memory. Even a single slab page in that pageblock will make all of this work useless. Reclaimed memory not accessible by the freeing scanner will make its work useless. We lack the ability to determine when compaction is successful in freeing a full pageblock.

On Tue, Oct 09, 2018 at 03:17:30PM -0700, David Rientjes wrote:

causes workloads to severely regress both in fault and access latency when we know that direct reclaim is unlikely to make direct compaction free an entire pageblock. It's more likely than not that the reclaim was pointless and the allocation will still fail.

How do you know that? If all RAM is full of filesystem cache, but it's not heavily fragmented by slab or other unmovable objects, compaction will succeed every single time after reclaim frees 2M of cache like it's asked to do.

reclaim succeeds every time, compaction then succeeds every time.

Not doing reclaim after COMPACT_SKIPPED is returned simply makes compaction unable to compact memory once all nodes are filled by filesystem cache.

Certainly it's better not to invoke reclaim at all if __GFP_THISNODE is set, than swapping out heavy over the local node. Doing so however has the drawback of reducing the direct compaction effectiveness. I don't think it's true that reclaim is generally "pointless", it's just that invoking any reclaim backfired so bad if __GFP_THISNODE was set, than anything else (including weakining compaction effectiveness) was better.

If memory compaction were patched such that it can report that it could successfully free a page of the specified order if there were free pages at the end of the zone it could migrate to, reclaim might be helpful. But with the current implementation, I don't think that is reliably possible. These free pages could easily be skipped over by the migration scanner because of the presence of slab pages, for example, and unavailable to the freeing scanner.

Yes there's one case where reclaim is "pointless", but it happens once and then COMPACT_DEFERRED is returned and __GFP_NORETRY will skip reclaim then.

So you're right when we hit fragmentation there's one and only one "pointless" reclaim invocation. And immediately after we also exponentially backoff on the compaction invocations with the compaction deferred logic.

We could try optimize away such "pointless" reclaim event for sure, but it's probably an optimization that may just get lost in the noise and may not be measurable, because it only happens once when the first full fragmentation is encountered.

I really think that for this patch to be merged over my proposed change that it needs to be clearly demonstrated that reclaim was successful in that it freed memory that was subsequently available to the compaction freeing scanner and that enabled entire pageblocks to become free. That, in my experience, will very very seldom be successful because of internal slab fragmentation, compaction_alloc() cannot soak up pages from the reclaimed memory, and potentially thrash the zone completely pointlessly. The last point is the problem being reported here, but the other two are as legitimate.

I think the demonstration can already be inferred, because if hit full memory fragmentation after every reclaim, __GFP_NORETRY would have solved the "pathological THP allocation behavior" without requiring your change to __GFP_NORETRY that makes it behave like __GFP_COMPACT_ONLY for order == HPAGE_PMD_ORDER.

Anyway you can add a few statistic counters and verify in more accurate way how often a COMPACT_SKIPPED + reclaim cycle is followed by a COMPACT_DEFERRED.

I'd appreciate if Andrea can test this patch, have a rebuttal that we should still remove __GFP_THISNODE because we don't care about locality as much as forming a hugepage, we can make that change, and then merge this instead of causing such massive fault and access latencies.

I can certainly test, but from source review I'm already convinced it'll solve fine the "pathological THP allocation behavior", no argument about that. It's certainly better and more correct your patch than the current upstream (no security issues with lack of permissions for __GFP_THISNODE anymore either).

I expect your patch will run 100% equivalent to __GFP_COMPACT_ONLY alternative I posted, for our testcase that hit into the "pathological THP allocation behavior".

Your patch encodes __GFP_COMPACT_ONLY into the __GFP_NORETRY semantics and hardcodes the __GFP_COMPACT_ONLY for all orders = HPAGE_PMD_SIZE no matter which is the caller.

As opposed I let the caller choose and left __GFP_NORETRY semantics alone and orthogonal to the __GFP_COMPACT_ONLY semantics. I think letting the caller decide instead of hardcoding it for order 9 is better, because __GFP_COMPACT_ONLY made sense to be set only if __GFP_THISNODE was also set by the caller.

If a driver does an order 9 allocation with __GFP_THISNODE not set, your patch will prevent it to allocate remote THP if all remote nodes are full of cache (which is a reasonable common assumption as more THP are allocated over time eating in all free memory). My patch didn't alter that so I tend to prefer the __GFP_COMPACT_ONLY than the hardcoding __GFP_COMPACT_ONLY for all order 9 allocations regardless if __GFP_THISNODE is set or not.

Last but not the last, from another point of view, I thought calling remote compaction was a feature especially for MADV_HUGEPAGE. However if avoiding the 40% increase latency for MADV_HUGEPAGE was the primary motivation for __GFP_THISNODE, not sure how we can say that such an high allocation latency is only a concern for order 9 allocations and all other costly_order allocations (potentially with orders even higher than HPAGE_PMD_ORDER in fact) are ok to keep calling remote compaction and incur in the 40% higher latency.

Thanks, Andrea

On Tue, 9 Oct 2018, Andrea Arcangeli wrote:

On Tue, Oct 09, 2018 at 03:17:30PM -0700, David Rientjes wrote:

...

causes workloads to severely regress both in fault and access latency when we know that direct reclaim is unlikely to make direct compaction free an entire pageblock. It's more likely than not that the reclaim was pointless and the allocation will still fail.

How do you know that? If all RAM is full of filesystem cache, but it's not heavily fragmented by slab or other unmovable objects, compaction will succeed every single time after reclaim frees 2M of cache like it's asked to do.

reclaim succeeds every time, compaction then succeeds every time.

Not doing reclaim after COMPACT_SKIPPED is returned simply makes compaction unable to compact memory once all nodes are filled by filesystem cache.

For reclaim to assist memory compaction based on compaction's current implementation, it would require that the freeing scanner starting at the end of the zone can find these reclaimed pages as migration targets and that compaction will be able to utilize these migration targets to make an entire pageblock free.

In such low on memory conditions when a node is fully saturated, it is much less likely that memory compaction can free an entire pageblock even if the freeing scanner can find these now-free pages. More likely is that we have unmovable pages in MIGRATE_MOVABLE pageblocks because the allocator allows fallback to pageblocks of other migratetypes to return node local memory (because affinity matters for kernel memory as it matters for thp) rather than fallback to remote memory.

This has caused us significant pain where we have 1.5GB of slab, for example, spread over 100GB of pageblocks once the node has become saturated.

So reclaim is not always "pointless" as you point out, but it should at least only be attempted if memory compaction could free an entire pageblock if it had free memory to migrate to. It's much harder to make sure that these freed pages can be utilized by the freeing scanner. Based on how memory compaction is implemented, I do not think any guarantee can be made that reclaim will ever be successful in allowing it to make order-9 memory available, unfortunately.

...

If memory compaction were patched such that it can report that it could successfully free a page of the specified order if there were free pages at the end of the zone it could migrate to, reclaim might be helpful. But with the current implementation, I don't think that is reliably possible. These free pages could easily be skipped over by the migration scanner because of the presence of slab pages, for example, and unavailable to the freeing scanner.

Yes there's one case where reclaim is "pointless", but it happens once and then COMPACT_DEFERRED is returned and __GFP_NORETRY will skip reclaim then.

So you're right when we hit fragmentation there's one and only one "pointless" reclaim invocation. And immediately after we also exponentially backoff on the compaction invocations with the compaction deferred logic.

This assumes that every time we get COMPACT_SKIPPED that if we can simply free memory that it'll succeed and that's definitely not the case based on the implementation of memory compaction: compaction_alloc() needs to find the memory and the migration scanner needs to free an entire pageblock. The migration scanner doesn't even look ahead to see if that's possible before starting to migrate pages, it's limited to COMPACT_CLUSTER_MAX.

The scenario we have: compaction returns COMPACT_SKIPPED; reclaim expensively tries to reclaim memory by thrashing the local node; the compaction migration scanner has already passed over the now-freed pages so it's inaccessible; if accessible, the migration scanner migrates memory to the newly freed pages but fails to make a pageblock free; loop.

My contention is that the second step is only justified if we can guarantee the freed memory can be useful for compaction and that it can free an entire pageblock for the hugepage if it can migrate. Both of those are not possible to determine based on the current implementation.

...

I'd appreciate if Andrea can test this patch, have a rebuttal that we should still remove __GFP_THISNODE because we don't care about locality as much as forming a hugepage, we can make that change, and then merge this instead of causing such massive fault and access latencies.

I can certainly test, but from source review I'm already convinced it'll solve fine the "pathological THP allocation behavior", no argument about that. It's certainly better and more correct your patch than the current upstream (no security issues with lack of permissions for __GFP_THISNODE anymore either).

I expect your patch will run 100% equivalent to __GFP_COMPACT_ONLY alternative I posted, for our testcase that hit into the "pathological THP allocation behavior".

Your patch encodes __GFP_COMPACT_ONLY into the __GFP_NORETRY semantics and hardcodes the __GFP_COMPACT_ONLY for all orders = HPAGE_PMD_SIZE no matter which is the caller.

As opposed I let the caller choose and left __GFP_NORETRY semantics alone and orthogonal to the __GFP_COMPACT_ONLY semantics. I think letting the caller decide instead of hardcoding it for order 9 is better, because __GFP_COMPACT_ONLY made sense to be set only if __GFP_THISNODE was also set by the caller.

I've hardcoded it directly for pageblock_order because compaction works over pageblocks and we lack the two crucial points of information I've stated that determines whether direct reclaim could possibly be useful. (It's more correctly implemented as order >= pageblock_order as opposed to order == pageblock_order.)

If a driver does an order 9 allocation with __GFP_THISNODE not set, your patch will prevent it to allocate remote THP if all remote nodes are full of cache (which is a reasonable common assumption as more THP are allocated over time eating in all free memory).

Iff it's using __GFP_NORETRY, yes, the allocation and remote access latency that is incurred is the same as for thp. Hugetlbfs doesn't use __GFP_NORETRY, it uses __GFP_RETRY_MAYFAIL, so it will attempt to reclaim memory after my patch.

My patch is assuming that a single call to reclaim followed up by another attempt at compaction is not beneficial for pageblock_order sized allocations with __GFP_NORETRY because it's unlikely to either free an entire pageblock and compaction may not be able to access this memory. It's based on how memory compaction is implemented rather than any special heuristic.

I won't argue if you protect this logic with __GFP_COMPACT_ONLY, but I think thp allocations should always have __GFP_NORETRY based on compaction's implementation.

I see removing __GFP_THISNODE as a separate discussion: if, after my patch (perhaps with a modification for __GFP_COMPACT_ONLY on top of it), you still get unacceptable fault latency and can show that remote access latency to remotely allocated hugepage is better on some platform that isn't Haswell, Naples, and Rome, we can address that but it will probably require more work that simply unsetting __GFP_THISNODE because it will depend on the latency to certain remote nodes over others.

On Mon, Oct 08, 2018 at 01:41:09PM -0700, David Rientjes wrote:

The page allocator is expecting __GFP_NORETRY for thp allocations per its comment:

/*
 * Checks for costly allocations with __GFP_NORETRY, which
 * includes THP page fault allocations
 */
if (costly_order && (gfp_mask & __GFP_NORETRY)) {

And that enables us to check compact_result to determine whether thp allocation should fail or continue to reclaim. I don't think it helps that some thp fault allocations use __GFP_NORETRY and others do not. I think that deserves a fix to alloc_hugepage_direct_gfpmask() or GFP_TRANSHUGE_LIGHT.

With your patch that changes how __GFP_NORETRY works for order = pageblock_order then this can help. However it solves nothing for large skbs and all other "costly_order" allocations. So if you think it's so bad to have a 40% increased allocation latency if all nodes are heavily fragmented for THP under MADV_HUGEPAGE (which are guaranteed to be long lived allocations or they wouldn't set MADV_HUGEPAGE in the first place), I'm not sure why you ignore the same exact overhead for all other "costly_order" allocations that would never set __GFP_THISNODE and will not even use GFP_TRANSHUGE_LIGHT so they may not set __GFP_NORETRY at all.

I think it would be better to view the problem from a generic "costly_order" allocation prospective without so much "hardcoded" focus on THP MADV_HUGEPAGE only (which in fact are the least concern of all in terms of that 40% latency increase if all nodes are totally fragmented and compaction fails on all nodes, because they're long lived so the impact of the increased latency is more likely to be lost in the noise).

Our library that uses MADV_HUGEPAGE only invokes direct compaction because we're on an older kernel: it does not attempt to do reclaim to make compaction happy so that it finds memory that it can migrate memory to. For reference, we use defrag setting of "defer+madvise". Everybody who does not use MADV_HUGEPAGE kicks off kswapd/kcompactd and fails, MADV_HUGEPAGE users do the same but also try direct compaction. That direct compaction uses a mode of MIGRATE_ASYNC so it normally fails because of need_resched() or spinlock contention. These allocations always fail, MADV_HUGEPAGE or otherwise, without invoking direct reclaim.

Even older kernels (before "defer+madvise" option was available) always invoked reclaim if COMPACT_SKIPPED was returned. The COMPACT_SKIPPED behavior I referred that explains why __GFP_NORETRY doesn't prevent reclaim to be invoked, is nothing new, it always worked that way from the day compaction was introduced.

So it's fairly strange that your kernel doesn't call reclaim at all if COMPACT_SKIPPED is returned.

I am agreeing with both you and Mel that it makes no sense to thrash the local node to make compaction happy and then hugepage-order memory available. I'm only differing with you on the mechanism to fail early: we never want to do attempt reclaim on thp allocations specifically because it leads to the behavior you are addressing.

Not sure I can agree with the above.

If all nodes (not only the local one) are already below the watermarks and compaction returns COMPACT_SKIPPED, there is zero global free memory available, we would need to swap anyway to succeed the 2M allocation. So it's better to reclaim 2M from on node and then retry compaction again on the same node if compaction is failing on the node because of COMPACT_SKIPPED and the global free memory is zero. If it swapouts it would have swapped out anyway but this way THP will be returned.

It's just __GFP_THISNODE that broke the above logic. __GFP_THISNODE may just be safe to use only if the global amount of free memory (in all nodes) is zero (or below HPAGE_PMD_SIZE generally speaking).

My contention is that removing __GFP_THISNODE papers over the problem, especially in cases where remote memory is also fragmnented. It incurs a much higher (40%) fault latency and then incurs 13.9% greater access latency. It is not a good result, at least for Haswell, Naples, and Rome.

To make a case that we should fault hugepages remotely as fallback, either for non-MADV_HUGEPAGE users who do not use direct compaction, or MADV_HUGEPAGE users who use direct compaction, we need numbers that suggest there is a performance benefit in terms of access latency to suggest that it is better; this is especially the case when the fault latency is 40% higher. On Haswell, Naples, and Rome, it is quite obvious that this patch works much harder to fault memory remotely that incurs a substantial performance penalty when it fails and in the cases where it succeeds we have much worse access latency.

What we're fixing is a effectively a breakage and insecure behavior too and that must be fixed first. A heavily multithreaded processes with its memory not fitting in a single NUMA node is too common of a workload to misbehave so bad as it did. The above issue is a much smaller concern and it's not a breakage nor insecure.

__GFP_THISNODE semantics are not enough to express what you need there.

Like Mel said we need two tests, one that represents the "pathological THP allocation behavior" which is trivial (any heavy multithreaded app accessing all memory of all nodes from all CPUs will do). And one that represents your workload.

By thinking about it, it's almost impossible that eliminating 100% of swapouts from the multithreaded app runtime, will not bring a benefit that is orders of magnitude bigger than the elimination of remote compaction and having more local THP pages for your app.

It's just on a different scale and the old behavior was insecure to begin with because it allowed a process to heavily penalize the runtime of other apps running with different uid, without requiring any privilege.

For users who bind their applications to a subset of cores on a NUMA node, fallback is egregious: we are guaranteed to never have local access latency and in the case when the local node is fragmented and remote memory is not our fault latency goes through the roof when local native pages would have been much better.

I've brought numbers on how poor this patch performs, so I'm asking for a rebuttal that suggests it is better on some platforms. (1) On what platforms is it better to fault remote hugepages over local native pages? (2) What is the fault latency increase when remote nodes are fragmented as well?

That I don't think is the primary issue, the issue here is the one of the previous paragrah.

Then there may be workloads and/or platforms where allocating remote THP instead of local PAGE_SIZEd memory is better regardless and __GFP_THISNODE hurted twice (but we could ignore those for now, and just focus where __GFP_THISNODE provided a benefit like for your app using the lib).

...

Like Mel said, your app just happens to fit in a local node, if the user of the lib is slightly different and allocates 16G on a system where each node is 4G, the post-fix MADV_HUGEPAGE will perform extremely better also for the lib user.

It won't if the remote memory is fragmented; the patch is premised on the

How could compaction run slower than heavy swapping gigabytes of memory?

argument that remote memory is never fragmented or under memory pressure otherwise it is multiplying the fault latency by the number of nodes. Sure, creating test cases where the local node is under heavy memory pressure yet remote nodes are mostly free will minimize the impact this has on fault latency. It still is a substantial increase, as I've measured on Haswell, but the access latency forever is the penalty. This patch cannot make access to remote memory faster.

I still doubt you measured the performance of the app using the lib when the app requires more 4 times the size of the local NUMA node under "taskset -c 0". If you do that, the app using the lib shall fall into the "pathological THP allocation behavior" too. As long as the spillover is 10-20% perhaps it's not as bad and if it's not heavily multithreaded doing scattered random access like a virtual machine will do, NUMA balancing will hide the issue by moving the thread to CPU of the NUMA node where the 10-20% spillover happened (which is why I asked "taskset -c 0" to simulate a multithreaded app accessing all memory in a scattered way without locality.

The multithreaded app with scattered access is not unreasonable workload and it must perform well: all workloads using MPOL_INTERLEAVE would fit the criteria, not just qemu.

...

And you know, if the lib user fits in one node, it can use mbind and it won't hit OOM... and you'd need some capability giving the app privilege anyway to keep MADV_HUGEPAGE as deep and unfair to the rest of the processes running the local node (like mbind and mlock require too).

No, the lib user does not fit into a single node, we have cases where these nodes are under constant memory pressure. We are on an older kernel, however, that fails because of need_resched() and spinlock contention rather than falling back to local reclaim.

It's unclear which need_resched() and spinlock contention induces this failure.

...

Did you try the __GFP_COMPACT_ONLY patch? That won't have the 40% fault latency already.

I remember Kirill saying that he preferred node local memory allocation over thp allocation in the review, which I agree with, but it was much better than this patch. I see no reason why we should work so hard to reclaim memory, thrash local memory, and swap so that the compaction freeing scanner can find memory when there is *no* guarantee that the migration scanner can free an entire pageblock. That is completely pointless work, even for an MADV_HUGEPAGE user, and we certainly don't have such pathological behavior on older kernels.

The compaction scanner is succeeding at migrating the entire pageblock every time. The local node isn't so fragmented, it's just full of anonymous memory that is getting swapped out at every further invocation of reclaim. If it was fragmented __GFP_NORETRY would have had an effect because compaction would have been deferred.

Completely agreed, and I'm very strongly suggesting that we do not invoke reclaim. Memory compaction provides no guarantee that it can free an entire pageblock, even for MIGRATE_ASYNC compaction under MADV_HUGEPAGE it will attempt to migrate SWAP_CLUSTER_MAX pages from a pageblock without considering if the entire pageblock could eventually become freed for order-9 memory. The vast majority of the time this reclaim could be completely pointless. We cannot use it.

If we don't invoke reclaim __GFP_THISNODE is fine to keep, however that means direction compaction doesn't work at all if all nodes are full of filesystem cache. This was a drawback of __GFP_COMPACT_ONLY too. A minor one compared to the corner case created by __GFP_THISNODE.

We don't need __GFP_NORETRY anymore with this, it wouldn't be useful to continually compact memory in isolation if it already has failed. It seems strange to be using __GFP_DIRECT_RECLAIM | __GFP_ONLY_COMPACT, though, as I assume this would evolve into.

Are there any other proposed users for __GFP_ONLY_COMPACT beyond thp allocations? If not, we should just save the gfp bit and encode the logic directly into the page allocator.

Yes that's seems equivalent, just it embeds __GFP_COMPACT_ONLY in __GFP_NORETRY and ends up setting __GFP_NORETRY for everything, __GFP_COMPACT_ONLY didn't alter the __GFP_NORETRY semantics instead.

Would you support this?

I didn't consider it a problem the 40% increased allocation latency, I thought __GFP_COMPACT_ONLY or the other fix in -mm was just about the remote THP vs local PAGE_SIZEd memory tradeoff (considering this logic was already hardcoded only for THP so I tried to keep it).

Now that you raise this higher compaction latency as a big regression in not fixing the VM with __GFP_COMPACT_ONLY semantics in __GFP_NORETRY, the next question is why you don't care at all about such increased latency for all other "costly_order" allocations that won't use __GFP_THISNODE and where your altered __GFP_NORETRY won't help to skip compaction on the remote nodes.

In other words if such compaction invocation in fully fragmented remote nodes is such a big concern for THP, why is not an issue at all for all other "costly_order" allocations?

• 	if (order == pageblock_order &&
  

• 			!(current->flags & PF_KTHREAD))
  

• 		goto nopage;
  

For a more reliable THP hardcoding HPAGE_PMD_ORDER would be better than pageblock_order (I guess especially for non-x86 archs where the match between hugetlbfs size and thp size is less obvious), but then the hardcoding itself now looks the worst part of this patch if the concern is the 40% increased allocation latency (and not only the remote compound page vs local 4k page).

Thanks, Andrea

On Mon 29-10-18 16:17:52, Balbir Singh wrote: [...]

I wonder if alloc_pool_huge_page() should also trim out it's logic of __GFP_THISNODE for the same reasons as mentioned here. I like that we round robin to alloc the pool pages, but __GFP_THISNODE might be an overkill for that case as well.

alloc_pool_huge_page uses __GFP_THISNODE for a different reason than THP. We really do want to allocated for a per-node pool. THP can fallback or use a different node.

These hugetlb allocations might be disruptive and that is an expected behavior because this is an explicit requirement from an admin to pre-allocate large pages for the future use. __GFP_RETRY_MAYFAIL just underlines that requirement.

Maybe the compaction logic could be improved and that might be a shared goal with future changes though.

On Mon 29-10-18 20:42:53, Balbir Singh wrote:

On Mon, Oct 29, 2018 at 10:00:35AM +0100, Michal Hocko wrote:

[...]

...

These hugetlb allocations might be disruptive and that is an expected behavior because this is an explicit requirement from an admin to pre-allocate large pages for the future use. __GFP_RETRY_MAYFAIL just underlines that requirement.

Yes, but in the absence of a particular node, for example via sysctl (as the compaction does), I don't think it is a hard requirement to get a page from a particular node.

Again this seems like a deliberate decision. You want your distributions as even as possible otherwise the NUMA placement will be much less deterministic. At least that was the case for a long time. If you have different per-node preferences, just use NUMA aware pre-allocation.

I agree we need __GFP_RETRY_FAIL, in any case the real root cause for me is should_reclaim_continue() which keeps the task looping without making forward progress.

This seems like a separate issue which should better be debugged. Please open a new thread describing the problem and the state of the node.