fix for "pathological THP behavior"

Reported-by Alex Williamson <[email protected]>

Signed-off-by Andrea Arcangeli <[email protected]>

---
 include/linux/gfp.h | 18 ++++++++++++++++++
 mm/mempolicy.c      | 12 +++++++++++-
 mm/page_alloc.c     |  4 ++++
 3 files changed, 33 insertions(+), 1 deletion(-)

include/linux/gfp.h +18 -0

index a6afcec53795..3c04d5d90e6d 100644
@@ -44,6 +44,7 @@ struct vm_area_struct;
 #else
 #define ___GFP_NOLOCKDEP	0
 #endif
+#define ___GFP_ONLY_COMPACT	0x1000000u
 /* If the above are modified, __GFP_BITS_SHIFT may need updating */
 
 /*

@@ -178,6 +179,21 @@ struct vm_area_struct;
  *   definitely preferable to use the flag rather than opencode endless
  *   loop around allocator.
  *   Using this flag for costly allocations is _highly_ discouraged.
+ *
+ * __GFP_ONLY_COMPACT: Only invoke compaction. Do not try to succeed
+ * the allocation by freeing memory. Never risk to free any
+ * "PAGE_SIZE" memory unit even if compaction failed specifically
+ * because of not enough free pages in the zone. This only makes sense
+ * only in combination with __GFP_THISNODE (enforced with a
+ * VM_WARN_ON), to restrict the THP allocation in the local node that
+ * triggered the page fault and fallback into PAGE_SIZE allocations in
+ * the same node. We don't want to invoke reclaim because there may be
+ * plenty of free memory already in the local node. More importantly
+ * there may be even plenty of free THP available in remote nodes so
+ * we should allocate those if something instead of reclaiming any
+ * memory in the local node. Implementation detail: set ___GFP_NORETRY
+ * too so that ___GFP_ONLY_COMPACT only needs to be checked in a slow
+ * path.
  */
 #define __GFP_IO	((__force gfp_t)___GFP_IO)
 #define __GFP_FS	((__force gfp_t)___GFP_FS)

@@ -187,6 +203,8 @@ struct vm_area_struct;
 #define __GFP_RETRY_MAYFAIL	((__force gfp_t)___GFP_RETRY_MAYFAIL)
 #define __GFP_NOFAIL	((__force gfp_t)___GFP_NOFAIL)
 #define __GFP_NORETRY	((__force gfp_t)___GFP_NORETRY)
+#define __GFP_ONLY_COMPACT	((__force gfp_t)(___GFP_NORETRY | \
+						 ___GFP_ONLY_COMPACT))
 
 /*
  * Action modifiers

mm/mempolicy.c +11 -1

index d6512ef28cde..6bf839f20dcc 100644
@@ -2047,8 +2047,18 @@ alloc_pages_vma(gfp_t gfp, int order, struct vm_area_struct *vma,
 
 		if (!nmask || node_isset(hpage_node, *nmask)) {
 			mpol_cond_put(pol);
+			/*
+			 * We restricted the allocation to the
+			 * hpage_node so we must use
+			 * __GFP_ONLY_COMPACT to allow at most a
+			 * compaction attempt and not ever get into
+			 * reclaim or it'll swap heavily with
+			 * transparent_hugepage/defrag = always (or
+			 * madvise under MADV_HUGEPAGE).
+			 */
 			page = __alloc_pages_node(hpage_node,
-						gfp | __GFP_THISNODE, order);
+						  gfp | __GFP_THISNODE |
+						  __GFP_ONLY_COMPACT, order);
 			goto out;
 		}
 	}

mm/page_alloc.c +0 -0

index a790ef4be74e..01a5c2bd0860 100644

I think this can also be triggered in khugepaged. In collapse_huge_page(), khugepaged_alloc_page() would also cause DIRECT_RECLAIM if defrag==always, since GFP_TRANSHUGE implies __GFP_DIRECT_RECLAIM.

But is it an expected behavior of khugepaged?

qemu uses MADV_HUGEPAGE which allows direct compaction (i.e. __GFP_DIRECT_RECLAIM is set).

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).

It's very easy to reproduce this by setting transparent_hugepage/defrag to "always", even with a simple memhog.

1) This can be fixed by retaining the __GFP_THISNODE logic also for __GFP_DIRECT_RELCAIM by allowing only one compaction run. Not even COMPACT_SKIPPED (i.e. compaction failing because not enough free memory in the zone) should be allowed to invoke reclaim.

2) An alternative is not use __GFP_THISNODE if __GFP_DIRECT_RELCAIM has been set by the caller (i.e. MADV_HUGEPAGE or defrag="always"). That would keep the NUMA locality restriction only when __GFP_DIRECT_RECLAIM is not set by the caller. So THP will be provided from remote nodes if available before falling back to PAGE_SIZE units in the local node, but an app using defrag = always (or madvise with MADV_HUGEPAGE) supposedly prefers that.

+ * __GFP_ONLY_COMPACT: Only invoke compaction. Do not try to succeed
+ * the allocation by freeing memory. Never risk to free any
+ * "PAGE_SIZE" memory unit even if compaction failed specifically
+ * because of not enough free pages in the zone. This only makes sense
+ * only in combination with __GFP_THISNODE (enforced with a
+ * VM_WARN_ON), to restrict the THP allocation in the local node that

+ * triggered the page fault and fallback into PAGE_SIZE allocations in
+ * the same node. We don't want to invoke reclaim because there may be
+ * plenty of free memory already in the local node. More importantly
+ * there may be even plenty of free THP available in remote nodes so
+ * we should allocate those if something instead of reclaiming any
+ * memory in the local node. Implementation detail: set ___GFP_NORETRY
+ * too so that ___GFP_ONLY_COMPACT only needs to be checked in a slow
+ * path.

So does reverting 5265047ac301 ("mm, thp: really limit transparent hugepage allocation to local node") help?

I really detest a new gfp flag for one time semantic that is muddy as hell.

This is simply incomprehensible. How can anybody who is not deeply familiar with the allocator/reclaim internals know when to use it.

If this is really a regression then we should start by pinpointing the

real culprit and go from there. If this is really 5265047ac301 then just

start by reverting it. I strongly suspect there is some mismatch in expectations here. What others consider acceptable seems to be a problem for others. I believe that was one of the reasons why we have changed the default THP direct compaction behavior, no?

On Tue, Aug 21, 2018 at 01:50:57PM +0200, Michal Hocko wrote:

Quoted 2 lines

I really detest a new gfp flag for one time semantic that is muddy as hell.

Well there's no way to fix this other than to prevent reclaim to run, if you still want to give a chance to page faults to obtain THP under MADV_HUGEPAGE in the page fault without waiting minutes or hours for khugpaged to catch up with it.

Quoted 2 lines

This is simply incomprehensible. How can anybody who is not deeply familiar with the allocator/reclaim internals know when to use it.

Nobody should use this in drivers, it's a __GFP flag.

Note:

	if (unlikely(IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE) && hugepage)) {

#define alloc_hugepage_vma(gfp_mask, vma, addr, order) \
alloc_pages_vma(gfp_mask, order, vma, addr, numa_node_id(), true)

Only THP is ever affected by the BUG so nothing else will ever need to call __GFP_COMPACT_ONLY. It is a VM internal flag, I wish there was a way to make the build fail if a driver would use it but there isn't right now.

Quoted 1 line

If this is really a regression then we should start by pinpointing the

You can check yourself, create a 2 node vnuma guest or pick any host with more than one node. Set defrag=always and run "memhog -r11111111 18g" if host has 16g per node. Add some swap and notice the swap storm while all ram is left free in the other node.

Quoted 1 line

real culprit and go from there. If this is really 5265047ac301 then just

In my view there's no single culprit, but it was easy to identify the last drop that made the MADV_HUGEPAGE glass overflow, and it's that commit that adds __GFP_THISNODE. The combination of the previous code that prioritized NUMA over THP and then the MADV_HUGEPAGE logic that still uses compaction (and in turn reclaim if compaction fails with COMPACT_SKIPPED because there's no 4k page in the local node) just falls apart with __GFP_THISNODE set as well on top of it and it doesn't do the expected thing either without it (i.e. THP gets priority over NUMA locality without such flag).

__GFP_THISNODE and the logic there, only works ok when __GFP_DIRECT_RECLAIM is not set, i.e. MADV_HUGEPAGE not set.

Quoted 1 line · Andrea Arcangeli

On Tue, Aug 21, 2018 at 01:50:57PM +0200, Michal Hocko wrote:

[...]

Quoted 7 lines

Quoted 2 lines

I really detest a new gfp flag for one time semantic that is muddy as hell.

Well there's no way to fix this other than to prevent reclaim to run, if you still want to give a chance to page faults to obtain THP under MADV_HUGEPAGE in the page fault without waiting minutes or hours for khugpaged to catch up with it.

I do not get that part. Why should caller even care about reclaim vs. compaction. How can you even make an educated guess what makes more sense? This should be fully controlled by the allocator path. The caller should only care about how hard to try. It's been some time since I've looked but we used to have a gfp flags to tell that for THP allocations as well.

Quoted 11 lines · Michal Hocko

Quoted 1 line · Andrea Arcangeli

On Tue, Aug 21, 2018 at 01:50:57PM +0200, Michal Hocko wrote:

[...]

Quoted 7 lines

Quoted 2 lines

I really detest a new gfp flag for one time semantic that is muddy as hell.

Well there's no way to fix this other than to prevent reclaim to run, if you still want to give a chance to page faults to obtain THP under MADV_HUGEPAGE in the page fault without waiting minutes or hours for khugpaged to catch up with it.

Quoted 6 lines

I do not get that part. Why should caller even care about reclaim vs. compaction. How can you even make an educated guess what makes more sense? This should be fully controlled by the allocator path. The caller should only care about how hard to try. It's been some time since I've looked but we used to have a gfp flags to tell that for THP allocations as well.

In other words, why do we even try to swap out when allocating costly high order page for requests which do not insist to try really hard?

I mean why don't we do something like this?
---

mm/vmscan.c +8 -0

index 03822f86f288..41005d3d4c2d 100644
@@ -3071,6 +3071,14 @@ unsigned long try_to_free_pages(struct zonelist *zonelist, int order,
 	if (throttle_direct_reclaim(sc.gfp_mask, zonelist, nodemask))
 		return 1;
 
+	/*
+	 * If we are allocating a costly order and do not insist on trying really
+	 * hard then we should keep the reclaim impact at minimum. So only
+	 * focus on easily reclaimable memory.
+	 */
+	if (order > PAGE_ALLOC_COSTLY_ORDER && !(gfp_mask & __GFP_RETRY_MAYFAIL))
+		sc.may_swap = sc.may_unmap = 0;
+
 	trace_mm_vmscan_direct_reclaim_begin(order,
 				sc.may_writepage,
 				sc.gfp_mask,

Signature · 2 lines

Michal Hocko
SUSE Labs

Quoted 19 lines · Michal Hocko

Quoted 17 lines · Michal Hocko

Quoted 1 line · Andrea Arcangeli

On Tue, Aug 21, 2018 at 01:50:57PM +0200, Michal Hocko wrote:

[...]

Quoted 7 lines

Quoted 2 lines

I really detest a new gfp flag for one time semantic that is muddy as hell.

Well there's no way to fix this other than to prevent reclaim to run, if you still want to give a chance to page faults to obtain THP under MADV_HUGEPAGE in the page fault without waiting minutes or hours for khugpaged to catch up with it.

I do not get that part. Why should caller even care about reclaim vs. compaction. How can you even make an educated guess what makes more sense? This should be fully controlled by the allocator path. The caller should only care about how hard to try. It's been some time since I've looked but we used to have a gfp flags to tell that for THP allocations as well.

Quoted 2 lines

In other words, why do we even try to swap out when allocating costly high order page for requests which do not insist to try really hard?

Note that the testcase with vfio swaps nothing and writes nothing to disk. No memory at all is being swapped or freed because 100% of the node is pinned with GUP pins, so I'm dubious this could possible move the needle for the reproducer that I used for the benchmark.

The swap storm I suggested to you as reproducer, because it's another way the bug would see the light of the day and it's easier to reproduce without requiring device assignment, but the badness is the fact reclaim is called when it shouldn't be and whatever fix must cover vfio too. The below I can't imagine how it could possibly have an effect on vfio, and even for the swap storm case you're converting a swap storm into a CPU waste, it'll still run just extremely slow allocations like with vfio.

The effect of the below should be evaluated regardless of the issue we've been discussing in this thread and it's a new corner case for order > PAGE_ALLOC_COSTLY_ORDER. I don't like very much order > PAGE_ALLOC_COSTLY_ORDER checks, those are arbitrary numbers, the more checks are needed in various places for that, the more it's a sign the VM is bad and arbitrary and with one more corner case required to hide some badness. But again this will have effects unrelated to what we're discussing here and it will just convert I/O into CPU waste and have no effect on vfio.

Now I am confused. How can compaction help at all then? I mean if the node is full of GUP pins then you can hardly do anything but fallback to other node. Or how come your new GFP flag makes any difference?

It would still try to reclaim easy target as compaction requires. If you do not reclaim at all you can make the current implementation of the compaction noop due to its own watermark checks IIRC.

yeah, I agree about PAGE_ALLOC_COSTLY_ORDER being an arbitrary limit for a different behavior. But we already do handle those specially so it kind of makes sense to me to expand on that.

Quoted 3 lines · Michal Hocko

Now I am confused. How can compaction help at all then? I mean if the node is full of GUP pins then you can hardly do anything but fallback to other node. Or how come your new GFP flag makes any difference?

It helps until the node is full.

If you don't call compaction you will get zero THP even when you've plenty of free memory.

So the free memory goes down and down as more and more THP are generated y compaction until compaction then fails with COMPACT_SKIPPED, there's not enough free memory to relocate an "order 9" amount of physically contiguous PAGE_SIZEd fragments.

At that point the code calls reclaim to make space for a new compaction run. Then if that fails again it's not because there's no enough free memory.

Problem is if you ever call reclaim when compaction fails, what happens is you free an "order 9" and then it gets eaten up by the app so then next compaction call, calls COMPACT_SKIPPED again.

This is how compaction works since day zero it was introduced in kernel 2.6.x something, if you don't have crystal clear the inner workings of compaction you have an hard time to review this. So hope the above shed some light of how this plays out.

So in general calling reclaim is ok because compaction fails more often than not in such case because it can't compact memory not because there aren't at least 2m free in any node. However when you use __GFP_THISNODE combined with reclaim that changes the whole angle and behavior of compaction if reclaim is still active.

Not calling compaction in MADV_HUGEPAGE means you can drop MADV_HUGEPAGE as a whole. There's no point to ever set it unless we call compaction. And if you don't call at least once compaction you have near zero chances to get gigabytes of THP even if it's all compactable memory and there are gigabytes of free memory in the node, after some runtime that shakes the fragments in the buddy.

To make it even more clear why compaction has to run once at least when MADV_HUGEPAGE is set, just check the second last column of your /proc/buddyinfo before and after "echo 3 >/proc/sys/vm/drop_caches; echo >/proc/sys/vm/compact_memory". Try to allocate memory without MADV_HUGEPAGE and without running the "echo 3; echo" and see how much THP you'll get. I've plenty of workloads that use MADV_HUGEPAGE not just qemu and that totally benefit immediately from THP and there's no point to ever defer compaction to khugepaged when userland says "this is a long lived allocation".

Compaction is only potentially wasteful for short lived allocation, so MADV_HUGEPAGE has to call compaction.

Quoted 3 lines

It would still try to reclaim easy target as compaction requires. If you do not reclaim at all you can make the current implementation of the compaction noop due to its own watermark checks IIRC.

That's the feature, if you don't make it a noop when watermark checks trigger, it'll end up wasting CPU and breaking vfio.

The point is that we want compaction to run when there's free memory and compaction keeps succeeding.

So when compaction fails, if it's because we finished all free memory in the node, we should just remove __GFP_THISNODE and allocate without it (i.e. the optimization). If compaction fails because the memory is fragmented but here's still free memory we should fail the allocation and trigger the THP fallback to PAGE_SIZE fault.

Overall removing __GFP_THISNODE unconditionally would simply prioritize THP over NUMA locality which is the point of this special logic for THP. I can't blame the logic because it certainly helps NUMA balancing a lot in letting the memory be in the right place from the start. This is why __GFP_COMPACT_ONLY makes sense, to be able to retain the logic but still preventing the corner case of such __GFP_THISNODE that breaks the VM with MADV_HUGEPAGE.

Quoted 3 lines

yeah, I agree about PAGE_ALLOC_COSTLY_ORDER being an arbitrary limit for a different behavior. But we already do handle those specially so it kind of makes sense to me to expand on that.

It's still a sign of one more place that needs magic for whatever reason. So unless it can be justified by some runtime tests I wouldn't make such change by just thinking about it. Reclaim is called if there's no free memory left anywhere for compaction to run (i.e. if __GFP_THISNODE is not set, if __GPF_THISNODE is set then the caller better use __GFP_COMPACT_ONLY).

Now we could also get away without __GFP_COMPACT_ONLY, we could check __GFP_THISNODE and make it behave exactly like __GFP_COMPACT_ONLY whenever __GFP_DIRECT_RECLAIM was also set in addition of __GFP_THISNODE, but then you couldn't use __GFP_THISNODE as a mbind anymore and it would have more obscure semantics than a new flag I think.

I am not disputing the bug itself. How hard should defrag=allways really try is good question and I would say different people would have different ideas but a swapping storm sounds like genuinely unwanted behavior. I would expect that to be handled in the reclaim/compaction. GFP_TRANSHUGE doesn't have ___GFP_RETRY_MAYFAIL so it shouldn't really try too hard to reclaim.

I still have to digest the __GFP_THISNODE thing but I _think_ that the alloc_pages_vma code is just trying to be overly clever and __GFP_THISNODE is not a good fit for it.

On Wed, Aug 22, 2018 at 11:02:14AM +0200, Michal Hocko wrote:

Quoted 3 lines

I still have to digest the __GFP_THISNODE thing but I _think_ that the alloc_pages_vma code is just trying to be overly clever and __GFP_THISNODE is not a good fit for it.

My option 2 did just that, it removed __GFP_THISNODE but only for MADV_HUGEPAGE and in general whenever reclaim was activated by __GFP_DIRECT_RECLAIM. That is also signal that the user really wants THP so then it's less bad to prefer THP over NUMA locality.

For the default which is tuned for short lived allocation, preferring local memory is most certainly better win for short lived allocation where THP can't help much, this is why I didn't remove __GFP_THISNODE from the default defrag policy.

Quoted 1 line · Andrea Arcangeli

On Wed, Aug 22, 2018 at 11:02:14AM +0200, Michal Hocko wrote:

[...]

Quoted 9 lines

Quoted 3 lines

I still have to digest the __GFP_THISNODE thing but I _think_ that the alloc_pages_vma code is just trying to be overly clever and __GFP_THISNODE is not a good fit for it.

My option 2 did just that, it removed __GFP_THISNODE but only for MADV_HUGEPAGE and in general whenever reclaim was activated by __GFP_DIRECT_RECLAIM. That is also signal that the user really wants THP so then it's less bad to prefer THP over NUMA locality.

Quoted 4 lines

For the default which is tuned for short lived allocation, preferring local memory is most certainly better win for short lived allocation where THP can't help much, this is why I didn't remove __GFP_THISNODE from the default defrag policy.

Yes I agree.

Quoted 12 lines · Michal Hocko

Quoted 1 line · Andrea Arcangeli

On Wed, Aug 22, 2018 at 11:02:14AM +0200, Michal Hocko wrote:

[...]

Quoted 9 lines

Quoted 3 lines

I still have to digest the __GFP_THISNODE thing but I _think_ that the alloc_pages_vma code is just trying to be overly clever and __GFP_THISNODE is not a good fit for it.

My option 2 did just that, it removed __GFP_THISNODE but only for MADV_HUGEPAGE and in general whenever reclaim was activated by __GFP_DIRECT_RECLAIM. That is also signal that the user really wants THP so then it's less bad to prefer THP over NUMA locality.

Quoted 6 lines

Quoted 4 lines

For the default which is tuned for short lived allocation, preferring local memory is most certainly better win for short lived allocation where THP can't help much, this is why I didn't remove __GFP_THISNODE from the default defrag policy.

Yes I agree.

I finally got back to this again. I have checked your patch and I am really wondering whether alloc_pages_vma is really the proper place to play these tricks. We already have that mind blowing alloc_hugepage_direct_gfpmask and it should be the proper place to handle this special casing. So what do you think about the following. It should be essentially the same thing. Aka use __GFP_THIS_NODE only when we are doing an optimistic THP allocation. Madvise signalizes you know what you are doing and THP has the top priority. If you care enough about the numa placement then you should better use mempolicy.

[CC Stefan Priebe who has reported the same/similar issue on openSUSE
mailing list recently - the thread starts http://lkml.kernel.org/r/[email protected]]

Quoted 20 lines · Michal Hocko

Quoted 12 lines · Michal Hocko

Quoted 1 line · Andrea Arcangeli

On Wed, Aug 22, 2018 at 11:02:14AM +0200, Michal Hocko wrote:

[...]

Quoted 9 lines

Quoted 3 lines

I still have to digest the __GFP_THISNODE thing but I _think_ that the alloc_pages_vma code is just trying to be overly clever and __GFP_THISNODE is not a good fit for it.

My option 2 did just that, it removed __GFP_THISNODE but only for MADV_HUGEPAGE and in general whenever reclaim was activated by __GFP_DIRECT_RECLAIM. That is also signal that the user really wants THP so then it's less bad to prefer THP over NUMA locality.

Quoted 6 lines

Quoted 4 lines

For the default which is tuned for short lived allocation, preferring local memory is most certainly better win for short lived allocation where THP can't help much, this is why I didn't remove __GFP_THISNODE from the default defrag policy.

Yes I agree.

Quoted 9 lines

I finally got back to this again. I have checked your patch and I am really wondering whether alloc_pages_vma is really the proper place to play these tricks. We already have that mind blowing alloc_hugepage_direct_gfpmask and it should be the proper place to handle this special casing. So what do you think about the following. It should be essentially the same thing. Aka use __GFP_THIS_NODE only when we are doing an optimistic THP allocation. Madvise signalizes you know what you are doing and THP has the top priority. If you care enough about the numa placement then you should better use mempolicy.

Now the patch is still untested but it compiles at least.

---
From 88e0ca4c9c403c6046f1c47d5ee17548f9dc841a Mon Sep 17 00:00:00 2001
From: Michal Hocko <[email protected]>
Date: Tue, 28 Aug 2018 09:59:19 +0200
Subject: [PATCH] mm, thp: relax __GFP_THISNODE for MADV_HUGEPAGE mappings

Andrea has noticed [1] that a 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-phSLE15 (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 handling from alloc_pages_vma where it doesn't belong and move it to alloc_hugepage_direct_gfpmask where we juggle gfp flags for different allocation modes. The rationale is that __GFP_THISNODE is helpful in relaxed defrag modes because falling back to a different node might be more harmful than the benefit of a large page. If the user really requires THP (e.g. by MADV_HUGEPAGE) then the THP has a higher priority than local NUMA placement. The later might be controlled via NUMA policies to be more fine grained.

[1] http://lkml.kernel.org/r/[email protected]

Fixes 5265047ac301 ("mm, thp: really limit transparent hugepage allocation to local node")

Reported-by Stefan Priebe <[email protected]>

Debugged-by Andrea Arcangeli <[email protected]>

Signed-off-by Michal Hocko <[email protected]>

---
 mm/huge_memory.c | 10 +++++-----
 mm/mempolicy.c   | 26 --------------------------
 2 files changed, 5 insertions(+), 31 deletions(-)

mm/huge_memory.c +5 -5

index c3bc7e9c9a2a..a703c23f8bab 100644
@@ -634,16 +634,16 @@ static inline gfp_t alloc_hugepage_direct_gfpmask(struct vm_area_struct *vma)
 	const bool vma_madvised = !!(vma->vm_flags & VM_HUGEPAGE);
 
 	if (test_bit(TRANSPARENT_HUGEPAGE_DEFRAG_DIRECT_FLAG, &transparent_hugepage_flags))
-		return GFP_TRANSHUGE | (vma_madvised ? 0 : __GFP_NORETRY);
+		return GFP_TRANSHUGE | (vma_madvised ? 0 : __GFP_NORETRY | __GFP_THISNODE);
 	if (test_bit(TRANSPARENT_HUGEPAGE_DEFRAG_KSWAPD_FLAG, &transparent_hugepage_flags))
-		return GFP_TRANSHUGE_LIGHT | __GFP_KSWAPD_RECLAIM;
+		return GFP_TRANSHUGE_LIGHT | __GFP_KSWAPD_RECLAIM | __GFP_THISNODE;
 	if (test_bit(TRANSPARENT_HUGEPAGE_DEFRAG_KSWAPD_OR_MADV_FLAG, &transparent_hugepage_flags))
 		return GFP_TRANSHUGE_LIGHT | (vma_madvised ? __GFP_DIRECT_RECLAIM :
-							     __GFP_KSWAPD_RECLAIM);
+							     __GFP_KSWAPD_RECLAIM | __GFP_THISNODE);
 	if (test_bit(TRANSPARENT_HUGEPAGE_DEFRAG_REQ_MADV_FLAG, &transparent_hugepage_flags))
 		return GFP_TRANSHUGE_LIGHT | (vma_madvised ? __GFP_DIRECT_RECLAIM :
-							     0);
-	return GFP_TRANSHUGE_LIGHT;
+							     __GFP_THISNODE);
+	return GFP_TRANSHUGE_LIGHT | __GFP_THISNODE;
 }
 
 /* Caller must hold page table lock. */

mm/mempolicy.c +0 -26

index da858f794eb6..9f0800885613 100644
@@ -2026,32 +2026,6 @@ alloc_pages_vma(gfp_t gfp, int order, struct vm_area_struct *vma,
 		goto out;
 	}
 
-	if (unlikely(IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE) && hugepage)) {
-		int hpage_node = node;
-
-		/*
-		 * For hugepage allocation and non-interleave policy which
-		 * allows the current node (or other explicitly preferred
-		 * node) we only try to allocate from the current/preferred
-		 * node and don't fall back to other nodes, as the cost of
-		 * remote accesses would likely offset THP benefits.
-		 *
-		 * If the policy is interleave, or does not allow the current
-		 * node in its nodemask, we allocate the standard way.
-		 */
-		if (pol->mode == MPOL_PREFERRED &&
-						!(pol->flags & MPOL_F_LOCAL))
-			hpage_node = pol->v.preferred_node;
-
-		nmask = policy_nodemask(gfp, pol);
-		if (!nmask || node_isset(hpage_node, *nmask)) {
-			mpol_cond_put(pol);
-			page = __alloc_pages_node(hpage_node,
-						gfp | __GFP_THISNODE, order);
-			goto out;
-		}
-	}
-
 	nmask = policy_nodemask(gfp, pol);
 	preferred_nid = policy_node(gfp, pol, node);
 	page = __alloc_pages_nodemask(gfp, order, preferred_nid, nmask);

Am 28.08.2018 um 10:18 schrieb Michal Hocko:

Quoted 34 lines

[CC Stefan Priebe who has reported the same/similar issue on openSUSE
mailing list recently - the thread starts http://lkml.kernel.org/r/[email protected]]

Quoted 29 lines · Michal Hocko

Quoted 12 lines · Michal Hocko

Quoted 1 line · Andrea Arcangeli

On Wed, Aug 22, 2018 at 11:02:14AM +0200, Michal Hocko wrote:

[...]

Quoted 9 lines

Quoted 3 lines

I still have to digest the __GFP_THISNODE thing but I _think_ that the alloc_pages_vma code is just trying to be overly clever and __GFP_THISNODE is not a good fit for it.

My option 2 did just that, it removed __GFP_THISNODE but only for MADV_HUGEPAGE and in general whenever reclaim was activated by __GFP_DIRECT_RECLAIM. That is also signal that the user really wants THP so then it's less bad to prefer THP over NUMA locality.

Quoted 6 lines

Quoted 4 lines

For the default which is tuned for short lived allocation, preferring local memory is most certainly better win for short lived allocation where THP can't help much, this is why I didn't remove __GFP_THISNODE from the default defrag policy.

Yes I agree.

I finally got back to this again. I have checked your patch and I am really wondering whether alloc_pages_vma is really the proper place to play these tricks. We already have that mind blowing alloc_hugepage_direct_gfpmask and it should be the proper place to handle this special casing. So what do you think about the following. It should be essentially the same thing. Aka use __GFP_THIS_NODE only when we are doing an optimistic THP allocation. Madvise signalizes you know what you are doing and THP has the top priority. If you care enough about the numa placement then you should better use mempolicy.

Quoted 1 line

Now the patch is still untested but it compiles at least.

Great - i recompiled the SLES15 kernel with that one applied and will test if it helps.

Quoted 156 lines

---
From 88e0ca4c9c403c6046f1c47d5ee17548f9dc841a Mon Sep 17 00:00:00 2001
From: Michal Hocko <[email protected]>
Date: Tue, 28 Aug 2018 09:59:19 +0200
Subject: [PATCH] mm, thp: relax __GFP_THISNODE for MADV_HUGEPAGE mappings

Andrea has noticed [1] that a 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-phSLE15 (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 handling from alloc_pages_vma where it doesn't belong and move it to alloc_hugepage_direct_gfpmask where we juggle gfp flags for different allocation modes. The rationale is that __GFP_THISNODE is helpful in relaxed defrag modes because falling back to a different node might be more harmful than the benefit of a large page. If the user really requires THP (e.g. by MADV_HUGEPAGE) then the THP has a higher priority than local NUMA placement. The later might be controlled via NUMA policies to be more fine grained.

[1] http://lkml.kernel.org/r/[email protected]

Fixes 5265047ac301 ("mm, thp: really limit transparent hugepage allocation to local node")

Reported-by Stefan Priebe <[email protected]>

Debugged-by Andrea Arcangeli <[email protected]>

Signed-off-by Michal Hocko <[email protected]>

---
 mm/huge_memory.c | 10 +++++-----
 mm/mempolicy.c   | 26 --------------------------
 2 files changed, 5 insertions(+), 31 deletions(-)

mm/huge_memory.c +5 -5

index c3bc7e9c9a2a..a703c23f8bab 100644
@@ -634,16 +634,16 @@ static inline gfp_t alloc_hugepage_direct_gfpmask(struct vm_area_struct *vma)
 	const bool vma_madvised = !!(vma->vm_flags & VM_HUGEPAGE);
 
 	if (test_bit(TRANSPARENT_HUGEPAGE_DEFRAG_DIRECT_FLAG, &transparent_hugepage_flags))
-		return GFP_TRANSHUGE | (vma_madvised ? 0 : __GFP_NORETRY);
+		return GFP_TRANSHUGE | (vma_madvised ? 0 : __GFP_NORETRY | __GFP_THISNODE);
 	if (test_bit(TRANSPARENT_HUGEPAGE_DEFRAG_KSWAPD_FLAG, &transparent_hugepage_flags))
-		return GFP_TRANSHUGE_LIGHT | __GFP_KSWAPD_RECLAIM;
+		return GFP_TRANSHUGE_LIGHT | __GFP_KSWAPD_RECLAIM | __GFP_THISNODE;
 	if (test_bit(TRANSPARENT_HUGEPAGE_DEFRAG_KSWAPD_OR_MADV_FLAG, &transparent_hugepage_flags))
 		return GFP_TRANSHUGE_LIGHT | (vma_madvised ? __GFP_DIRECT_RECLAIM :
-							     __GFP_KSWAPD_RECLAIM);
+							     __GFP_KSWAPD_RECLAIM | __GFP_THISNODE);
 	if (test_bit(TRANSPARENT_HUGEPAGE_DEFRAG_REQ_MADV_FLAG, &transparent_hugepage_flags))
 		return GFP_TRANSHUGE_LIGHT | (vma_madvised ? __GFP_DIRECT_RECLAIM :
-							     0);
-	return GFP_TRANSHUGE_LIGHT;
+							     __GFP_THISNODE);
+	return GFP_TRANSHUGE_LIGHT | __GFP_THISNODE;
 }
 
 /* Caller must hold page table lock. */

mm/mempolicy.c +0 -26

index da858f794eb6..9f0800885613 100644
@@ -2026,32 +2026,6 @@ alloc_pages_vma(gfp_t gfp, int order, struct vm_area_struct *vma,
 		goto out;
 	}
 
-	if (unlikely(IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE) && hugepage)) {
-		int hpage_node = node;
-
-		/*
-		 * For hugepage allocation and non-interleave policy which
-		 * allows the current node (or other explicitly preferred
-		 * node) we only try to allocate from the current/preferred
-		 * node and don't fall back to other nodes, as the cost of
-		 * remote accesses would likely offset THP benefits.
-		 *
-		 * If the policy is interleave, or does not allow the current
-		 * node in its nodemask, we allocate the standard way.
-		 */
-		if (pol->mode == MPOL_PREFERRED &&
-						!(pol->flags & MPOL_F_LOCAL))
-			hpage_node = pol->v.preferred_node;
-
-		nmask = policy_nodemask(gfp, pol);
-		if (!nmask || node_isset(hpage_node, *nmask)) {
-			mpol_cond_put(pol);
-			page = __alloc_pages_node(hpage_node,
-						gfp | __GFP_THISNODE, order);
-			goto out;
-		}
-	}
-
 	nmask = policy_nodemask(gfp, pol);
 	preferred_nid = policy_node(gfp, pol, node);
 	page = __alloc_pages_nodemask(gfp, order, preferred_nid, nmask);

This patch triggers WARN_ON_ONCE() in policy_node() when MPOL_BIND is used and THP is on. Should this WARN_ON_ONCE be removed?

/*
* __GFP_THISNODE shouldn't even be used with the bind policy
* because we might easily break the expectation to stay on the
* requested node and not break the policy.
*/
WARN_ON_ONCE(policy->mode == MPOL_BIND && (gfp & __GFP_THISNODE));

On Wed 29-08-18 09:28:21, Zi Yan wrote:
[...]

Quoted 10 lines

This patch triggers WARN_ON_ONCE() in policy_node() when MPOL_BIND is used and THP is on. Should this WARN_ON_ONCE be removed?

/*
* __GFP_THISNODE shouldn't even be used with the bind policy
* because we might easily break the expectation to stay on the
* requested node and not break the policy.
*/
WARN_ON_ONCE(policy->mode == MPOL_BIND && (gfp & __GFP_THISNODE));

This is really interesting. It seems to be me who added this warning but I cannot simply make any sense of it. Let me try to dig some more.

Quoted 13 lines · Michal Hocko

On Wed 29-08-18 09:28:21, Zi Yan wrote:
[...]

Quoted 10 lines

This patch triggers WARN_ON_ONCE() in policy_node() when MPOL_BIND is used and THP is on. Should this WARN_ON_ONCE be removed?

/*
* __GFP_THISNODE shouldn't even be used with the bind policy
* because we might easily break the expectation to stay on the
* requested node and not break the policy.
*/
WARN_ON_ONCE(policy->mode == MPOL_BIND && (gfp & __GFP_THISNODE));

Quoted 2 lines

This is really interesting. It seems to be me who added this warning but I cannot simply make any sense of it. Let me try to dig some more.

OK, I get it now. The warning seems to be incomplete. It is right to complain when __GFP_THISNODE disagrees with MPOL_BIND policy but that is not what we check here. Does this heal the warning?

mm/mempolicy.c +0 -0

index da858f794eb6..7bb9354b1e4c 100644
@@ -1728,7 +1728,10 @@ static int policy_node(gfp_t gfp, struct mempolicy *policy,
 		 * because we might easily break the expectation to stay on the

Quoted 26 lines · Michal Hocko

Quoted 13 lines · Michal Hocko

On Wed 29-08-18 09:28:21, Zi Yan wrote:
[...]

Quoted 10 lines

This patch triggers WARN_ON_ONCE() in policy_node() when MPOL_BIND is used and THP is on. Should this WARN_ON_ONCE be removed?

/*
* __GFP_THISNODE shouldn't even be used with the bind policy
* because we might easily break the expectation to stay on the
* requested node and not break the policy.
*/
WARN_ON_ONCE(policy->mode == MPOL_BIND && (gfp & __GFP_THISNODE));

Quoted 2 lines

This is really interesting. It seems to be me who added this warning but I cannot simply make any sense of it. Let me try to dig some more.

OK, I get it now. The warning seems to be incomplete. It is right to complain when __GFP_THISNODE disagrees with MPOL_BIND policy but that is not what we check here. Does this heal the warning?

mm/mempolicy.c +0 -0

index da858f794eb6..7bb9354b1e4c 100644
@@ -1728,7 +1728,10 @@ static int policy_node(gfp_t gfp, struct mempolicy *policy,
 		 * because we might easily break the expectation to stay on the

mm/mempolicy.c static int policy_node(gfp_t gfp, struct mempolicy *policy, · line 1737

Quoted 10 lines

mm/mempolicy.c +4 -1

 		 * requested node and not break the policy.
 		 */
-		WARN_ON_ONCE(policy->mode == MPOL_BIND && (gfp & __GFP_THISNODE));
+		if (policy->mode == MPOL_BIND && (gfp & __GFP_THISNODE)) {
+			nodemask_t *nmask = policy_nodemask(gfp, policy);
+			WARN_ON_ONCE(!node_isset(nd, *nmask));
+		}
 	}
 
 	return nd;

Unfortunately no. I simply ran a??memhog -r3 1g membind 1a?? to test and the warning still showed up.

The reason is that nd is just a hint about which node to prefer for allocation and can be ignored if it does not conform to mempolicy.

Taking my test as an example, if an application is only memory bound to node 1 but can run on any CPU nodes and it launches on node 0, alloc_pages_vma() will see 0 as its node parameter and passes 0 to policy_node()a??s nd parameter. This should be OK, but your patches would give a warning, because nd=0 is not set in nmask=1.

Now I get your comment a??__GFP_THISNODE shouldn't even be used with the bind policya??, since they are indeed incompatible. __GFP_THISNODE wants to use the node, which can be ignored by MPOL_BIND policy.

IMHO, we could get rid of __GFP_THISNODE when MPOL_BIND is set, like

mm/mempolicy.c +7 -8

index 0d2be5786b0c..a0fcb998d277 100644
@@ -1722,14 +1722,6 @@ static int policy_node(gfp_t gfp, struct mempolicy *policy,
 {
        if (policy->mode == MPOL_PREFERRED && !(policy->flags & MPOL_F_LOCAL))
                nd = policy->v.preferred_node;
-       else {
-               /*
-                * __GFP_THISNODE shouldn't even be used with the bind policy
-                * because we might easily break the expectation to stay on the
-                * requested node and not break the policy.
-                */
-               WARN_ON_ONCE(policy->mode == MPOL_BIND && (gfp & __GFP_THISNODE));
-       }
 
        return nd;
 }

@@ -2026,6 +2018,13 @@ alloc_pages_vma(gfp_t gfp, int order, struct vm_area_struct *vma,
                goto out;
        }
 
+       /*
+        * __GFP_THISNODE shouldn't even be used with the bind policy
+        * because we might easily break the expectation to stay on the
+        * requested node and not break the policy.
+        */
+       if (pol->mode == MPOL_BIND)
+               gfp &= ~__GFP_THISNODE;
 
        nmask = policy_nodemask(gfp, pol);
        preferred_nid = policy_node(gfp, pol, node);

What do you think?

I do not like overwriting gfp flags like that. It is just ugly and error prone. A more proper way would be to handle that at the layer we play with __GFP_THISNODE. The resulting diff is larger though.

If there is a general concensus that this is growing too complicated then Andrea's patch (the second variant to overwrite gfp mask) is much simpler of course but I really detest the subtle gfp rewriting. I still believe that all the nasty details should be covered at the single place.

include/linux/mempolicy.h +2 -0

index 5228c62af416..bac395f1d00a 100644
@@ -139,6 +139,8 @@ struct mempolicy *mpol_shared_policy_lookup(struct shared_policy *sp,
 struct mempolicy *get_task_policy(struct task_struct *p);
 struct mempolicy *__get_vma_policy(struct vm_area_struct *vma,
 		unsigned long addr);
+struct mempolicy *get_vma_policy(struct vm_area_struct *vma,
+						unsigned long addr);
 bool vma_policy_mof(struct vm_area_struct *vma);
 
 extern void numa_default_policy(void);

mm/huge_memory.c +17 -8

index a703c23f8bab..94472bf9a31b 100644
@@ -629,21 +629,30 @@ static vm_fault_t __do_huge_pmd_anonymous_page(struct vm_fault *vmf,
  *	    available
  * never: never stall for any thp allocation
  */
-static inline gfp_t alloc_hugepage_direct_gfpmask(struct vm_area_struct *vma)
+static inline gfp_t alloc_hugepage_direct_gfpmask(struct vm_area_struct *vma, unsigned long addr)
 {
 	const bool vma_madvised = !!(vma->vm_flags & VM_HUGEPAGE);
+	gfp_t this_node = 0;
+	struct mempolicy *pol;
+
+#ifdef CONFIG_NUMA
+	/* __GFP_THISNODE makes sense only if there is no explicit binding */
+	pol = get_vma_policy(vma, addr);
+	if (pol->mode != MPOL_BIND)
+		this_node = __GFP_THISNODE;
+#endif
 
 	if (test_bit(TRANSPARENT_HUGEPAGE_DEFRAG_DIRECT_FLAG, &transparent_hugepage_flags))
-		return GFP_TRANSHUGE | (vma_madvised ? 0 : __GFP_NORETRY | __GFP_THISNODE);
+		return GFP_TRANSHUGE | (vma_madvised ? 0 : __GFP_NORETRY | this_node);
 	if (test_bit(TRANSPARENT_HUGEPAGE_DEFRAG_KSWAPD_FLAG, &transparent_hugepage_flags))
-		return GFP_TRANSHUGE_LIGHT | __GFP_KSWAPD_RECLAIM | __GFP_THISNODE;
+		return GFP_TRANSHUGE_LIGHT | __GFP_KSWAPD_RECLAIM | this_node;
 	if (test_bit(TRANSPARENT_HUGEPAGE_DEFRAG_KSWAPD_OR_MADV_FLAG, &transparent_hugepage_flags))
 		return GFP_TRANSHUGE_LIGHT | (vma_madvised ? __GFP_DIRECT_RECLAIM :
-							     __GFP_KSWAPD_RECLAIM | __GFP_THISNODE);
+							     __GFP_KSWAPD_RECLAIM | this_node);
 	if (test_bit(TRANSPARENT_HUGEPAGE_DEFRAG_REQ_MADV_FLAG, &transparent_hugepage_flags))
 		return GFP_TRANSHUGE_LIGHT | (vma_madvised ? __GFP_DIRECT_RECLAIM :
-							     __GFP_THISNODE);
-	return GFP_TRANSHUGE_LIGHT | __GFP_THISNODE;
+							     this_node);
+	return GFP_TRANSHUGE_LIGHT | this_node;
 }
 
 /* Caller must hold page table lock. */

@@ -715,7 +724,7 @@ vm_fault_t do_huge_pmd_anonymous_page(struct vm_fault *vmf)
 			pte_free(vma->vm_mm, pgtable);
 		return ret;
 	}
-	gfp = alloc_hugepage_direct_gfpmask(vma);
+	gfp = alloc_hugepage_direct_gfpmask(vma, haddr);
 	page = alloc_hugepage_vma(gfp, vma, haddr, HPAGE_PMD_ORDER);
 	if (unlikely(!page)) {
 		count_vm_event(THP_FAULT_FALLBACK);

@@ -1290,7 +1299,7 @@ vm_fault_t do_huge_pmd_wp_page(struct vm_fault *vmf, pmd_t orig_pmd)
 alloc:
 	if (transparent_hugepage_enabled(vma) &&
 	    !transparent_hugepage_debug_cow()) {
-		huge_gfp = alloc_hugepage_direct_gfpmask(vma);
+		huge_gfp = alloc_hugepage_direct_gfpmask(vma, haddr);
 		new_page = alloc_hugepage_vma(huge_gfp, vma, haddr, HPAGE_PMD_ORDER);
 	} else
 		new_page = NULL;

mm/mempolicy.c +0 -0

index 9f0800885613..75bbfc3d6233 100644

The warning goes away with this change. I am OK with this patch (plus the original one you sent out, which could be merged with this one).

Quoted 2 lines · Zi Yan

The warning goes away with this change. I am OK with this patch (plus the original one you sent out, which could be merged with this one).

I will respin the patch, update the changelog and repost. Tomorrow I hope.

Fixes 5265047ac301 ("mm, thp: really limit transparent hugepage allocation to local node")

Reported-by Stefan Priebe <[email protected]>

Debugged-by Andrea Arcangeli <[email protected]>

Signed-off-by Michal Hocko <[email protected]>

---
 include/linux/mempolicy.h |  2 ++
 mm/huge_memory.c          | 25 +++++++++++++++++--------
 mm/mempolicy.c            | 28 +---------------------------
 3 files changed, 20 insertions(+), 35 deletions(-)

include/linux/mempolicy.h +2 -0

index 5228c62af416..bac395f1d00a 100644
@@ -139,6 +139,8 @@ struct mempolicy *mpol_shared_policy_lookup(struct shared_policy *sp,
 struct mempolicy *get_task_policy(struct task_struct *p);
 struct mempolicy *__get_vma_policy(struct vm_area_struct *vma,
 		unsigned long addr);
+struct mempolicy *get_vma_policy(struct vm_area_struct *vma,
+						unsigned long addr);
 bool vma_policy_mof(struct vm_area_struct *vma);
 
 extern void numa_default_policy(void);

mm/huge_memory.c +17 -8

index c3bc7e9c9a2a..94472bf9a31b 100644
@@ -629,21 +629,30 @@ static vm_fault_t __do_huge_pmd_anonymous_page(struct vm_fault *vmf,
  *	    available
  * never: never stall for any thp allocation
  */
-static inline gfp_t alloc_hugepage_direct_gfpmask(struct vm_area_struct *vma)
+static inline gfp_t alloc_hugepage_direct_gfpmask(struct vm_area_struct *vma, unsigned long addr)
 {
 	const bool vma_madvised = !!(vma->vm_flags & VM_HUGEPAGE);
+	gfp_t this_node = 0;
+	struct mempolicy *pol;
+
+#ifdef CONFIG_NUMA
+	/* __GFP_THISNODE makes sense only if there is no explicit binding */
+	pol = get_vma_policy(vma, addr);
+	if (pol->mode != MPOL_BIND)
+		this_node = __GFP_THISNODE;
+#endif
 
 	if (test_bit(TRANSPARENT_HUGEPAGE_DEFRAG_DIRECT_FLAG, &transparent_hugepage_flags))
-		return GFP_TRANSHUGE | (vma_madvised ? 0 : __GFP_NORETRY);
+		return GFP_TRANSHUGE | (vma_madvised ? 0 : __GFP_NORETRY | this_node);
 	if (test_bit(TRANSPARENT_HUGEPAGE_DEFRAG_KSWAPD_FLAG, &transparent_hugepage_flags))
-		return GFP_TRANSHUGE_LIGHT | __GFP_KSWAPD_RECLAIM;
+		return GFP_TRANSHUGE_LIGHT | __GFP_KSWAPD_RECLAIM | this_node;
 	if (test_bit(TRANSPARENT_HUGEPAGE_DEFRAG_KSWAPD_OR_MADV_FLAG, &transparent_hugepage_flags))
 		return GFP_TRANSHUGE_LIGHT | (vma_madvised ? __GFP_DIRECT_RECLAIM :
-							     __GFP_KSWAPD_RECLAIM);
+							     __GFP_KSWAPD_RECLAIM | this_node);
 	if (test_bit(TRANSPARENT_HUGEPAGE_DEFRAG_REQ_MADV_FLAG, &transparent_hugepage_flags))
 		return GFP_TRANSHUGE_LIGHT | (vma_madvised ? __GFP_DIRECT_RECLAIM :
-							     0);
-	return GFP_TRANSHUGE_LIGHT;
+							     this_node);
+	return GFP_TRANSHUGE_LIGHT | this_node;
 }
 
 /* Caller must hold page table lock. */

@@ -715,7 +724,7 @@ vm_fault_t do_huge_pmd_anonymous_page(struct vm_fault *vmf)
 			pte_free(vma->vm_mm, pgtable);
 		return ret;
 	}
-	gfp = alloc_hugepage_direct_gfpmask(vma);
+	gfp = alloc_hugepage_direct_gfpmask(vma, haddr);
 	page = alloc_hugepage_vma(gfp, vma, haddr, HPAGE_PMD_ORDER);
 	if (unlikely(!page)) {
 		count_vm_event(THP_FAULT_FALLBACK);

@@ -1290,7 +1299,7 @@ vm_fault_t do_huge_pmd_wp_page(struct vm_fault *vmf, pmd_t orig_pmd)
 alloc:
 	if (transparent_hugepage_enabled(vma) &&
 	    !transparent_hugepage_debug_cow()) {
-		huge_gfp = alloc_hugepage_direct_gfpmask(vma);
+		huge_gfp = alloc_hugepage_direct_gfpmask(vma, haddr);
 		new_page = alloc_hugepage_vma(huge_gfp, vma, haddr, HPAGE_PMD_ORDER);
 	} else
 		new_page = NULL;

mm/mempolicy.c +1 -27

index da858f794eb6..75bbfc3d6233 100644
@@ -1648,7 +1648,7 @@ struct mempolicy *__get_vma_policy(struct vm_area_struct *vma,
  * freeing by another task.  It is the caller's responsibility to free the
  * extra reference for shared policies.
  */
-static struct mempolicy *get_vma_policy(struct vm_area_struct *vma,
+struct mempolicy *get_vma_policy(struct vm_area_struct *vma,
 						unsigned long addr)
 {
 	struct mempolicy *pol = __get_vma_policy(vma, addr);

@@ -2026,32 +2026,6 @@ alloc_pages_vma(gfp_t gfp, int order, struct vm_area_struct *vma,
 		goto out;
 	}
 
-	if (unlikely(IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE) && hugepage)) {
-		int hpage_node = node;
-
-		/*
-		 * For hugepage allocation and non-interleave policy which
-		 * allows the current node (or other explicitly preferred
-		 * node) we only try to allocate from the current/preferred
-		 * node and don't fall back to other nodes, as the cost of
-		 * remote accesses would likely offset THP benefits.
-		 *
-		 * If the policy is interleave, or does not allow the current
-		 * node in its nodemask, we allocate the standard way.
-		 */
-		if (pol->mode == MPOL_PREFERRED &&
-						!(pol->flags & MPOL_F_LOCAL))
-			hpage_node = pol->v.preferred_node;
-
-		nmask = policy_nodemask(gfp, pol);
-		if (!nmask || node_isset(hpage_node, *nmask)) {
-			mpol_cond_put(pol);
-			page = __alloc_pages_node(hpage_node,
-						gfp | __GFP_THISNODE, order);
-			goto out;
-		}
-	}
-
 	nmask = policy_nodemask(gfp, pol);
 	preferred_nid = policy_node(gfp, pol, node);
 	page = __alloc_pages_nodemask(gfp, order, preferred_nid, nmask);

Signature · 1 line

2.18.0

I tested it against Linusa??s tree with a??memhog -r3 130ga?? in a two-socket machine with 128GB memory on each node and got the results below. I expect this test should fill one node, then fall back to the other.

1. madvise(MADV_HUGEPAGE) + defrag = {always, madvise, defer+madvise}:
no swap, THPs are allocated in the fallback node.
2. madvise(MADV_HUGEPAGE) + defrag = defer: pages got swapped to the disk instead of being allocated in the fallback node.
3. no madvise, THP is on by default + defrag = {always, defer, defer+madvise}: pages got swapped to the disk instead of being allocated in the fallback node.
4. no madvise, THP is on by default + defrag = madvise: no swap, base pages are allocated in the fallback node.

The result 2 and 3 seems unexpected, since pages should be allocated in the fallback node.

The reason, as Andrea mentioned in his email, is that the combination of __THIS_NODE and __GFP_DIRECT_RECLAIM (plus __GFP_KSWAPD_RECLAIM from this experiment).

__THIS_NODE uses ZONELIST_NOFALLBACK, which
removes the fallback possibility and __GFP_*_RECLAIM triggers page reclaim in the first page allocation node when fallback nodes are removed by ZONELIST_NOFALLBACK.

IMHO, __THIS_NODE should not be used for user memory allocation at all, since it fights against most of memory policies. But kernel memory allocation would need it as a kernel MPOL_BIND memory policy.

On Wed 29-08-18 18:54:23, Zi Yan wrote:
[...]

Quoted 15 lines

I tested it against Linus’s tree with “memhog -r3 130g” in a two-socket machine with 128GB memory on each node and got the results below. I expect this test should fill one node, then fall back to the other.

1. madvise(MADV_HUGEPAGE) + defrag = {always, madvise, defer+madvise}:
no swap, THPs are allocated in the fallback node.
2. madvise(MADV_HUGEPAGE) + defrag = defer: pages got swapped to the disk instead of being allocated in the fallback node.
3. no madvise, THP is on by default + defrag = {always, defer, defer+madvise}: pages got swapped to the disk instead of being allocated in the fallback node.
4. no madvise, THP is on by default + defrag = madvise: no swap, base pages are allocated in the fallback node.

The result 2 and 3 seems unexpected, since pages should be allocated in the fallback node.

Quoted 3 lines

The reason, as Andrea mentioned in his email, is that the combination of __THIS_NODE and __GFP_DIRECT_RECLAIM (plus __GFP_KSWAPD_RECLAIM from this experiment).

But we do not set __GFP_THISNODE along with __GFP_DIRECT_RECLAIM AFAICS. We do for __GFP_KSWAPD_RECLAIM though and I guess that it is expected to see kswapd do the reclaim to balance the node. If the node is full of anonymous pages then there is no other way than swap out.

Quoted 5 lines

__THIS_NODE uses ZONELIST_NOFALLBACK, which
removes the fallback possibility and __GFP_*_RECLAIM triggers page reclaim in the first page allocation node when fallback nodes are removed by ZONELIST_NOFALLBACK.

Yes but the point is that the allocations which use __GFP_THISNODE are optimistic so they shouldn't fallback to remote NUMA nodes.

Quoted 4 lines

IMHO, __THIS_NODE should not be used for user memory allocation at all, since it fights against most of memory policies. But kernel memory allocation would need it as a kernel MPOL_BIND memory policy.

__GFP_THISNODE is indeed an ugliness. I would really love to get rid of it here. But the problem is that optimistic THP allocations should prefer a local node because a remote node might easily offset the advantage of the THP. I do not have a great idea how to achieve that without __GFP_THISNODE though.

Quoted 21 lines · Michal Hocko

On Wed 29-08-18 18:54:23, Zi Yan wrote:
[...]

Quoted 18 lines

I tested it against Linusa??s tree with a??memhog -r3 130ga?? in a two-socket machine with 128GB memory on each node and got the results below. I expect this test should fill one node, then fall back to the other.

1. madvise(MADV_HUGEPAGE) + defrag = {always, madvise, defer+madvise}:
no swap, THPs are allocated in the fallback node.
2. madvise(MADV_HUGEPAGE) + defrag = defer: pages got swapped to the disk instead of being allocated in the fallback node.
3. no madvise, THP is on by default + defrag = {always, defer, defer+madvise}: pages got swapped to the disk instead of being allocated in the fallback node.
4. no madvise, THP is on by default + defrag = madvise: no swap, base pages are allocated in the fallback node.

The result 2 and 3 seems unexpected, since pages should be allocated in the fallback node.

The reason, as Andrea mentioned in his email, is that the combination of __THIS_NODE and __GFP_DIRECT_RECLAIM (plus __GFP_KSWAPD_RECLAIM from this experiment).

Quoted 4 lines

But we do not set __GFP_THISNODE along with __GFP_DIRECT_RECLAIM AFAICS. We do for __GFP_KSWAPD_RECLAIM though and I guess that it is expected to see kswapd do the reclaim to balance the node. If the node is full of anonymous pages then there is no other way than swap out.

GFP_TRANSHUGE implies __GFP_DIRECT_RECLAIM. When no madvise is given, THP is on
+ defrag=always, gfp_mask has __GFP_THISNODE and __GFP_DIRECT_RECLAIM, so swapping can be triggered.

The key issue here is that a??memhog -r3 130ga?? uses the default memory policy (MPOL_DEFAULT), which should allow page allocation fallback to other nodes, but as shown in result 3, swapping is triggered instead of page allocation fallback.

Quoted 7 lines

Quoted 4 lines

__THIS_NODE uses ZONELIST_NOFALLBACK, which
removes the fallback possibility and __GFP_*_RECLAIM triggers page reclaim in the first page allocation node when fallback nodes are removed by ZONELIST_NOFALLBACK.

Yes but the point is that the allocations which use __GFP_THISNODE are optimistic so they shouldn't fallback to remote NUMA nodes.

This can be achieved by using MPOL_BIND memory policy which restricts nodemask in struct alloc_context for user space memory allocations.

Quoted 9 lines

Quoted 3 lines

IMHO, __THIS_NODE should not be used for user memory allocation at all, since it fights against most of memory policies. But kernel memory allocation would need it as a kernel MPOL_BIND memory policy.

__GFP_THISNODE is indeed an ugliness. I would really love to get rid of it here. But the problem is that optimistic THP allocations should prefer a local node because a remote node might easily offset the advantage of the THP. I do not have a great idea how to achieve that without __GFP_THISNODE though.

MPOL_PREFERRED memory policy can be used to achieve this optimistic THP allocation for user space. Even with the default memory policy, local memory node will be used first until it is full. It seems to me that __GFP_THISNODE is not necessary if a proper memory policy is used.

Let me know if I miss anything. Thanks.

Quoted 23 lines · Zi Yan

Quoted 21 lines · Michal Hocko

On Wed 29-08-18 18:54:23, Zi Yan wrote:
[...]

Quoted 18 lines

I tested it against Linus’s tree with “memhog -r3 130g” in a two-socket machine with 128GB memory on each node and got the results below. I expect this test should fill one node, then fall back to the other.

1. madvise(MADV_HUGEPAGE) + defrag = {always, madvise, defer+madvise}:
no swap, THPs are allocated in the fallback node.
2. madvise(MADV_HUGEPAGE) + defrag = defer: pages got swapped to the disk instead of being allocated in the fallback node.
3. no madvise, THP is on by default + defrag = {always, defer, defer+madvise}: pages got swapped to the disk instead of being allocated in the fallback node.
4. no madvise, THP is on by default + defrag = madvise: no swap, base pages are allocated in the fallback node.

The result 2 and 3 seems unexpected, since pages should be allocated in the fallback node.

The reason, as Andrea mentioned in his email, is that the combination of __THIS_NODE and __GFP_DIRECT_RECLAIM (plus __GFP_KSWAPD_RECLAIM from this experiment).

Quoted 8 lines

Quoted 4 lines

But we do not set __GFP_THISNODE along with __GFP_DIRECT_RECLAIM AFAICS. We do for __GFP_KSWAPD_RECLAIM though and I guess that it is expected to see kswapd do the reclaim to balance the node. If the node is full of anonymous pages then there is no other way than swap out.

GFP_TRANSHUGE implies __GFP_DIRECT_RECLAIM. When no madvise is given, THP is on
+ defrag=always, gfp_mask has __GFP_THISNODE and __GFP_DIRECT_RECLAIM, so swapping can be triggered.

Yes, but the setup tells that you are willing to pay price to get a THP. defered=always uses that special __GFP_NORETRY (unless it is madvised mapping) that should back off if the compaction failed recently. How much that reduces the reclaim is not really clear to me right now to be honest.

Quoted 3 lines

The key issue here is that “memhog -r3 130g” uses the default memory policy (MPOL_DEFAULT), which should allow page allocation fallback to other nodes, but as shown in result 3, swapping is triggered instead of page allocation fallback.

Well, I guess this really depends. Fallback to a different node might be seen as a bad thing and worse than the reclaim on the local node.

Quoted 10 lines

Quoted 7 lines

Quoted 4 lines

__THIS_NODE uses ZONELIST_NOFALLBACK, which
removes the fallback possibility and __GFP_*_RECLAIM triggers page reclaim in the first page allocation node when fallback nodes are removed by ZONELIST_NOFALLBACK.

Yes but the point is that the allocations which use __GFP_THISNODE are optimistic so they shouldn't fallback to remote NUMA nodes.

This can be achieved by using MPOL_BIND memory policy which restricts nodemask in struct alloc_context for user space memory allocations.

Yes, but that requires and explicit NUMA handling. And we are trying to handle those cases which do not really give a damn and just want to use THP if it is available or try harder when they ask by using madvise.

Quoted 10 lines

Quoted 9 lines

Quoted 3 lines

IMHO, __THIS_NODE should not be used for user memory allocation at all, since it fights against most of memory policies. But kernel memory allocation would need it as a kernel MPOL_BIND memory policy.

__GFP_THISNODE is indeed an ugliness. I would really love to get rid of it here. But the problem is that optimistic THP allocations should prefer a local node because a remote node might easily offset the advantage of the THP. I do not have a great idea how to achieve that without __GFP_THISNODE though.

Quoted 7 lines

MPOL_PREFERRED memory policy can be used to achieve this optimistic THP allocation for user space. Even with the default memory policy, local memory node will be used first until it is full. It seems to me that __GFP_THISNODE is not necessary if a proper memory policy is used.

Let me know if I miss anything. Thanks.

You are missing that we are trying to define a sensible model for those who do not really care about mempolicies. THP shouldn't cause more harm than good for those.

I wish we could come up with a remotely sane and comprehensible model. That means that you know how hard the allocator tries to get a THP for you depending on the defrag configuration, your memory policy and your madvise setting. The easiest one I can think of is to
- always follow mempolicy when specified because you asked for it explicitly
- stay node local and low latency for the light THP defrag mode (defrag, madvise without hint and none) because THP is a nice to have
- if the defrag mode is always then you are willing to pay the latency price but off-node might be still a no-no.
- allow fallback for madvised mappings because you really want THP. If you care about specific numa placement then combine with the mempolicy.

As you can see I do not really mention anything about the direct reclaim because that is just an implementation detail of the page allocator and compaction interaction.

Maybe you can formulate a saner matrix with all the available modes that we have.

Anyway, I guess we can agree that (almost) unconditional __GFP_THISNODE is clearly wrong and we should address that first. Either Andrea's option 2) patch or mine which does the similar thing except at the proper layer (I believe). We can continue discussing other odd cases on top I guess. Unless somebody has much brighter idea, of course.

Am 30.08.2018 um 16:02 schrieb Zi Yan <[email protected]>:

Tested-by Zi Yan <[email protected]>

Quoted 101 lines · Michal Hocko

Quoted 23 lines · Zi Yan

Quoted 21 lines · Michal Hocko

On Wed 29-08-18 18:54:23, Zi Yan wrote:
[...]

Quoted 18 lines

I tested it against Linusa??s tree with a??memhog -r3 130ga?? in a two-socket machine with 128GB memory on each node and got the results below. I expect this test should fill one node, then fall back to the other.

1. madvise(MADV_HUGEPAGE) + defrag = {always, madvise, defer+madvise}:
no swap, THPs are allocated in the fallback node.
2. madvise(MADV_HUGEPAGE) + defrag = defer: pages got swapped to the disk instead of being allocated in the fallback node.
3. no madvise, THP is on by default + defrag = {always, defer, defer+madvise}: pages got swapped to the disk instead of being allocated in the fallback node.
4. no madvise, THP is on by default + defrag = madvise: no swap, base pages are allocated in the fallback node.

The result 2 and 3 seems unexpected, since pages should be allocated in the fallback node.

The reason, as Andrea mentioned in his email, is that the combination of __THIS_NODE and __GFP_DIRECT_RECLAIM (plus __GFP_KSWAPD_RECLAIM from this experiment).

Quoted 8 lines

Quoted 4 lines

But we do not set __GFP_THISNODE along with __GFP_DIRECT_RECLAIM AFAICS. We do for __GFP_KSWAPD_RECLAIM though and I guess that it is expected to see kswapd do the reclaim to balance the node. If the node is full of anonymous pages then there is no other way than swap out.

GFP_TRANSHUGE implies __GFP_DIRECT_RECLAIM. When no madvise is given, THP is on
+ defrag=always, gfp_mask has __GFP_THISNODE and __GFP_DIRECT_RECLAIM, so swapping can be triggered.

Yes, but the setup tells that you are willing to pay price to get a THP. defered=always uses that special __GFP_NORETRY (unless it is madvised mapping) that should back off if the compaction failed recently. How much that reduces the reclaim is not really clear to me right now to be honest.

Quoted 3 lines

The key issue here is that a??memhog -r3 130ga?? uses the default memory policy (MPOL_DEFAULT), which should allow page allocation fallback to other nodes, but as shown in result 3, swapping is triggered instead of page allocation fallback.

Well, I guess this really depends. Fallback to a different node might be seen as a bad thing and worse than the reclaim on the local node.

Quoted 10 lines

Quoted 7 lines

Quoted 4 lines

__THIS_NODE uses ZONELIST_NOFALLBACK, which
removes the fallback possibility and __GFP_*_RECLAIM triggers page reclaim in the first page allocation node when fallback nodes are removed by ZONELIST_NOFALLBACK.

Yes but the point is that the allocations which use __GFP_THISNODE are optimistic so they shouldn't fallback to remote NUMA nodes.

This can be achieved by using MPOL_BIND memory policy which restricts nodemask in struct alloc_context for user space memory allocations.

Yes, but that requires and explicit NUMA handling. And we are trying to handle those cases which do not really give a damn and just want to use THP if it is available or try harder when they ask by using madvise.

Quoted 10 lines

Quoted 9 lines

Quoted 3 lines

IMHO, __THIS_NODE should not be used for user memory allocation at all, since it fights against most of memory policies. But kernel memory allocation would need it as a kernel MPOL_BIND memory policy.

__GFP_THISNODE is indeed an ugliness. I would really love to get rid of it here. But the problem is that optimistic THP allocations should prefer a local node because a remote node might easily offset the advantage of the THP. I do not have a great idea how to achieve that without __GFP_THISNODE though.

Quoted 7 lines

MPOL_PREFERRED memory policy can be used to achieve this optimistic THP allocation for user space. Even with the default memory policy, local memory node will be used first until it is full. It seems to me that __GFP_THISNODE is not necessary if a proper memory policy is used.

Let me know if I miss anything. Thanks.

You are missing that we are trying to define a sensible model for those who do not really care about mempolicies. THP shouldn't cause more harm than good for those.

I wish we could come up with a remotely sane and comprehensible model. That means that you know how hard the allocator tries to get a THP for you depending on the defrag configuration, your memory policy and your madvise setting. The easiest one I can think of is to
- always follow mempolicy when specified because you asked for it explicitly
- stay node local and low latency for the light THP defrag mode (defrag, madvise without hint and none) because THP is a nice to have
- if the defrag mode is always then you are willing to pay the latency price but off-node might be still a no-no.
- allow fallback for madvised mappings because you really want THP. If you care about specific numa placement then combine with the mempolicy.

As you can see I do not really mention anything about the direct reclaim because that is just an implementation detail of the page allocator and compaction interaction.

Quoted 8 lines

Maybe you can formulate a saner matrix with all the available modes that we have.

Anyway, I guess we can agree that (almost) unconditional __GFP_THISNODE is clearly wrong and we should address that first. Either Andrea's option 2) patch or mine which does the similar thing except at the proper layer (I believe). We can continue discussing other odd cases on top I guess. Unless somebody has much brighter idea, of course.

Thanks for your explanation. It makes sense to me. I am fine with your patch. You can add my Tested-by: Zi Yan <[email protected]>, since my test result 1 shows that the problem mentioned in your changelog is solved.

Quoted 5 lines · Zi Yan

Quoted 3 lines · Michal Hocko

Quoted 2 lines · Zi Yan

On 30 Aug 2018, at 3:00, Michal Hocko wrote:

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines · Zi Yan

[...]

Quoted 5 lines

I tested it against Linusa??s tree with a??memhog -r3 130ga?? in a two-socket machine with 128GB memory on each node and got the results below. I expect this test should fill one node, then fall back to the other.

1. madvise(MADV_HUGEPAGE) + defrag = {always, madvise, defer+madvise}:
no swap, THPs are allocated in the fallback node.

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

2. madvise(MADV_HUGEPAGE) + defrag = defer: pages got swapped to the disk instead of being allocated in the fallback node.

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

3. no madvise, THP is on by default + defrag = {always, defer, defer+madvise}: pages got swapped to the disk instead of being allocated in the fallback node.

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

4. no madvise, THP is on by default + defrag = madvise: no swap, base pages are allocated in the fallback node.

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

The result 2 and 3 seems unexpected, since pages should be allocated in the fallback node.

Thanks for your and Stefan's testing. I will wait for some more feedback. I will be offline next few days and if there are no major objections I will repost with both tested-bys early next week.

That kind of swapping may only pay off in the very long long term, which is what khugepaged is for. khugepaged already takes care of the long term, so we could later argue and think if khugepaged should swapout or not in such condition, but I don't think there's much to argue about the page fault.

Quoted 3 lines

Thanks for your and Stefan's testing. I will wait for some more feedback. I will be offline next few days and if there are no major objections I will repost with both tested-bys early next week.

I'm not so positive about 2 of the above tests if I understood the test correctly.

Those results are totally fine if you used the non default memory policy, but with MPOL_DEFAULT and in turn no hard bind of the memory, I'm afraid it'll be even be harder to reproduce when things will go wrong again in those two cases.

On Tue 04-09-18 23:44:03, Andrea Arcangeli wrote:
[...]

Quoted 5 lines

That kind of swapping may only pay off in the very long long term, which is what khugepaged is for. khugepaged already takes care of the long term, so we could later argue and think if khugepaged should swapout or not in such condition, but I don't think there's much to argue about the page fault.

I agree that defrag==always doing a reclaim is not really good and benefits are questionable. If you remember this was the primary reason why the default has been changed.

Quoted 11 lines

Quoted 3 lines

Thanks for your and Stefan's testing. I will wait for some more feedback. I will be offline next few days and if there are no major objections I will repost with both tested-bys early next week.

I'm not so positive about 2 of the above tests if I understood the test correctly.

Those results are totally fine if you used the non default memory policy, but with MPOL_DEFAULT and in turn no hard bind of the memory, I'm afraid it'll be even be harder to reproduce when things will go wrong again in those two cases.

We can and should think about this much more but I would like to have this regression closed. So can we address GFP_THISNODE part first and build more complex solution on top?

Is there any objection to my patch which does the similar thing to your patch v2 in a different location?

Quoted 6 lines

We can and should think about this much more but I would like to have this regression closed. So can we address GFP_THISNODE part first and build more complex solution on top?

Is there any objection to my patch which does the similar thing to your patch v2 in a different location?

Similar but not the same. It fixes the madvise case, but I wonder about the no-madvise defrag=defer case, where Zi Yan reports it still causes swapping.

Quoted 10 lines · Vlastimil Babka

Quoted 6 lines

We can and should think about this much more but I would like to have this regression closed. So can we address GFP_THISNODE part first and build more complex solution on top?

Is there any objection to my patch which does the similar thing to your patch v2 in a different location?

Similar but not the same. It fixes the madvise case, but I wonder about the no-madvise defrag=defer case, where Zi Yan reports it still causes swapping.

Ah, but that should be the same with Andrea's variant 2) patch. There should only be difference with defrag=always, which is direct reclaim with __GFP_NORETRY, Andrea's patch would drop __GFP_THISNODE and your not. Maybe Zi Yan can do the same kind of tests with Andrea's patch [1] to confirm?

Quoted 12 lines · Vlastimil Babka

Quoted 10 lines · Vlastimil Babka

Quoted 6 lines

We can and should think about this much more but I would like to have this regression closed. So can we address GFP_THISNODE part first and build more complex solution on top?

Is there any objection to my patch which does the similar thing to your patch v2 in a different location?

Similar but not the same. It fixes the madvise case, but I wonder about the no-madvise defrag=defer case, where Zi Yan reports it still causes swapping.

Quoted 5 lines

Ah, but that should be the same with Andrea's variant 2) patch. There should only be difference with defrag=always, which is direct reclaim with __GFP_NORETRY, Andrea's patch would drop __GFP_THISNODE and your not. Maybe Zi Yan can do the same kind of tests with Andrea's patch [1] to confirm?

Yes, that is the only difference and that is why I've said those patches are mostly similar. I do not want to touch defrag=always case because this one has always been stall prone and we have replaced it as a default just because of that. We should discuss what should be done with that case separately IMHO.

Thanks for your patch.

I tested it against Linusa??s tree with a??memhog -r3 130ga?? in a two-socket machine with 128GB memory on each node and got the results below. I expect this test should fill one node, then fall back to the other.

1. madvise(MADV_HUGEPAGE) + defrag = {always, madvise, defer+madvise}: no swap, THPs are allocated in the fallback node.
2. madvise(MADV_HUGEPAGE) + defrag = defer: pages got swapped to the disk instead of being allocated in the fallback node.

3. no madvise, THP is on by default + defrag = {always, defer, defer+madvise}: pages got swapped to the disk instead of being allocated in the fallback node.

Quoted 6 lines · Zi Yan

Thanks for your patch.

I tested it against Linus’s tree with “memhog -r3 130g” in a two-socket machine with 128GB memory on each node and got the results below. I expect this test should fill one node, then fall back to the other.

Quoted 2 lines

1. madvise(MADV_HUGEPAGE) + defrag = {always, madvise, defer+madvise}: no swap, THPs are allocated in the fallback node.
2. madvise(MADV_HUGEPAGE) + defrag = defer: pages got swapped to the disk instead of being allocated in the fallback node.

Hmm this is GFP_TRANSHUGE_LIGHT | __GFP_KSWAPD_RECLAIM | __GFP_THISNODE. No direct reclaim, so it would have to be kswapd causing the swapping? I wouldn't expect it to be significant and over-reclaiming. What exactly is your definition of "pages got swapped"?

Quoted 9 lines · Michal Hocko

mm/huge_memory.c +6 -1

-static inline gfp_t alloc_hugepage_direct_gfpmask(struct vm_area_struct *vma)
+static inline gfp_t alloc_hugepage_direct_gfpmask(struct vm_area_struct *vma, unsigned long addr)
 {
 	const bool vma_madvised = !!(vma->vm_flags & VM_HUGEPAGE);
+	gfp_t this_node = 0;
+	struct mempolicy *pol;
+
+#ifdef CONFIG_NUMA
+	/* __GFP_THISNODE makes sense only if there is no explicit binding */

mm/huge_memory.c static vm_fault_t __do_huge_pmd_anonymous_page(struct vm_fault *vmf,

Quoted 4 lines

mm/huge_memory.c +4 -0

+	pol = get_vma_policy(vma, addr);
+	if (pol->mode != MPOL_BIND)
+		this_node = __GFP_THISNODE;
+	mpol_cond_put(pol);

The code is better without the hack in alloc_pages_vma() but I'm not thrilled about getting vma policy here and then immediately again in alloc_pages_vma(). But if it can't be helped...

From 4dc2f772756e6f91b9e64d1a3e2df4dca3475f5b Mon Sep 17 00:00:00 2001
From: Michal Hocko <[email protected]>
Date: Tue, 28 Aug 2018 09:59:19 +0200
Subject: [PATCH] mm, thp: relax __GFP_THISNODE for MADV_HUGEPAGE mappings

Andrea has noticed [1] that a 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:

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 handling from alloc_pages_vma where it doesn't belong and move it to alloc_hugepage_direct_gfpmask where we juggle gfp flags for different allocation modes.

The rationale is that
__GFP_THISNODE is helpful in relaxed defrag modes because falling back to a different node might be more harmful than the benefit of a large page. If the user really requires THP (e.g. by MADV_HUGEPAGE) then the THP has a higher priority than local NUMA placement.

Be careful when the vma has an explicit numa binding though, because __GFP_THISNODE is not playing well with it. We want to follow the explicit numa policy rather than enforce a node which happens to be local to the cpu we are running on.

[1] http://lkml.kernel.org/r/[email protected]

Fixes 5265047ac301 ("mm, thp: really limit transparent hugepage allocation to local node")

Reported-by Stefan Priebe <[email protected]>

Debugged-by Andrea Arcangeli <[email protected]>

Signed-off-by Michal Hocko <[email protected]>

---
 include/linux/mempolicy.h |  2 ++
 mm/huge_memory.c          | 25 +++++++++++++++++--------
 mm/mempolicy.c            | 28 +---------------------------
 3 files changed, 20 insertions(+), 35 deletions(-)

include/linux/mempolicy.h +2 -0

index 5228c62af416..bac395f1d00a 100644
@@ -139,6 +139,8 @@ struct mempolicy *mpol_shared_policy_lookup(struct shared_policy *sp,
 struct mempolicy *get_task_policy(struct task_struct *p);
 struct mempolicy *__get_vma_policy(struct vm_area_struct *vma,
 		unsigned long addr);
+struct mempolicy *get_vma_policy(struct vm_area_struct *vma,
+						unsigned long addr);
 bool vma_policy_mof(struct vm_area_struct *vma);
 
 extern void numa_default_policy(void);

mm/huge_memory.c +1 -1

index c3bc7e9c9a2a..94472bf9a31b 100644
@@ -629,21 +629,30 @@ static vm_fault_t __do_huge_pmd_anonymous_page(struct vm_fault *vmf,
  *	    available
  * never: never stall for any thp allocation
  */
-static inline gfp_t alloc_hugepage_direct_gfpmask(struct vm_area_struct *vma)
+static inline gfp_t alloc_hugepage_direct_gfpmask(struct vm_area_struct *vma, unsigned long addr)
 {
 	const bool vma_madvised = !!(vma->vm_flags & VM_HUGEPAGE);

mm/huge_memory.c +7 -7

 	if (test_bit(TRANSPARENT_HUGEPAGE_DEFRAG_DIRECT_FLAG, &transparent_hugepage_flags))
-		return GFP_TRANSHUGE | (vma_madvised ? 0 : __GFP_NORETRY);
+		return GFP_TRANSHUGE | (vma_madvised ? 0 : __GFP_NORETRY | this_node);
 	if (test_bit(TRANSPARENT_HUGEPAGE_DEFRAG_KSWAPD_FLAG, &transparent_hugepage_flags))
-		return GFP_TRANSHUGE_LIGHT | __GFP_KSWAPD_RECLAIM;
+		return GFP_TRANSHUGE_LIGHT | __GFP_KSWAPD_RECLAIM | this_node;
 	if (test_bit(TRANSPARENT_HUGEPAGE_DEFRAG_KSWAPD_OR_MADV_FLAG, &transparent_hugepage_flags))
 		return GFP_TRANSHUGE_LIGHT | (vma_madvised ? __GFP_DIRECT_RECLAIM :
-							     __GFP_KSWAPD_RECLAIM);
+							     __GFP_KSWAPD_RECLAIM | this_node);
 	if (test_bit(TRANSPARENT_HUGEPAGE_DEFRAG_REQ_MADV_FLAG, &transparent_hugepage_flags))
 		return GFP_TRANSHUGE_LIGHT | (vma_madvised ? __GFP_DIRECT_RECLAIM :
-							     0);
-	return GFP_TRANSHUGE_LIGHT;
+							     this_node);
+	return GFP_TRANSHUGE_LIGHT | this_node;
 }
 
 /* Caller must hold page table lock. */

@@ -715,7 +724,7 @@ vm_fault_t do_huge_pmd_anonymous_page(struct vm_fault *vmf)
 			pte_free(vma->vm_mm, pgtable);
 		return ret;
 	}
-	gfp = alloc_hugepage_direct_gfpmask(vma);
+	gfp = alloc_hugepage_direct_gfpmask(vma, haddr);
 	page = alloc_hugepage_vma(gfp, vma, haddr, HPAGE_PMD_ORDER);
 	if (unlikely(!page)) {
 		count_vm_event(THP_FAULT_FALLBACK);

@@ -1290,7 +1299,7 @@ vm_fault_t do_huge_pmd_wp_page(struct vm_fault *vmf, pmd_t orig_pmd)
 alloc:
 	if (transparent_hugepage_enabled(vma) &&
 	    !transparent_hugepage_debug_cow()) {
-		huge_gfp = alloc_hugepage_direct_gfpmask(vma);
+		huge_gfp = alloc_hugepage_direct_gfpmask(vma, haddr);
 		new_page = alloc_hugepage_vma(huge_gfp, vma, haddr, HPAGE_PMD_ORDER);
 	} else
 		new_page = NULL;

mm/mempolicy.c +1 -17

index da858f794eb6..75bbfc3d6233 100644
@@ -1648,7 +1648,7 @@ struct mempolicy *__get_vma_policy(struct vm_area_struct *vma,
  * freeing by another task.  It is the caller's responsibility to free the
  * extra reference for shared policies.
  */
-static struct mempolicy *get_vma_policy(struct vm_area_struct *vma,
+struct mempolicy *get_vma_policy(struct vm_area_struct *vma,
 						unsigned long addr)
 {
 	struct mempolicy *pol = __get_vma_policy(vma, addr);

@@ -2026,32 +2026,6 @@ alloc_pages_vma(gfp_t gfp, int order, struct vm_area_struct *vma,
 		goto out;
 	}
 
-	if (unlikely(IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE) && hugepage)) {
-		int hpage_node = node;
-
-		/*
-		 * For hugepage allocation and non-interleave policy which
-		 * allows the current node (or other explicitly preferred
-		 * node) we only try to allocate from the current/preferred
-		 * node and don't fall back to other nodes, as the cost of
-		 * remote accesses would likely offset THP benefits.
-		 *
-		 * If the policy is interleave, or does not allow the current
-		 * node in its nodemask, we allocate the standard way.
-		 */
-		if (pol->mode == MPOL_PREFERRED &&
-						!(pol->flags & MPOL_F_LOCAL))
-			hpage_node = pol->v.preferred_node;

On Wed, Aug 29, 2018 at 09:24:51PM +0200, Michal Hocko wrote:

I recognise that this fix means that users that expect zone_reclaim_mode==1 type behaviour may get burned but the users that benefit from that should also be users that benefit from sizing their workload to a node. They should be able to replicate that with mempolicies or at least use prepation scripts to clear memory on a target node (e.g. membind a memhog to the desired size, exit and then start the target workload).

Quoted 31 lines

include/linux/mempolicy.h +2 -0

index 5228c62af416..bac395f1d00a 100644
@@ -139,6 +139,8 @@ struct mempolicy *mpol_shared_policy_lookup(struct shared_policy *sp,
 struct mempolicy *get_task_policy(struct task_struct *p);
 struct mempolicy *__get_vma_policy(struct vm_area_struct *vma,
 		unsigned long addr);
+struct mempolicy *get_vma_policy(struct vm_area_struct *vma,
+						unsigned long addr);
 bool vma_policy_mof(struct vm_area_struct *vma);
 
 extern void numa_default_policy(void);

mm/huge_memory.c +7 -1

index c3bc7e9c9a2a..94472bf9a31b 100644
@@ -629,21 +629,30 @@ static vm_fault_t __do_huge_pmd_anonymous_page(struct vm_fault *vmf,
  *	    available
  * never: never stall for any thp allocation
  */
-static inline gfp_t alloc_hugepage_direct_gfpmask(struct vm_area_struct *vma)
+static inline gfp_t alloc_hugepage_direct_gfpmask(struct vm_area_struct *vma, unsigned long addr)
 {
 	const bool vma_madvised = !!(vma->vm_flags & VM_HUGEPAGE);
+	gfp_t this_node = 0;
+	struct mempolicy *pol;
+
+#ifdef CONFIG_NUMA
+	/* __GFP_THISNODE makes sense only if there is no explicit binding */
+	pol = get_vma_policy(vma, addr);

mm/huge_memory.c static vm_fault_t __do_huge_pmd_anonymous_page(struct vm_fault *vmf, · line 644

Quoted 4 lines

mm/huge_memory.c +3 -0

+	if (pol->mode != MPOL_BIND)
+		this_node = __GFP_THISNODE;
+#endif
 

Where is the mpol_cond_put? Historically it might not have mattered because THP could not be used with a shared possibility but it probably matters now that tmpfs can be backed by THP.

The comment needs more expansion as well. Arguably it only makes sense in the event we are explicitly bound to one node because if we are bound to two nodes without interleaving then why not fall back? The answer to that is outside the scope of the patch but the comment as-is will cause head scratches in a years time.

Quoted 70 lines

mm/huge_memory.c +7 -7

 	if (test_bit(TRANSPARENT_HUGEPAGE_DEFRAG_DIRECT_FLAG, &transparent_hugepage_flags))
-		return GFP_TRANSHUGE | (vma_madvised ? 0 : __GFP_NORETRY);
+		return GFP_TRANSHUGE | (vma_madvised ? 0 : __GFP_NORETRY | this_node);
 	if (test_bit(TRANSPARENT_HUGEPAGE_DEFRAG_KSWAPD_FLAG, &transparent_hugepage_flags))
-		return GFP_TRANSHUGE_LIGHT | __GFP_KSWAPD_RECLAIM;
+		return GFP_TRANSHUGE_LIGHT | __GFP_KSWAPD_RECLAIM | this_node;
 	if (test_bit(TRANSPARENT_HUGEPAGE_DEFRAG_KSWAPD_OR_MADV_FLAG, &transparent_hugepage_flags))
 		return GFP_TRANSHUGE_LIGHT | (vma_madvised ? __GFP_DIRECT_RECLAIM :
-							     __GFP_KSWAPD_RECLAIM);
+							     __GFP_KSWAPD_RECLAIM | this_node);
 	if (test_bit(TRANSPARENT_HUGEPAGE_DEFRAG_REQ_MADV_FLAG, &transparent_hugepage_flags))
 		return GFP_TRANSHUGE_LIGHT | (vma_madvised ? __GFP_DIRECT_RECLAIM :
-							     0);
-	return GFP_TRANSHUGE_LIGHT;
+							     this_node);
+	return GFP_TRANSHUGE_LIGHT | this_node;
 }
 
 /* Caller must hold page table lock. */

@@ -715,7 +724,7 @@ vm_fault_t do_huge_pmd_anonymous_page(struct vm_fault *vmf)
 			pte_free(vma->vm_mm, pgtable);
 		return ret;
 	}
-	gfp = alloc_hugepage_direct_gfpmask(vma);
+	gfp = alloc_hugepage_direct_gfpmask(vma, haddr);
 	page = alloc_hugepage_vma(gfp, vma, haddr, HPAGE_PMD_ORDER);
 	if (unlikely(!page)) {
 		count_vm_event(THP_FAULT_FALLBACK);

@@ -1290,7 +1299,7 @@ vm_fault_t do_huge_pmd_wp_page(struct vm_fault *vmf, pmd_t orig_pmd)
 alloc:
 	if (transparent_hugepage_enabled(vma) &&
 	    !transparent_hugepage_debug_cow()) {
-		huge_gfp = alloc_hugepage_direct_gfpmask(vma);
+		huge_gfp = alloc_hugepage_direct_gfpmask(vma, haddr);
 		new_page = alloc_hugepage_vma(huge_gfp, vma, haddr, HPAGE_PMD_ORDER);
 	} else
 		new_page = NULL;

mm/mempolicy.c +1 -17

index da858f794eb6..75bbfc3d6233 100644
@@ -1648,7 +1648,7 @@ struct mempolicy *__get_vma_policy(struct vm_area_struct *vma,
  * freeing by another task.  It is the caller's responsibility to free the
  * extra reference for shared policies.
  */
-static struct mempolicy *get_vma_policy(struct vm_area_struct *vma,
+struct mempolicy *get_vma_policy(struct vm_area_struct *vma,
 						unsigned long addr)
 {
 	struct mempolicy *pol = __get_vma_policy(vma, addr);

@@ -2026,32 +2026,6 @@ alloc_pages_vma(gfp_t gfp, int order, struct vm_area_struct *vma,
 		goto out;
 	}
 
-	if (unlikely(IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE) && hugepage)) {
-		int hpage_node = node;
-
-		/*
-		 * For hugepage allocation and non-interleave policy which
-		 * allows the current node (or other explicitly preferred
-		 * node) we only try to allocate from the current/preferred
-		 * node and don't fall back to other nodes, as the cost of
-		 * remote accesses would likely offset THP benefits.
-		 *
-		 * If the policy is interleave, or does not allow the current
-		 * node in its nodemask, we allocate the standard way.
-		 */
-		if (pol->mode == MPOL_PREFERRED &&
-						!(pol->flags & MPOL_F_LOCAL))
-			hpage_node = pol->v.preferred_node;

mm/mempolicy.c alloc_pages_vma(gfp_t gfp, int order, struct vm_area_struct *vma, · line 2054

Quoted 10 lines

mm/mempolicy.c +0 -10

-
-		nmask = policy_nodemask(gfp, pol);
-		if (!nmask || node_isset(hpage_node, *nmask)) {
-			mpol_cond_put(pol);
-			page = __alloc_pages_node(hpage_node,
-						gfp | __GFP_THISNODE, order);
-			goto out;
-		}
-	}
-

The hugepage flag passed into this function is now redundant and that

means that callers of alloc_hugepage_vma need to move back to using alloc_pages_vma() directly and remove the API entirely. This block of code is about both GFP flag settings and node selection but at a glance I cannot see the point of it because it's very similar to the base page code. The whole point may be to get around the warning in policy_node and that could just as easily be side-stepped in alloc_hugepage_direct_gfpmask as you do already in this patch. There should be no reason why THP has a different policy than a base page within a single VMA.

[sorry I've missed your reply]

Quoted 1 line · Mel Gorman

On Wed, Aug 29, 2018 at 09:24:51PM +0200, Michal Hocko wrote:

[...]

Quoted 6 lines

I recognise that this fix means that users that expect zone_reclaim_mode==1 type behaviour may get burned but the users that benefit from that should also be users that benefit from sizing their workload to a node. They should be able to replicate that with mempolicies or at least use prepation scripts to clear memory on a target node (e.g. membind a memhog to the desired size, exit and then start the target workload).

As I've said in other email. We probably want to add a new mempolicy which has zone_reclaim_mode-like semantic.

[...]

Quoted 31 lines

Quoted 31 lines

include/linux/mempolicy.h +2 -0

index 5228c62af416..bac395f1d00a 100644
@@ -139,6 +139,8 @@ struct mempolicy *mpol_shared_policy_lookup(struct shared_policy *sp,
 struct mempolicy *get_task_policy(struct task_struct *p);
 struct mempolicy *__get_vma_policy(struct vm_area_struct *vma,
 		unsigned long addr);
+struct mempolicy *get_vma_policy(struct vm_area_struct *vma,
+						unsigned long addr);
 bool vma_policy_mof(struct vm_area_struct *vma);
 
 extern void numa_default_policy(void);

mm/huge_memory.c +7 -1

index c3bc7e9c9a2a..94472bf9a31b 100644
@@ -629,21 +629,30 @@ static vm_fault_t __do_huge_pmd_anonymous_page(struct vm_fault *vmf,
  *	    available
  * never: never stall for any thp allocation
  */
-static inline gfp_t alloc_hugepage_direct_gfpmask(struct vm_area_struct *vma)
+static inline gfp_t alloc_hugepage_direct_gfpmask(struct vm_area_struct *vma, unsigned long addr)
 {
 	const bool vma_madvised = !!(vma->vm_flags & VM_HUGEPAGE);
+	gfp_t this_node = 0;
+	struct mempolicy *pol;
+
+#ifdef CONFIG_NUMA
+	/* __GFP_THISNODE makes sense only if there is no explicit binding */
+	pol = get_vma_policy(vma, addr);

Quoted 8 lines

mm/huge_memory.c static vm_fault_t __do_huge_pmd_anonymous_page(struct vm_fault *vmf, · line 644

Quoted 4 lines

mm/huge_memory.c +3 -0

+	if (pol->mode != MPOL_BIND)
+		this_node = __GFP_THISNODE;
+#endif
 

Where is the mpol_cond_put? Historically it might not have mattered because THP could not be used with a shared possibility but it probably matters now that tmpfs can be backed by THP.

http://lkml.kernel.org/r/[email protected]

Quoted 5 lines

The comment needs more expansion as well. Arguably it only makes sense in the event we are explicitly bound to one node because if we are bound to two nodes without interleaving then why not fall back? The answer to that is outside the scope of the patch but the comment as-is will cause head scratches in a years time.

Do you have any specific wording in mind? I have a bit hard time to come up with something more precise and do not go into details too much.

Quoted 82 lines

Quoted 70 lines

mm/huge_memory.c +7 -7

 	if (test_bit(TRANSPARENT_HUGEPAGE_DEFRAG_DIRECT_FLAG, &transparent_hugepage_flags))
-		return GFP_TRANSHUGE | (vma_madvised ? 0 : __GFP_NORETRY);
+		return GFP_TRANSHUGE | (vma_madvised ? 0 : __GFP_NORETRY | this_node);
 	if (test_bit(TRANSPARENT_HUGEPAGE_DEFRAG_KSWAPD_FLAG, &transparent_hugepage_flags))
-		return GFP_TRANSHUGE_LIGHT | __GFP_KSWAPD_RECLAIM;
+		return GFP_TRANSHUGE_LIGHT | __GFP_KSWAPD_RECLAIM | this_node;
 	if (test_bit(TRANSPARENT_HUGEPAGE_DEFRAG_KSWAPD_OR_MADV_FLAG, &transparent_hugepage_flags))
 		return GFP_TRANSHUGE_LIGHT | (vma_madvised ? __GFP_DIRECT_RECLAIM :
-							     __GFP_KSWAPD_RECLAIM);
+							     __GFP_KSWAPD_RECLAIM | this_node);
 	if (test_bit(TRANSPARENT_HUGEPAGE_DEFRAG_REQ_MADV_FLAG, &transparent_hugepage_flags))
 		return GFP_TRANSHUGE_LIGHT | (vma_madvised ? __GFP_DIRECT_RECLAIM :
-							     0);
-	return GFP_TRANSHUGE_LIGHT;
+							     this_node);
+	return GFP_TRANSHUGE_LIGHT | this_node;
 }
 
 /* Caller must hold page table lock. */

@@ -715,7 +724,7 @@ vm_fault_t do_huge_pmd_anonymous_page(struct vm_fault *vmf)
 			pte_free(vma->vm_mm, pgtable);
 		return ret;
 	}
-	gfp = alloc_hugepage_direct_gfpmask(vma);
+	gfp = alloc_hugepage_direct_gfpmask(vma, haddr);
 	page = alloc_hugepage_vma(gfp, vma, haddr, HPAGE_PMD_ORDER);
 	if (unlikely(!page)) {
 		count_vm_event(THP_FAULT_FALLBACK);

@@ -1290,7 +1299,7 @@ vm_fault_t do_huge_pmd_wp_page(struct vm_fault *vmf, pmd_t orig_pmd)
 alloc:
 	if (transparent_hugepage_enabled(vma) &&
 	    !transparent_hugepage_debug_cow()) {
-		huge_gfp = alloc_hugepage_direct_gfpmask(vma);
+		huge_gfp = alloc_hugepage_direct_gfpmask(vma, haddr);
 		new_page = alloc_hugepage_vma(huge_gfp, vma, haddr, HPAGE_PMD_ORDER);
 	} else
 		new_page = NULL;

mm/mempolicy.c +1 -17

index da858f794eb6..75bbfc3d6233 100644
@@ -1648,7 +1648,7 @@ struct mempolicy *__get_vma_policy(struct vm_area_struct *vma,
  * freeing by another task.  It is the caller's responsibility to free the
  * extra reference for shared policies.
  */
-static struct mempolicy *get_vma_policy(struct vm_area_struct *vma,
+struct mempolicy *get_vma_policy(struct vm_area_struct *vma,
 						unsigned long addr)
 {
 	struct mempolicy *pol = __get_vma_policy(vma, addr);

@@ -2026,32 +2026,6 @@ alloc_pages_vma(gfp_t gfp, int order, struct vm_area_struct *vma,
 		goto out;
 	}
 
-	if (unlikely(IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE) && hugepage)) {
-		int hpage_node = node;
-
-		/*
-		 * For hugepage allocation and non-interleave policy which
-		 * allows the current node (or other explicitly preferred
-		 * node) we only try to allocate from the current/preferred
-		 * node and don't fall back to other nodes, as the cost of
-		 * remote accesses would likely offset THP benefits.
-		 *
-		 * If the policy is interleave, or does not allow the current
-		 * node in its nodemask, we allocate the standard way.
-		 */
-		if (pol->mode == MPOL_PREFERRED &&
-						!(pol->flags & MPOL_F_LOCAL))
-			hpage_node = pol->v.preferred_node;

mm/mempolicy.c alloc_pages_vma(gfp_t gfp, int order, struct vm_area_struct *vma, · line 2054

Quoted 10 lines

mm/mempolicy.c +0 -10

-
-		nmask = policy_nodemask(gfp, pol);
-		if (!nmask || node_isset(hpage_node, *nmask)) {
-			mpol_cond_put(pol);
-			page = __alloc_pages_node(hpage_node,
-						gfp | __GFP_THISNODE, order);
-			goto out;
-		}
-	}
-

The hugepage flag passed into this function is now redundant and that

Quoted 8 lines

means that callers of alloc_hugepage_vma need to move back to using alloc_pages_vma() directly and remove the API entirely. This block of code is about both GFP flag settings and node selection but at a glance I cannot see the point of it because it's very similar to the base page code. The whole point may be to get around the warning in policy_node and that could just as easily be side-stepped in alloc_hugepage_direct_gfpmask as you do already in this patch. There should be no reason why THP has a different policy than a base page within a single VMA.

OK, I can follow up with a cleanup patch once we settle down with this approach to fix the issue.

Thanks!

Hi,

i had multiple memory stalls this weekend again. All kvm processes where spinning trying to get > 100% CPU and i was not able to even login to ssh. After 5-10 minutes i was able to login.

There were about 150GB free mem on the host.

Relevant settings (no local storage involved):

        vm.dirty_background_ratio:
            3
        vm.dirty_ratio:
            10
        vm.min_free_kbytes:
            10567004

# cat /sys/kernel/mm/transparent_hugepage/defrag
always defer [defer+madvise] madvise never

# cat /sys/kernel/mm/transparent_hugepage/enabled
[always] madvise never

After that i had the following traces on the host node:
https://pastebin.com/raw/0VhyQmAv

Thanks!

Greets,
Stefan

Am 17.09.2018 um 08:11 schrieb Michal Hocko:

Quoted 164 lines

[sorry I've missed your reply]

Quoted 1 line · Mel Gorman

On Wed, Aug 29, 2018 at 09:24:51PM +0200, Michal Hocko wrote:

[...]

Quoted 6 lines

I recognise that this fix means that users that expect zone_reclaim_mode==1 type behaviour may get burned but the users that benefit from that should also be users that benefit from sizing their workload to a node. They should be able to replicate that with mempolicies or at least use prepation scripts to clear memory on a target node (e.g. membind a memhog to the desired size, exit and then start the target workload).

As I've said in other email. We probably want to add a new mempolicy which has zone_reclaim_mode-like semantic.

[...]

Quoted 31 lines

Quoted 31 lines

include/linux/mempolicy.h +2 -0

index 5228c62af416..bac395f1d00a 100644
@@ -139,6 +139,8 @@ struct mempolicy *mpol_shared_policy_lookup(struct shared_policy *sp,
 struct mempolicy *get_task_policy(struct task_struct *p);
 struct mempolicy *__get_vma_policy(struct vm_area_struct *vma,
 		unsigned long addr);
+struct mempolicy *get_vma_policy(struct vm_area_struct *vma,
+						unsigned long addr);
 bool vma_policy_mof(struct vm_area_struct *vma);
 
 extern void numa_default_policy(void);

mm/huge_memory.c +7 -1

index c3bc7e9c9a2a..94472bf9a31b 100644
@@ -629,21 +629,30 @@ static vm_fault_t __do_huge_pmd_anonymous_page(struct vm_fault *vmf,
  *	    available
  * never: never stall for any thp allocation
  */
-static inline gfp_t alloc_hugepage_direct_gfpmask(struct vm_area_struct *vma)
+static inline gfp_t alloc_hugepage_direct_gfpmask(struct vm_area_struct *vma, unsigned long addr)
 {
 	const bool vma_madvised = !!(vma->vm_flags & VM_HUGEPAGE);
+	gfp_t this_node = 0;
+	struct mempolicy *pol;
+
+#ifdef CONFIG_NUMA
+	/* __GFP_THISNODE makes sense only if there is no explicit binding */
+	pol = get_vma_policy(vma, addr);

Quoted 8 lines

mm/huge_memory.c static vm_fault_t __do_huge_pmd_anonymous_page(struct vm_fault *vmf, · line 644

Quoted 4 lines

mm/huge_memory.c +3 -0

+	if (pol->mode != MPOL_BIND)
+		this_node = __GFP_THISNODE;
+#endif
 

Where is the mpol_cond_put? Historically it might not have mattered because THP could not be used with a shared possibility but it probably matters now that tmpfs can be backed by THP.

http://lkml.kernel.org/r/[email protected]

Quoted 5 lines

The comment needs more expansion as well. Arguably it only makes sense in the event we are explicitly bound to one node because if we are bound to two nodes without interleaving then why not fall back? The answer to that is outside the scope of the patch but the comment as-is will cause head scratches in a years time.

Do you have any specific wording in mind? I have a bit hard time to come up with something more precise and do not go into details too much.

Quoted 82 lines

Quoted 70 lines

mm/huge_memory.c +7 -7

 	if (test_bit(TRANSPARENT_HUGEPAGE_DEFRAG_DIRECT_FLAG, &transparent_hugepage_flags))
-		return GFP_TRANSHUGE | (vma_madvised ? 0 : __GFP_NORETRY);
+		return GFP_TRANSHUGE | (vma_madvised ? 0 : __GFP_NORETRY | this_node);
 	if (test_bit(TRANSPARENT_HUGEPAGE_DEFRAG_KSWAPD_FLAG, &transparent_hugepage_flags))
-		return GFP_TRANSHUGE_LIGHT | __GFP_KSWAPD_RECLAIM;
+		return GFP_TRANSHUGE_LIGHT | __GFP_KSWAPD_RECLAIM | this_node;
 	if (test_bit(TRANSPARENT_HUGEPAGE_DEFRAG_KSWAPD_OR_MADV_FLAG, &transparent_hugepage_flags))
 		return GFP_TRANSHUGE_LIGHT | (vma_madvised ? __GFP_DIRECT_RECLAIM :
-							     __GFP_KSWAPD_RECLAIM);
+							     __GFP_KSWAPD_RECLAIM | this_node);
 	if (test_bit(TRANSPARENT_HUGEPAGE_DEFRAG_REQ_MADV_FLAG, &transparent_hugepage_flags))
 		return GFP_TRANSHUGE_LIGHT | (vma_madvised ? __GFP_DIRECT_RECLAIM :
-							     0);
-	return GFP_TRANSHUGE_LIGHT;
+							     this_node);
+	return GFP_TRANSHUGE_LIGHT | this_node;
 }
 
 /* Caller must hold page table lock. */

@@ -715,7 +724,7 @@ vm_fault_t do_huge_pmd_anonymous_page(struct vm_fault *vmf)
 			pte_free(vma->vm_mm, pgtable);
 		return ret;
 	}
-	gfp = alloc_hugepage_direct_gfpmask(vma);
+	gfp = alloc_hugepage_direct_gfpmask(vma, haddr);
 	page = alloc_hugepage_vma(gfp, vma, haddr, HPAGE_PMD_ORDER);
 	if (unlikely(!page)) {
 		count_vm_event(THP_FAULT_FALLBACK);

@@ -1290,7 +1299,7 @@ vm_fault_t do_huge_pmd_wp_page(struct vm_fault *vmf, pmd_t orig_pmd)
 alloc:
 	if (transparent_hugepage_enabled(vma) &&
 	    !transparent_hugepage_debug_cow()) {
-		huge_gfp = alloc_hugepage_direct_gfpmask(vma);
+		huge_gfp = alloc_hugepage_direct_gfpmask(vma, haddr);
 		new_page = alloc_hugepage_vma(huge_gfp, vma, haddr, HPAGE_PMD_ORDER);
 	} else
 		new_page = NULL;

mm/mempolicy.c +1 -17

index da858f794eb6..75bbfc3d6233 100644
@@ -1648,7 +1648,7 @@ struct mempolicy *__get_vma_policy(struct vm_area_struct *vma,
  * freeing by another task.  It is the caller's responsibility to free the
  * extra reference for shared policies.
  */
-static struct mempolicy *get_vma_policy(struct vm_area_struct *vma,
+struct mempolicy *get_vma_policy(struct vm_area_struct *vma,
 						unsigned long addr)
 {
 	struct mempolicy *pol = __get_vma_policy(vma, addr);

@@ -2026,32 +2026,6 @@ alloc_pages_vma(gfp_t gfp, int order, struct vm_area_struct *vma,
 		goto out;
 	}
 
-	if (unlikely(IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE) && hugepage)) {
-		int hpage_node = node;
-
-		/*
-		 * For hugepage allocation and non-interleave policy which
-		 * allows the current node (or other explicitly preferred
-		 * node) we only try to allocate from the current/preferred
-		 * node and don't fall back to other nodes, as the cost of
-		 * remote accesses would likely offset THP benefits.
-		 *
-		 * If the policy is interleave, or does not allow the current
-		 * node in its nodemask, we allocate the standard way.
-		 */
-		if (pol->mode == MPOL_PREFERRED &&
-						!(pol->flags & MPOL_F_LOCAL))
-			hpage_node = pol->v.preferred_node;

mm/mempolicy.c alloc_pages_vma(gfp_t gfp, int order, struct vm_area_struct *vma, · line 2054

Quoted 10 lines

mm/mempolicy.c +0 -10

-
-		nmask = policy_nodemask(gfp, pol);
-		if (!nmask || node_isset(hpage_node, *nmask)) {
-			mpol_cond_put(pol);
-			page = __alloc_pages_node(hpage_node,
-						gfp | __GFP_THISNODE, order);
-			goto out;
-		}
-	}
-

The hugepage flag passed into this function is now redundant and that

Quoted 8 lines

means that callers of alloc_hugepage_vma need to move back to using alloc_pages_vma() directly and remove the API entirely. This block of code is about both GFP flag settings and node selection but at a glance I cannot see the point of it because it's very similar to the base page code. The whole point may be to get around the warning in policy_node and that could just as easily be side-stepped in alloc_hugepage_direct_gfpmask as you do already in this patch. There should be no reason why THP has a different policy than a base page within a single VMA.

OK, I can follow up with a cleanup patch once we settle down with this approach to fix the issue.

Thanks!

Hi,

i had multiple memory stalls this weekend again. All kvm processes where spinning trying to get > 100% CPU and i was not able to even login to ssh. After 5-10 minutes i was able to login.

There were about 150GB free mem on the host.

Relevant settings (no local storage involved):

        vm.dirty_background_ratio:
            3
        vm.dirty_ratio:
            10
        vm.min_free_kbytes:
            10567004

# cat /sys/kernel/mm/transparent_hugepage/defrag
always defer [defer+madvise] madvise never

# cat /sys/kernel/mm/transparent_hugepage/enabled
[always] madvise never

After that i had the following traces on the host node:
https://pastebin.com/raw/0VhyQmAv

Hello,

we detected a regression compared to older kernels, only happening with defrag=always or by using MADV_HUGEPAGE (and QEMU uses it).

I haven't bisected but I suppose this started since commit 5265047ac30191ea24b16503165000c225f54feb combined with previous commits that introduced the logic to not try to invoke reclaim for THP allocations in the remote nodes.

Once I looked into it the problem was pretty obvious and there are two possible simple fixes, one is not to invoke reclaim and stick to compaction in the local node only (still __GFP_THISNODE model).

This approach keeps the logic the same and prioritizes for NUMA locality over THP generation.

Then I'll send the an alternative that drops the __GFP_THISNODE logic if_DIRECT_RECLAIM is set. That however changes the behavior for MADV_HUGEPAGE and prioritizes THP generation over NUMA locality.

A possible incremental improvement for this __GFP_COMPACT_ONLY solution would be to remove __GFP_THISNODE (and in turn __GFP_COMPACT_ONLY) after checking the watermarks if there's no free PAGE_SIZEd memory in the local node. However checking the watermarks in mempolicy.c is not ideal so it would be a more messy change and it'd still need to use __GFP_COMPACT_ONLY as implemented here for when there's no PAGE_SIZEd free memory in the local node. That further improvement wouldn't be necessary if there's agreement to prioritize THP generation over NUMA locality (the alternative solution I'll send in a separate post).

I personally prefer to prioritize NUMA locality over THP (__GFP_COMPACT_ONLY variant), but I don't know page-alloc/compaction good enough to Ack it.

Hi Kirill,

Quoted 3 lines · Kirill A. Shutemov

I personally prefer to prioritize NUMA locality over THP (__GFP_COMPACT_ONLY variant), but I don't know page-alloc/compaction good enough to Ack it.

If we go in this direction it'd be nice after fixing the showstopper bug, if we could then proceed with an orthogonal optimization by checking the watermarks and if the watermarks shows there are no PAGE_SIZEd pages available in the local node we should remove both __GFP_THISNODE and __GFP_COMPACT_ONLY.

If as opposed there's still PAGE_SIZEd free memory in the local node (not possible to compact for whatever reason), we should stick to __GFP_THISNODE | __GFP_COMPACT_ONLY.

It's orthogonal because the above addition would make sense also in the current (buggy) code.

The main implementation issue is that the watermark checking is not well done in mempolicy.c but the place to clear __GFP_THISNODE and __GFP_COMPACT_ONLY currently is there.

The case that the local node gets completely full and has not even 4k pages available should be totally common, because if you keep allocating and you allocate more than the size of a NUMA node eventually you will fill the local node with THP then consume all 4k pages and then you get into the case where the current code is totally unable to allocate THP from the other nodes and it would be totally possible to fix with the removal of __GFP_THISNODE | __GFP_COMPACT_ONLY, after the PAGE_SIZE watermark check.

I'm mentioning this optimization in this context, even if it's orthogonal, because the alternative patch that prioritizes THP over NUMA locality for MADV_HUGEPAGE and defer=always would solve that too with a one liner and there would be no need of watermark checking and flipping gfp bits whatsoever. Once the local node is full, THPs keeps being provided as expected.

Thanks,
Andrea

Frankly, I would rather go with this option and assume that if someone explicitly wants THP's, they don't care about NUMA locality that much. (Note: I hate __GFP_THISNODE, it's an endless source of issues.) Trying to be clever about "is there still PAGE_SIZEd free memory in the local node" is imperfect anyway. If there isn't, is it because there's clean page cache that we can easily reclaim (so it would be worth staying local) or is it really exhausted? Watermark check won't tell...

MADV_HUGEPAGE (or defrag == "always") would now become a combination of "try to compact locally" and "allocate remotely if necesary" without the ability to avoid the latter absent a mempolicy that affects all memory

allocations. I think the complete solution would be a MPOL_F_HUGEPAGE flag that defines mempolicies for hugepage allocations. In my experience thp falling back to remote nodes for intrasocket latency is a win but intersocket or two-hop intersocket latency is a no go.

If it's "not possible to compact" then the expected outcome of this is to fail?

You could do that without calling watermark checking explicitly, but it's rather complicated:

1. try alocating with __GFP_THISNODE and ~GFP_DIRECT_RECLAIM
2. if that fails, try PAGE_SIZE with same flags
3. if that fails, try THP size without __GFP_THISNODE
4. PAGE_SIZE without __GFP_THISNODE

Yeah, not possible in current alloc_pages_vma() which should return the requested order. But the advantage is that it's not prone to races between watermark checking and actual attempt.

Frankly, I would rather go with this option and assume that if someone explicitly wants THP's, they don't care about NUMA locality that much.

(Note: I hate __GFP_THISNODE, it's an endless source of issues.) Trying to be clever about "is there still PAGE_SIZEd free memory in the local node" is imperfect anyway. If there isn't, is it because there's clean page cache that we can easily reclaim (so it would be worth staying local) or is it really exhausted? Watermark check won't tell...

Frankly, I would rather go with this option and assume that if someone explicitly wants THP's, they don't care about NUMA locality that much.

(Note: I hate __GFP_THISNODE, it's an endless source of issues.)

Trying to be clever about "is there still PAGE_SIZEd free memory in the local node" is imperfect anyway. If there isn't, is it because there's clean page cache that we can easily reclaim (so it would be worth staying local) or is it really exhausted? Watermark check won't tell...

abuse elsewhere. Like here for the THP. I am pretty sure that the intention was not to stick to a specific node but rather all local nodes within the reclaim distance (or other unit to define that nodes are sufficiently close).

Quoted 21 lines · Andrea Arcangeli

Hello,

we detected a regression compared to older kernels, only happening with defrag=always or by using MADV_HUGEPAGE (and QEMU uses it).

I haven't bisected but I suppose this started since commit 5265047ac30191ea24b16503165000c225f54feb combined with previous commits that introduced the logic to not try to invoke reclaim for THP allocations in the remote nodes.

Once I looked into it the problem was pretty obvious and there are two possible simple fixes, one is not to invoke reclaim and stick to compaction in the local node only (still __GFP_THISNODE model).

This approach keeps the logic the same and prioritizes for NUMA locality over THP generation.

Then I'll send the an alternative that drops the __GFP_THISNODE logic if_DIRECT_RECLAIM is set. That however changes the behavior for MADV_HUGEPAGE and prioritizes THP generation over NUMA locality.

Quoted 10 lines

A possible incremental improvement for this __GFP_COMPACT_ONLY solution would be to remove __GFP_THISNODE (and in turn __GFP_COMPACT_ONLY) after checking the watermarks if there's no free PAGE_SIZEd memory in the local node. However checking the watermarks in mempolicy.c is not ideal so it would be a more messy change and it'd still need to use __GFP_COMPACT_ONLY as implemented here for when there's no PAGE_SIZEd free memory in the local node. That further improvement wouldn't be necessary if there's agreement to prioritize THP generation over NUMA locality (the alternative solution I'll send in a separate post).

I personally prefer to prioritize NUMA locality over THP (__GFP_COMPACT_ONLY variant), but I don't know page-alloc/compaction good enough to Ack it.

--
Kirill A. Shutemov

May the approach #1 break the setting of zone_reclaim_mode? Or it may behave like zone_reclaim_mode is set even though the knob is cleared?