kvm: Report unused guest pages to host

From: Alexander Duyck <[email protected]>

Add the host side of the KVM memory hinting support. With this we expose a feature bit indicating that the host will pass the messages along to the new madvise function.

This functionality is mutually exclusive with device assignment. If a device is assigned we will disable the functionality as it could lead to a potential memory corruption if a device writes to a page after KVM has flagged it as not being used.

The logic as it is currently defined limits the hint to only supporting a hugepage or larger notifications. This is meant to help prevent us from potentially breaking up huge pages by hinting that only a portion of the page is not needed.

Signed-off-by Alexander Duyck <[email protected]>

---
 Documentation/virtual/kvm/cpuid.txt      |    4 +++
 Documentation/virtual/kvm/hypercalls.txt |   14 ++++++++++++
 arch/x86/include/uapi/asm/kvm_para.h     |    3 +++
 arch/x86/kvm/cpuid.c                     |    6 ++++-
 arch/x86/kvm/x86.c                       |   35 ++++++++++++++++++++++++++++++
 include/uapi/linux/kvm_para.h            |    1 +
 6 files changed, 62 insertions(+), 1 deletion(-)

Documentation/virtual/kvm/cpuid.txt +4 -0

index 97ca1940a0dc..fe3395a58b7e 100644
@@ -66,6 +66,10 @@ KVM_FEATURE_PV_SEND_IPI            ||    11 || guest checks this feature bit
                                    ||       || before using paravirtualized
                                    ||       || send IPIs.
 ------------------------------------------------------------------------------
+KVM_FEATURE_PV_UNUSED_PAGE_HINT    ||    12 || guest checks this feature bit
+                                   ||       || before using paravirtualized
+                                   ||       || unused page hints.
+------------------------------------------------------------------------------
 KVM_FEATURE_CLOCKSOURCE_STABLE_BIT ||    24 || host will warn if no guest-side
                                    ||       || per-cpu warps are expected in
                                    ||       || kvmclock.

Documentation/virtual/kvm/hypercalls.txt +14 -0

index da24c138c8d1..b374678ac1f9 100644
@@ -141,3 +141,17 @@ a0 corresponds to the APIC ID in the third argument (a2), bit 1
 corresponds to the APIC ID a2+1, and so on.
 
 Returns the number of CPUs to which the IPIs were delivered successfully.
+
+7. KVM_HC_UNUSED_PAGE_HINT
+------------------------
+Architecture: x86
+Status: active
+Purpose: Send unused page hint to host
+
+a0: physical address of region unused, page aligned
+a1: size of unused region, page aligned
+
+The hypercall lets a guest send notifications to the host that it will no
+longer be using a given page in memory. Multiple pages can be hinted at by
+using the size field to hint that a higher order page is available by
+specifying the higher order page size.

arch/x86/include/uapi/asm/kvm_para.h +3 -0

index 19980ec1a316..f066c23060df 100644
@@ -29,6 +29,7 @@
 #define KVM_FEATURE_PV_TLB_FLUSH	9
 #define KVM_FEATURE_ASYNC_PF_VMEXIT	10
 #define KVM_FEATURE_PV_SEND_IPI	11
+#define KVM_FEATURE_PV_UNUSED_PAGE_HINT	12
 
 #define KVM_HINTS_REALTIME      0
 

@@ -119,4 +120,6 @@ struct kvm_vcpu_pv_apf_data {
 #define KVM_PV_EOI_ENABLED KVM_PV_EOI_MASK
 #define KVM_PV_EOI_DISABLED 0x0
 
+#define KVM_PV_UNUSED_PAGE_HINT_MIN_ORDER	HUGETLB_PAGE_ORDER
+
 #endif /* _UAPI_ASM_X86_KVM_PARA_H */

arch/x86/kvm/cpuid.c +5 -1

index bbffa6c54697..b82bcbfbc420 100644
@@ -136,6 +136,9 @@ int kvm_update_cpuid(struct kvm_vcpu *vcpu)
 	if (kvm_hlt_in_guest(vcpu->kvm) && best &&
 		(best->eax & (1 << KVM_FEATURE_PV_UNHALT)))
 		best->eax &= ~(1 << KVM_FEATURE_PV_UNHALT);
+	if (kvm_arch_has_assigned_device(vcpu->kvm) && best &&
+		(best->eax & KVM_FEATURE_PV_UNUSED_PAGE_HINT))
+		best->eax &= ~(1 << KVM_FEATURE_PV_UNUSED_PAGE_HINT);
 
 	/* Update physical-address width */
 	vcpu->arch.maxphyaddr = cpuid_query_maxphyaddr(vcpu);

@@ -637,7 +640,8 @@ static inline int __do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
 			     (1 << KVM_FEATURE_PV_UNHALT) |
 			     (1 << KVM_FEATURE_PV_TLB_FLUSH) |
 			     (1 << KVM_FEATURE_ASYNC_PF_VMEXIT) |
-			     (1 << KVM_FEATURE_PV_SEND_IPI);
+			     (1 << KVM_FEATURE_PV_SEND_IPI) |
+			     (1 << KVM_FEATURE_PV_UNUSED_PAGE_HINT);
 
 		if (sched_info_on())
 			entry->eax |= (1 << KVM_FEATURE_STEAL_TIME);

arch/x86/kvm/x86.c +35 -0

index 3d27206f6c01..3ec75ab849e2 100644
@@ -55,6 +55,7 @@
 #include <linux/irqbypass.h>
 #include <linux/sched/stat.h>
 #include <linux/mem_encrypt.h>
+#include <linux/mm.h>
 
 #include <trace/events/kvm.h>
 

@@ -7052,6 +7053,37 @@ void kvm_vcpu_deactivate_apicv(struct kvm_vcpu *vcpu)
 	kvm_x86_ops->refresh_apicv_exec_ctrl(vcpu);
 }
 
+static int kvm_pv_unused_page_hint_op(struct kvm *kvm, gpa_t gpa, size_t len)
+{
+	unsigned long start;
+
+	/*
+	 * Guarantee the following:
+	 *	len meets minimum size
+	 *	len is a power of 2
+	 *	gpa is aligned to len
+	 */
+	if (len < (PAGE_SIZE << KVM_PV_UNUSED_PAGE_HINT_MIN_ORDER))
+		return -KVM_EINVAL;
+	if (!is_power_of_2(len) || !IS_ALIGNED(gpa, len))
+		return -KVM_EINVAL;
+
+	/*
+	 * If a device is assigned we cannot use use madvise as memory
+	 * is shared with the device and could lead to memory corruption
+	 * if the device writes to it after free.
+	 */
+	if (kvm_arch_has_assigned_device(kvm))
+		return -KVM_EOPNOTSUPP;
+
+	start = gfn_to_hva(kvm, gpa_to_gfn(gpa));
+
+	if (kvm_is_error_hva(start + len))
+		return -KVM_EFAULT;
+
+	return do_madvise_dontneed(start, len);
+}
+
 int kvm_emulate_hypercall(struct kvm_vcpu *vcpu)
 {
 	unsigned long nr, a0, a1, a2, a3, ret;

@@ -7098,6 +7130,9 @@ int kvm_emulate_hypercall(struct kvm_vcpu *vcpu)
 	case KVM_HC_SEND_IPI:
 		ret = kvm_pv_send_ipi(vcpu->kvm, a0, a1, a2, a3, op_64_bit);
 		break;
+	case KVM_HC_UNUSED_PAGE_HINT:
+		ret = kvm_pv_unused_page_hint_op(vcpu->kvm, a0, a1);
+		break;
 	default:
 		ret = -KVM_ENOSYS;
 		break;

include/uapi/linux/kvm_para.h +1 -0

index 6c0ce49931e5..75643b862a4e 100644
@@ -28,6 +28,7 @@
 #define KVM_HC_MIPS_CONSOLE_OUTPUT	8
 #define KVM_HC_CLOCK_PAIRING		9
 #define KVM_HC_SEND_IPI		10
+#define KVM_HC_UNUSED_PAGE_HINT		11
 
 /*
  * hypercalls use architecture specific

Quoted 10 lines · Alexander Duyck

From: Alexander Duyck <[email protected]>

Add the host side of the KVM memory hinting support. With this we expose a feature bit indicating that the host will pass the messages along to the new madvise function.

This functionality is mutually exclusive with device assignment. If a device is assigned we will disable the functionality as it could lead to a potential memory corruption if a device writes to a page after KVM has flagged it as not being used.

I really dislike this kind of tie-in.

Yes right now assignment is not smart enough but generally you can protect the unused page in the IOMMU and that's it, it's safe.

So the policy should not leak into host/guest interface. Instead it is better to just keep the pages pinned and ignore the hint for now.

Quoted 18 lines · Michael S. Tsirkin

Quoted 10 lines · Alexander Duyck

From: Alexander Duyck <[email protected]>

Add the host side of the KVM memory hinting support. With this we expose a feature bit indicating that the host will pass the messages along to the new madvise function.

This functionality is mutually exclusive with device assignment. If a device is assigned we will disable the functionality as it could lead to a potential memory corruption if a device writes to a page after KVM has flagged it as not being used.

I really dislike this kind of tie-in.

Yes right now assignment is not smart enough but generally you can protect the unused page in the IOMMU and that's it, it's safe.

Quoted 3 lines

So the policy should not leak into host/guest interface. Instead it is better to just keep the pages pinned and ignore the hint for now.

Okay, I can do that. It also gives me a means of benchmarking just the hypercall cost versus the extra page faults and zeroing.

So the policy should not leak into host/guest interface. Instead it is better to just keep the pages pinned and ignore the hint for now.

Quoted 3 lines · Michael S. Tsirkin

So the policy should not leak into host/guest interface. Instead it is better to just keep the pages pinned and ignore the hint for now.

It does seems a bit silly to have guests forever hinting about freed memory when the host never has a hope of doing anything about it.

Is that part fixable?

Quoted 9 lines · Dave Hansen

Quoted 3 lines · Michael S. Tsirkin

So the policy should not leak into host/guest interface. Instead it is better to just keep the pages pinned and ignore the hint for now.

It does seems a bit silly to have guests forever hinting about freed memory when the host never has a hope of doing anything about it.

Is that part fixable?

Yes just not with existing IOMMU APIs.

It's in the paragraph just above that you cut out:
Yes right now assignment is not smart enough but generally you can protect the unused page in the IOMMU and that's it, it's safe.

So e.g.

	extern int iommu_remap(struct iommu_domain *domain, unsigned long iova,
				     phys_addr_t paddr, size_t size, int prot);

I can elaborate if you like but generally we would need an API that allows you to atomically update a mapping for a specific page without perturbing the mapping for other pages.

Quoted 19 lines · Michael S. Tsirkin

Quoted 9 lines · Dave Hansen

Quoted 3 lines · Michael S. Tsirkin

So the policy should not leak into host/guest interface. Instead it is better to just keep the pages pinned and ignore the hint for now.

It does seems a bit silly to have guests forever hinting about freed memory when the host never has a hope of doing anything about it.

Is that part fixable?

Yes just not with existing IOMMU APIs.

It's in the paragraph just above that you cut out:
Yes right now assignment is not smart enough but generally you can protect the unused page in the IOMMU and that's it, it's safe.

Quoted 9 lines

So e.g.

	extern int iommu_remap(struct iommu_domain *domain, unsigned long iova,
				     phys_addr_t paddr, size_t size, int prot);

I can elaborate if you like but generally we would need an API that allows you to atomically update a mapping for a specific page without perturbing the mapping for other pages.

I still don't see how this would solve anything unless you have the guest somehow hinting on what pages it is providing to the devices.

You would have to have the host invalidating the pages when the hint is provided, and have a new hint tied to arch_alloc_page that would rebuild the IOMMU mapping when a page is allocated.

I'm pretty certain that the added cost of that would make the hinting pretty pointless as my experience has been that the IOMMU is too much of a bottleneck to have multiple CPUs trying to create and invalidate mappings simultaneously.

arch/x86/include/asm/page.h line 32

Quoted 12 lines · Alexander Duyck

arch/x86/include/asm/page.h +12 -0

+#ifdef CONFIG_KVM_GUEST
+#include <linux/jump_label.h>
+extern struct static_key_false pv_free_page_hint_enabled;
+
+#define HAVE_ARCH_FREE_PAGE
+void __arch_free_page(struct page *page, unsigned int order);
+static inline void arch_free_page(struct page *page, unsigned int order)
+{
+	if (static_branch_unlikely(&pv_free_page_hint_enabled))
+		__arch_free_page(page, order);
+}
+#endif

So, this ends up with at least a call, a branch and a ret added to the order-0 paths, including freeing pages to the per-cpu-pageset lists. That seems worrisome.

What performance testing has been performed to look into the overhead added to those paths?

From: Alexander Duyck <[email protected]>

Add guest support for providing free memory hints to the KVM hypervisor for freed pages huge TLB size or larger. I am restricting the size to huge TLB order and larger because the hypercalls are too expensive to be performing one per 4K page. Using the huge TLB order became the obvious choice for the order to use as it allows us to avoid fragmentation of higher order memory on the host.

I have limited the functionality so that it doesn't work when page poisoning is enabled. I did this because a write to the page after doing an MADV_DONTNEED would effectively negate the hint, so it would be wasting cycles to do so.

Signed-off-by Alexander Duyck <[email protected]>

---

arch/x86/include/asm/page.h |   13 +++++++++++++
arch/x86/kernel/kvm.c       |   23 +++++++++++++++++++++++
2 files changed, 36 insertions(+)

arch/x86/include/asm/page.h +2 -0

index 7555b48803a8..4487ad7a3385 100644
@@ -18,6 +18,19 @@
 
 struct page;
 
+#ifdef CONFIG_KVM_GUEST
+#include <linux/jump_label.h>

Quoted 32 lines · Alexander Duyck

From: Alexander Duyck <[email protected]>

Add guest support for providing free memory hints to the KVM hypervisor for freed pages huge TLB size or larger. I am restricting the size to huge TLB order and larger because the hypercalls are too expensive to be performing one per 4K page. Using the huge TLB order became the obvious choice for the order to use as it allows us to avoid fragmentation of higher order memory on the host.

I have limited the functionality so that it doesn't work when page poisoning is enabled. I did this because a write to the page after doing an MADV_DONTNEED would effectively negate the hint, so it would be wasting cycles to do so.

Signed-off-by Alexander Duyck <[email protected]>

---

arch/x86/include/asm/page.h |   13 +++++++++++++
arch/x86/kernel/kvm.c       |   23 +++++++++++++++++++++++
2 files changed, 36 insertions(+)

arch/x86/include/asm/page.h +2 -0

index 7555b48803a8..4487ad7a3385 100644
@@ -18,6 +18,19 @@
 
 struct page;
 
+#ifdef CONFIG_KVM_GUEST
+#include <linux/jump_label.h>

arch/x86/include/asm/page.h line 32

Quoted 10 lines

arch/x86/include/asm/page.h +10 -0

+extern struct static_key_false pv_free_page_hint_enabled;
+
+#define HAVE_ARCH_FREE_PAGE
+void __arch_free_page(struct page *page, unsigned int order);
+static inline void arch_free_page(struct page *page, unsigned int order)
+{
+	if (static_branch_unlikely(&pv_free_page_hint_enabled))
+		__arch_free_page(page, order);
+}
+#endif

This patch and the following one assume that only KVM should be able to hook to these events. I do not think it is appropriate for __arch_free_page() to effectively mean “kvm_guest_free_page()”.

Is it possible to use the paravirt infrastructure for this feature, similarly to other PV features? It is not the best infrastructure, but at least it is hypervisor-neutral.

Quoted 47 lines · Nadav Amit

Quoted 32 lines · Alexander Duyck

From: Alexander Duyck <[email protected]>

Add guest support for providing free memory hints to the KVM hypervisor for freed pages huge TLB size or larger. I am restricting the size to huge TLB order and larger because the hypercalls are too expensive to be performing one per 4K page. Using the huge TLB order became the obvious choice for the order to use as it allows us to avoid fragmentation of higher order memory on the host.

I have limited the functionality so that it doesn't work when page poisoning is enabled. I did this because a write to the page after doing an MADV_DONTNEED would effectively negate the hint, so it would be wasting cycles to do so.

Signed-off-by Alexander Duyck <[email protected]>

---

arch/x86/include/asm/page.h |   13 +++++++++++++
arch/x86/kernel/kvm.c       |   23 +++++++++++++++++++++++
2 files changed, 36 insertions(+)

arch/x86/include/asm/page.h +2 -0

index 7555b48803a8..4487ad7a3385 100644
@@ -18,6 +18,19 @@
 
 struct page;
 
+#ifdef CONFIG_KVM_GUEST
+#include <linux/jump_label.h>

arch/x86/include/asm/page.h line 32

Quoted 10 lines

arch/x86/include/asm/page.h +10 -0

+extern struct static_key_false pv_free_page_hint_enabled;
+
+#define HAVE_ARCH_FREE_PAGE
+void __arch_free_page(struct page *page, unsigned int order);
+static inline void arch_free_page(struct page *page, unsigned int order)
+{
+	if (static_branch_unlikely(&pv_free_page_hint_enabled))
+		__arch_free_page(page, order);
+}
+#endif

This patch and the following one assume that only KVM should be able to hook to these events. I do not think it is appropriate for __arch_free_page() to effectively mean “kvm_guest_free_page()”.

Quoted 3 lines

Is it possible to use the paravirt infrastructure for this feature, similarly to other PV features? It is not the best infrastructure, but at least it is hypervisor-neutral.

I could probably tie this into the paravirt infrastructure, but if I did so I would probably want to pull the checks for the page order out of the KVM specific bits and make it something we handle in the inline. Doing that I would probably make it a paravirtual hint that only operates at the PMD level. That way we wouldn't incur the cost of the paravirt infrastructure at the per 4K page level.

Quoted 55 lines · Alexander Duyck

Quoted 47 lines · Nadav Amit

Quoted 32 lines · Alexander Duyck

From: Alexander Duyck <[email protected]>

Add guest support for providing free memory hints to the KVM hypervisor for freed pages huge TLB size or larger. I am restricting the size to huge TLB order and larger because the hypercalls are too expensive to be performing one per 4K page. Using the huge TLB order became the obvious choice for the order to use as it allows us to avoid fragmentation of higher order memory on the host.

I have limited the functionality so that it doesn't work when page poisoning is enabled. I did this because a write to the page after doing an MADV_DONTNEED would effectively negate the hint, so it would be wasting cycles to do so.

Signed-off-by Alexander Duyck <[email protected]>

---

arch/x86/include/asm/page.h |   13 +++++++++++++
arch/x86/kernel/kvm.c       |   23 +++++++++++++++++++++++
2 files changed, 36 insertions(+)

arch/x86/include/asm/page.h +2 -0

index 7555b48803a8..4487ad7a3385 100644
@@ -18,6 +18,19 @@
 
 struct page;
 
+#ifdef CONFIG_KVM_GUEST
+#include <linux/jump_label.h>

arch/x86/include/asm/page.h line 32

Quoted 10 lines

arch/x86/include/asm/page.h +10 -0

+extern struct static_key_false pv_free_page_hint_enabled;
+
+#define HAVE_ARCH_FREE_PAGE
+void __arch_free_page(struct page *page, unsigned int order);
+static inline void arch_free_page(struct page *page, unsigned int order)
+{
+   if (static_branch_unlikely(&pv_free_page_hint_enabled))
+           __arch_free_page(page, order);
+}
+#endif

This patch and the following one assume that only KVM should be able to hook to these events. I do not think it is appropriate for __arch_free_page() to effectively mean “kvm_guest_free_page()”.

Quoted 3 lines

Is it possible to use the paravirt infrastructure for this feature, similarly to other PV features? It is not the best infrastructure, but at least it is hypervisor-neutral.

Quoted 6 lines

I could probably tie this into the paravirt infrastructure, but if I did so I would probably want to pull the checks for the page order out of the KVM specific bits and make it something we handle in the inline. Doing that I would probably make it a paravirtual hint that only operates at the PMD level. That way we wouldn't incur the cost of the paravirt infrastructure at the per 4K page level.

If I understand you correctly, you “complain” that this would affect performance.

While it might be, you may want to check whether the already available tools can solve the problem:

1. You can use a combination of static-key and pv-ops - see for example steal_account_process_time()

2. You can use callee-saved pv-ops.

The latter might anyhow be necessary since, IIUC, you change a very hot path. So you may want have a look on the assembly code of free_pcp_prepare() (or at least its code-size) before and after your changes. If they are too big, a callee-saved function might be necessary.

Quoted 61 lines · Nadav Amit

Quoted 54 lines · Alexander Duyck

Quoted 47 lines · Nadav Amit

Quoted 32 lines · Alexander Duyck

From: Alexander Duyck <[email protected]>

Add guest support for providing free memory hints to the KVM hypervisor for freed pages huge TLB size or larger. I am restricting the size to huge TLB order and larger because the hypercalls are too expensive to be performing one per 4K page. Using the huge TLB order became the obvious choice for the order to use as it allows us to avoid fragmentation of higher order memory on the host.

I have limited the functionality so that it doesn't work when page poisoning is enabled. I did this because a write to the page after doing an MADV_DONTNEED would effectively negate the hint, so it would be wasting cycles to do so.

Signed-off-by Alexander Duyck <[email protected]>

---

arch/x86/include/asm/page.h |   13 +++++++++++++
arch/x86/kernel/kvm.c       |   23 +++++++++++++++++++++++
2 files changed, 36 insertions(+)

arch/x86/include/asm/page.h +2 -0

index 7555b48803a8..4487ad7a3385 100644
@@ -18,6 +18,19 @@
 
 struct page;
 
+#ifdef CONFIG_KVM_GUEST
+#include <linux/jump_label.h>

arch/x86/include/asm/page.h line 32

Quoted 10 lines

arch/x86/include/asm/page.h +10 -0

+extern struct static_key_false pv_free_page_hint_enabled;
+
+#define HAVE_ARCH_FREE_PAGE
+void __arch_free_page(struct page *page, unsigned int order);
+static inline void arch_free_page(struct page *page, unsigned int order)
+{
+   if (static_branch_unlikely(&pv_free_page_hint_enabled))
+           __arch_free_page(page, order);
+}
+#endif

This patch and the following one assume that only KVM should be able to hook to these events. I do not think it is appropriate for __arch_free_page() to effectively mean “kvm_guest_free_page()”.

Quoted 3 lines

Is it possible to use the paravirt infrastructure for this feature, similarly to other PV features? It is not the best infrastructure, but at least it is hypervisor-neutral.

Quoted 6 lines

I could probably tie this into the paravirt infrastructure, but if I did so I would probably want to pull the checks for the page order out of the KVM specific bits and make it something we handle in the inline. Doing that I would probably make it a paravirtual hint that only operates at the PMD level. That way we wouldn't incur the cost of the paravirt infrastructure at the per 4K page level.

Quoted 2 lines

If I understand you correctly, you “complain” that this would affect performance.

It wasn't so much a "complaint" as an "observation". What I was getting at is that if I am going to make it a PV operation I might set a hard limit on it so that it will specifically only apply to huge pages and larger. By doing that I can justify performing the screening based on page order in the inline path and avoid any PV infrastructure overhead unless I have to incur it.

Quoted 67 lines · Alexander Duyck

Quoted 63 lines · Nadav Amit

Quoted 54 lines · Alexander Duyck

Quoted 47 lines · Nadav Amit

Quoted 32 lines · Alexander Duyck

From: Alexander Duyck <[email protected]>

Add guest support for providing free memory hints to the KVM hypervisor for freed pages huge TLB size or larger. I am restricting the size to huge TLB order and larger because the hypercalls are too expensive to be performing one per 4K page. Using the huge TLB order became the obvious choice for the order to use as it allows us to avoid fragmentation of higher order memory on the host.

I have limited the functionality so that it doesn't work when page poisoning is enabled. I did this because a write to the page after doing an MADV_DONTNEED would effectively negate the hint, so it would be wasting cycles to do so.

Signed-off-by Alexander Duyck <[email protected]>

---

arch/x86/include/asm/page.h |   13 +++++++++++++
arch/x86/kernel/kvm.c       |   23 +++++++++++++++++++++++
2 files changed, 36 insertions(+)

arch/x86/include/asm/page.h +2 -0

index 7555b48803a8..4487ad7a3385 100644
@@ -18,6 +18,19 @@
 
 struct page;
 
+#ifdef CONFIG_KVM_GUEST
+#include <linux/jump_label.h>

arch/x86/include/asm/page.h line 32

Quoted 10 lines

arch/x86/include/asm/page.h +10 -0

+extern struct static_key_false pv_free_page_hint_enabled;
+
+#define HAVE_ARCH_FREE_PAGE
+void __arch_free_page(struct page *page, unsigned int order);
+static inline void arch_free_page(struct page *page, unsigned int order)
+{
+   if (static_branch_unlikely(&pv_free_page_hint_enabled))
+           __arch_free_page(page, order);
+}
+#endif

This patch and the following one assume that only KVM should be able to hook to these events. I do not think it is appropriate for __arch_free_page() to effectively mean “kvm_guest_free_page()”.

Quoted 3 lines

Is it possible to use the paravirt infrastructure for this feature, similarly to other PV features? It is not the best infrastructure, but at least it is hypervisor-neutral.

Quoted 6 lines

I could probably tie this into the paravirt infrastructure, but if I did so I would probably want to pull the checks for the page order out of the KVM specific bits and make it something we handle in the inline. Doing that I would probably make it a paravirtual hint that only operates at the PMD level. That way we wouldn't incur the cost of the paravirt infrastructure at the per 4K page level.

If I understand you correctly, you “complain” that this would affect performance.

Quoted 6 lines

It wasn't so much a "complaint" as an "observation". What I was getting at is that if I am going to make it a PV operation I might set a hard limit on it so that it will specifically only apply to huge pages and larger. By doing that I can justify performing the screening based on page order in the inline path and avoid any PV infrastructure overhead unless I have to incur it.

I understood. I guess my use of “double quotes” was lost in translation. ;-)

One more point regarding [2/4] - you may want to consider using madvise_free instead of madvise_dontneed to avoid unnecessary EPT violations.

From: Alexander Duyck <[email protected]>

Add guest support for providing free memory hints to the KVM hypervisor for freed pages huge TLB size or larger. I am restricting the size to huge TLB order and larger because the hypercalls are too expensive to be performing one per 4K page. Using the huge TLB order became the obvious choice for the order to use as it allows us to avoid fragmentation of higher order memory on the host.

I have limited the functionality so that it doesn't work when page poisoning is enabled. I did this because a write to the page after doing an MADV_DONTNEED would effectively negate the hint, so it would be wasting cycles to do so.

Signed-off-by Alexander Duyck <[email protected]>

---
 arch/x86/include/asm/page.h |   13 +++++++++++++
 arch/x86/kernel/kvm.c       |   23 +++++++++++++++++++++++
 2 files changed, 36 insertions(+)

arch/x86/include/asm/page.h +13 -0

index 7555b48803a8..4487ad7a3385 100644
@@ -18,6 +18,19 @@
 
 struct page;
 
+#ifdef CONFIG_KVM_GUEST
+#include <linux/jump_label.h>
+extern struct static_key_false pv_free_page_hint_enabled;
+
+#define HAVE_ARCH_FREE_PAGE
+void __arch_free_page(struct page *page, unsigned int order);
+static inline void arch_free_page(struct page *page, unsigned int order)
+{
+	if (static_branch_unlikely(&pv_free_page_hint_enabled))
+		__arch_free_page(page, order);
+}
+#endif
+
 #include <linux/range.h>
 extern struct range pfn_mapped[];
 extern int nr_pfn_mapped;

arch/x86/kernel/kvm.c +11 -0

index 5c93a65ee1e5..09c91641c36c 100644
@@ -48,6 +48,7 @@
 #include <asm/tlb.h>
 
 static int kvmapf = 1;
+DEFINE_STATIC_KEY_FALSE(pv_free_page_hint_enabled);
 
 static int __init parse_no_kvmapf(char *arg)
 {

@@ -648,6 +649,15 @@ static void __init kvm_guest_init(void)
 	if (kvm_para_has_feature(KVM_FEATURE_PV_EOI))
 		apic_set_eoi_write(kvm_guest_apic_eoi_write);
 
+	/*
+	 * The free page hinting doesn't add much value if page poisoning
+	 * is enabled. So we only enable the feature if page poisoning is
+	 * no present.
+	 */
+	if (!page_poisoning_enabled() &&
+	    kvm_para_has_feature(KVM_FEATURE_PV_UNUSED_PAGE_HINT))
+		static_branch_enable(&pv_free_page_hint_enabled);
+
 #ifdef CONFIG_SMP
 	smp_ops.smp_prepare_cpus = kvm_smp_prepare_cpus;
 	smp_ops.smp_prepare_boot_cpu = kvm_smp_prepare_boot_cpu;

@@ -762,6 +772,19 @@ static __init int kvm_setup_pv_tlb_flush(void)
 }
 arch_initcall(kvm_setup_pv_tlb_flush);
 
+void __arch_free_page(struct page *page, unsigned int order)

arch/x86/kernel/kvm.c +2 -0

+}
+
 #ifdef CONFIG_PARAVIRT_SPINLOCKS
 
 /* Kick a cpu by its apicid. Used to wake up a halted vcpu */

Quoted 76 lines · On Mon, 04 Feb 2019 10:15:52 -0800 Alexander Duyck <[email protected]>…

From: Alexander Duyck <[email protected]>

Add guest support for providing free memory hints to the KVM hypervisor for freed pages huge TLB size or larger. I am restricting the size to huge TLB order and larger because the hypercalls are too expensive to be performing one per 4K page. Using the huge TLB order became the obvious choice for the order to use as it allows us to avoid fragmentation of higher order memory on the host.

I have limited the functionality so that it doesn't work when page poisoning is enabled. I did this because a write to the page after doing an MADV_DONTNEED would effectively negate the hint, so it would be wasting cycles to do so.

Signed-off-by Alexander Duyck <[email protected]>

---
 arch/x86/include/asm/page.h |   13 +++++++++++++
 arch/x86/kernel/kvm.c       |   23 +++++++++++++++++++++++
 2 files changed, 36 insertions(+)

arch/x86/include/asm/page.h +13 -0

index 7555b48803a8..4487ad7a3385 100644
@@ -18,6 +18,19 @@
 
 struct page;
 
+#ifdef CONFIG_KVM_GUEST
+#include <linux/jump_label.h>
+extern struct static_key_false pv_free_page_hint_enabled;
+
+#define HAVE_ARCH_FREE_PAGE
+void __arch_free_page(struct page *page, unsigned int order);
+static inline void arch_free_page(struct page *page, unsigned int order)
+{
+	if (static_branch_unlikely(&pv_free_page_hint_enabled))
+		__arch_free_page(page, order);
+}
+#endif
+
 #include <linux/range.h>
 extern struct range pfn_mapped[];
 extern int nr_pfn_mapped;

arch/x86/kernel/kvm.c +10 -0

index 5c93a65ee1e5..09c91641c36c 100644
@@ -48,6 +48,7 @@
 #include <asm/tlb.h>
 
 static int kvmapf = 1;
+DEFINE_STATIC_KEY_FALSE(pv_free_page_hint_enabled);
 
 static int __init parse_no_kvmapf(char *arg)
 {

@@ -648,6 +649,15 @@ static void __init kvm_guest_init(void)
 	if (kvm_para_has_feature(KVM_FEATURE_PV_EOI))
 		apic_set_eoi_write(kvm_guest_apic_eoi_write);
 
+	/*
+	 * The free page hinting doesn't add much value if page poisoning
+	 * is enabled. So we only enable the feature if page poisoning is
+	 * no present.
+	 */
+	if (!page_poisoning_enabled() &&
+	    kvm_para_has_feature(KVM_FEATURE_PV_UNUSED_PAGE_HINT))
+		static_branch_enable(&pv_free_page_hint_enabled);
+
 #ifdef CONFIG_SMP
 	smp_ops.smp_prepare_cpus = kvm_smp_prepare_cpus;
 	smp_ops.smp_prepare_boot_cpu = kvm_smp_prepare_boot_cpu;

@@ -762,6 +772,19 @@ static __init int kvm_setup_pv_tlb_flush(void)
 }
 arch_initcall(kvm_setup_pv_tlb_flush);
 

arch/x86/kernel/kvm.c static __init int kvm_setup_pv_tlb_flush(void) · line 785

Quoted 11 lines

arch/x86/kernel/kvm.c +11 -0

+void __arch_free_page(struct page *page, unsigned int order)
+{
+	/*
+	 * Limit hints to blocks no smaller than pageblock in
+	 * size to limit the cost for the hypercalls.
+	 */
+	if (order < KVM_PV_UNUSED_PAGE_HINT_MIN_ORDER)
+		return;
+
+	kvm_hypercall2(KVM_HC_UNUSED_PAGE_HINT, page_to_phys(page),
+		       PAGE_SIZE << order);

Does this mean that the vCPU executing this will get stuck here for the duration of the hypercall? Isn't that too long, considering that the zone lock is taken and madvise in the host block on semaphores?

Quoted 79 lines · Luiz Capitulino

Quoted 76 lines · On Mon, 04 Feb 2019 10:15:52 -0800 Alexander Duyck <[email protected]>…

From: Alexander Duyck <[email protected]>

Add guest support for providing free memory hints to the KVM hypervisor for freed pages huge TLB size or larger. I am restricting the size to huge TLB order and larger because the hypercalls are too expensive to be performing one per 4K page. Using the huge TLB order became the obvious choice for the order to use as it allows us to avoid fragmentation of higher order memory on the host.

I have limited the functionality so that it doesn't work when page poisoning is enabled. I did this because a write to the page after doing an MADV_DONTNEED would effectively negate the hint, so it would be wasting cycles to do so.

Signed-off-by Alexander Duyck <[email protected]>

---
 arch/x86/include/asm/page.h |   13 +++++++++++++
 arch/x86/kernel/kvm.c       |   23 +++++++++++++++++++++++
 2 files changed, 36 insertions(+)

arch/x86/include/asm/page.h +13 -0

index 7555b48803a8..4487ad7a3385 100644
@@ -18,6 +18,19 @@
 
 struct page;
 
+#ifdef CONFIG_KVM_GUEST
+#include <linux/jump_label.h>
+extern struct static_key_false pv_free_page_hint_enabled;
+
+#define HAVE_ARCH_FREE_PAGE
+void __arch_free_page(struct page *page, unsigned int order);
+static inline void arch_free_page(struct page *page, unsigned int order)
+{
+	if (static_branch_unlikely(&pv_free_page_hint_enabled))
+		__arch_free_page(page, order);
+}
+#endif
+
 #include <linux/range.h>
 extern struct range pfn_mapped[];
 extern int nr_pfn_mapped;

arch/x86/kernel/kvm.c +10 -0

index 5c93a65ee1e5..09c91641c36c 100644
@@ -48,6 +48,7 @@
 #include <asm/tlb.h>
 
 static int kvmapf = 1;
+DEFINE_STATIC_KEY_FALSE(pv_free_page_hint_enabled);
 
 static int __init parse_no_kvmapf(char *arg)
 {

@@ -648,6 +649,15 @@ static void __init kvm_guest_init(void)
 	if (kvm_para_has_feature(KVM_FEATURE_PV_EOI))
 		apic_set_eoi_write(kvm_guest_apic_eoi_write);
 
+	/*
+	 * The free page hinting doesn't add much value if page poisoning
+	 * is enabled. So we only enable the feature if page poisoning is
+	 * no present.
+	 */
+	if (!page_poisoning_enabled() &&
+	    kvm_para_has_feature(KVM_FEATURE_PV_UNUSED_PAGE_HINT))
+		static_branch_enable(&pv_free_page_hint_enabled);
+
 #ifdef CONFIG_SMP
 	smp_ops.smp_prepare_cpus = kvm_smp_prepare_cpus;
 	smp_ops.smp_prepare_boot_cpu = kvm_smp_prepare_boot_cpu;

@@ -762,6 +772,19 @@ static __init int kvm_setup_pv_tlb_flush(void)
 }
 arch_initcall(kvm_setup_pv_tlb_flush);
 

Quoted 16 lines

arch/x86/kernel/kvm.c static __init int kvm_setup_pv_tlb_flush(void) · line 785

Quoted 11 lines

arch/x86/kernel/kvm.c +11 -0

+void __arch_free_page(struct page *page, unsigned int order)
+{
+	/*
+	 * Limit hints to blocks no smaller than pageblock in
+	 * size to limit the cost for the hypercalls.
+	 */
+	if (order < KVM_PV_UNUSED_PAGE_HINT_MIN_ORDER)
+		return;
+
+	kvm_hypercall2(KVM_HC_UNUSED_PAGE_HINT, page_to_phys(page),
+		       PAGE_SIZE << order);  

Does this mean that the vCPU executing this will get stuck here for the duration of the hypercall? Isn't that too long, considering that the zone lock is taken and madvise in the host block on semaphores?

I'm pretty sure the zone lock isn't held when this is called. The lock isn't acquired until later in the path. This gets executed just before the page poisoning call which would take time as well since it would have to memset an entire page. This function is called as a part of free_pages_prepare, the zone locks aren't acquired until we are calling into either free_one_page and a few spots before calling __free_one_page.

My other function in patch 4 which does this from inside of __free_one_page does have to release the zone lock since it is taken there.

Quoted 79 lines · Luiz Capitulino

Quoted 76 lines · On Mon, 04 Feb 2019 10:15:52 -0800 Alexander Duyck <[email protected]>…

From: Alexander Duyck <[email protected]>

Add guest support for providing free memory hints to the KVM hypervisor for freed pages huge TLB size or larger. I am restricting the size to huge TLB order and larger because the hypercalls are too expensive to be performing one per 4K page. Using the huge TLB order became the obvious choice for the order to use as it allows us to avoid fragmentation of higher order memory on the host.

I have limited the functionality so that it doesn't work when page poisoning is enabled. I did this because a write to the page after doing an MADV_DONTNEED would effectively negate the hint, so it would be wasting cycles to do so.

Signed-off-by Alexander Duyck <[email protected]>

---
 arch/x86/include/asm/page.h |   13 +++++++++++++
 arch/x86/kernel/kvm.c       |   23 +++++++++++++++++++++++
 2 files changed, 36 insertions(+)

arch/x86/include/asm/page.h +13 -0

index 7555b48803a8..4487ad7a3385 100644
@@ -18,6 +18,19 @@
 
 struct page;
 
+#ifdef CONFIG_KVM_GUEST
+#include <linux/jump_label.h>
+extern struct static_key_false pv_free_page_hint_enabled;
+
+#define HAVE_ARCH_FREE_PAGE
+void __arch_free_page(struct page *page, unsigned int order);
+static inline void arch_free_page(struct page *page, unsigned int order)
+{
+	if (static_branch_unlikely(&pv_free_page_hint_enabled))
+		__arch_free_page(page, order);
+}
+#endif
+
 #include <linux/range.h>
 extern struct range pfn_mapped[];
 extern int nr_pfn_mapped;

arch/x86/kernel/kvm.c +10 -0

index 5c93a65ee1e5..09c91641c36c 100644
@@ -48,6 +48,7 @@
 #include <asm/tlb.h>
 
 static int kvmapf = 1;
+DEFINE_STATIC_KEY_FALSE(pv_free_page_hint_enabled);
 
 static int __init parse_no_kvmapf(char *arg)
 {

@@ -648,6 +649,15 @@ static void __init kvm_guest_init(void)
 	if (kvm_para_has_feature(KVM_FEATURE_PV_EOI))
 		apic_set_eoi_write(kvm_guest_apic_eoi_write);
 
+	/*
+	 * The free page hinting doesn't add much value if page poisoning
+	 * is enabled. So we only enable the feature if page poisoning is
+	 * no present.
+	 */
+	if (!page_poisoning_enabled() &&
+	    kvm_para_has_feature(KVM_FEATURE_PV_UNUSED_PAGE_HINT))
+		static_branch_enable(&pv_free_page_hint_enabled);
+
 #ifdef CONFIG_SMP
 	smp_ops.smp_prepare_cpus = kvm_smp_prepare_cpus;
 	smp_ops.smp_prepare_boot_cpu = kvm_smp_prepare_boot_cpu;

@@ -762,6 +772,19 @@ static __init int kvm_setup_pv_tlb_flush(void)
 }
 arch_initcall(kvm_setup_pv_tlb_flush);
 

Quoted 15 lines

arch/x86/kernel/kvm.c static __init int kvm_setup_pv_tlb_flush(void) · line 785

Quoted 11 lines

arch/x86/kernel/kvm.c +11 -0

+void __arch_free_page(struct page *page, unsigned int order)
+{
+	/*
+	 * Limit hints to blocks no smaller than pageblock in
+	 * size to limit the cost for the hypercalls.
+	 */
+	if (order < KVM_PV_UNUSED_PAGE_HINT_MIN_ORDER)
+		return;
+
+	kvm_hypercall2(KVM_HC_UNUSED_PAGE_HINT, page_to_phys(page),
+		       PAGE_SIZE << order);

Does this mean that the vCPU executing this will get stuck here for the duration of the hypercall? Isn't that too long, considering that the zone lock is taken and madvise in the host block on semaphores?

I'm pretty sure the zone lock isn't held when this is called. The lock isn't acquired until later in the path. This gets executed just before the page poisoning call which would take time as well since it would have to memset an entire page. This function is called as a part of free_pages_prepare, the zone locks aren't acquired until we are calling into either free_one_page and a few spots before calling __free_one_page.

My other function in patch 4 which does this from inside of __free_one_page does have to release the zone lock since it is taken there.

Quoted 105 lines · Alexander Duyck

Quoted 79 lines · Luiz Capitulino

Quoted 76 lines · On Mon, 04 Feb 2019 10:15:52 -0800 Alexander Duyck <[email protected]>…

From: Alexander Duyck <[email protected]>

Add guest support for providing free memory hints to the KVM hypervisor for freed pages huge TLB size or larger. I am restricting the size to huge TLB order and larger because the hypercalls are too expensive to be performing one per 4K page. Using the huge TLB order became the obvious choice for the order to use as it allows us to avoid fragmentation of higher order memory on the host.

I have limited the functionality so that it doesn't work when page poisoning is enabled. I did this because a write to the page after doing an MADV_DONTNEED would effectively negate the hint, so it would be wasting cycles to do so.

Signed-off-by Alexander Duyck <[email protected]>

---
 arch/x86/include/asm/page.h |   13 +++++++++++++
 arch/x86/kernel/kvm.c       |   23 +++++++++++++++++++++++
 2 files changed, 36 insertions(+)

arch/x86/include/asm/page.h +13 -0

index 7555b48803a8..4487ad7a3385 100644
@@ -18,6 +18,19 @@
 
 struct page;
 
+#ifdef CONFIG_KVM_GUEST
+#include <linux/jump_label.h>
+extern struct static_key_false pv_free_page_hint_enabled;
+
+#define HAVE_ARCH_FREE_PAGE
+void __arch_free_page(struct page *page, unsigned int order);
+static inline void arch_free_page(struct page *page, unsigned int order)
+{
+	if (static_branch_unlikely(&pv_free_page_hint_enabled))
+		__arch_free_page(page, order);
+}
+#endif
+
 #include <linux/range.h>
 extern struct range pfn_mapped[];
 extern int nr_pfn_mapped;

arch/x86/kernel/kvm.c +10 -0

index 5c93a65ee1e5..09c91641c36c 100644
@@ -48,6 +48,7 @@
 #include <asm/tlb.h>
 
 static int kvmapf = 1;
+DEFINE_STATIC_KEY_FALSE(pv_free_page_hint_enabled);
 
 static int __init parse_no_kvmapf(char *arg)
 {

@@ -648,6 +649,15 @@ static void __init kvm_guest_init(void)
 	if (kvm_para_has_feature(KVM_FEATURE_PV_EOI))
 		apic_set_eoi_write(kvm_guest_apic_eoi_write);
 
+	/*
+	 * The free page hinting doesn't add much value if page poisoning
+	 * is enabled. So we only enable the feature if page poisoning is
+	 * no present.
+	 */
+	if (!page_poisoning_enabled() &&
+	    kvm_para_has_feature(KVM_FEATURE_PV_UNUSED_PAGE_HINT))
+		static_branch_enable(&pv_free_page_hint_enabled);
+
 #ifdef CONFIG_SMP
 	smp_ops.smp_prepare_cpus = kvm_smp_prepare_cpus;
 	smp_ops.smp_prepare_boot_cpu = kvm_smp_prepare_boot_cpu;

@@ -762,6 +772,19 @@ static __init int kvm_setup_pv_tlb_flush(void)
 }
 arch_initcall(kvm_setup_pv_tlb_flush);
 

Quoted 16 lines

arch/x86/kernel/kvm.c static __init int kvm_setup_pv_tlb_flush(void) · line 785

Quoted 11 lines

arch/x86/kernel/kvm.c +11 -0

+void __arch_free_page(struct page *page, unsigned int order)
+{
+	/*
+	 * Limit hints to blocks no smaller than pageblock in
+	 * size to limit the cost for the hypercalls.
+	 */
+	if (order < KVM_PV_UNUSED_PAGE_HINT_MIN_ORDER)
+		return;
+
+	kvm_hypercall2(KVM_HC_UNUSED_PAGE_HINT, page_to_phys(page),
+		       PAGE_SIZE << order);

Does this mean that the vCPU executing this will get stuck here for the duration of the hypercall? Isn't that too long, considering that the zone lock is taken and madvise in the host block on semaphores?

I'm pretty sure the zone lock isn't held when this is called. The lock isn't acquired until later in the path. This gets executed just before the page poisoning call which would take time as well since it would have to memset an entire page. This function is called as a part of free_pages_prepare, the zone locks aren't acquired until we are calling into either free_one_page and a few spots before calling __free_one_page.

Quoted 4 lines

My other function in patch 4 which does this from inside of __free_one_page does have to release the zone lock since it is taken there.

Considering hypercall's are costly, will it not make sense to coalesce the pages you are reporting and make a single hypercall for a bunch of pages?

From: Alexander Duyck <[email protected]>

Add guest support for providing free memory hints to the KVM hypervisor for freed pages huge TLB size or larger. I am restricting the size to huge TLB order and larger because the hypercalls are too expensive to be performing one per 4K page.

Using the huge TLB order became the obvious
choice for the order to use as it allows us to avoid fragmentation of higher order memory on the host.

I have limited the functionality so that it doesn't work when page poisoning is enabled. I did this because a write to the page after doing an MADV_DONTNEED would effectively negate the hint, so it would be wasting cycles to do so.

Signed-off-by Alexander Duyck <[email protected]>

---
 arch/x86/include/asm/page.h |   13 +++++++++++++
 arch/x86/kernel/kvm.c       |   23 +++++++++++++++++++++++
 2 files changed, 36 insertions(+)

arch/x86/include/asm/page.h +13 -0

index 7555b48803a8..4487ad7a3385 100644
@@ -18,6 +18,19 @@
 
 struct page;
 
+#ifdef CONFIG_KVM_GUEST
+#include <linux/jump_label.h>
+extern struct static_key_false pv_free_page_hint_enabled;
+
+#define HAVE_ARCH_FREE_PAGE
+void __arch_free_page(struct page *page, unsigned int order);
+static inline void arch_free_page(struct page *page, unsigned int order)
+{
+	if (static_branch_unlikely(&pv_free_page_hint_enabled))
+		__arch_free_page(page, order);
+}
+#endif
+
 #include <linux/range.h>
 extern struct range pfn_mapped[];
 extern int nr_pfn_mapped;

arch/x86/kernel/kvm.c +23 -0

index 5c93a65ee1e5..09c91641c36c 100644
@@ -48,6 +48,7 @@
 #include <asm/tlb.h>
 
 static int kvmapf = 1;
+DEFINE_STATIC_KEY_FALSE(pv_free_page_hint_enabled);
 
 static int __init parse_no_kvmapf(char *arg)
 {

@@ -648,6 +649,15 @@ static void __init kvm_guest_init(void)
 	if (kvm_para_has_feature(KVM_FEATURE_PV_EOI))
 		apic_set_eoi_write(kvm_guest_apic_eoi_write);
 
+	/*
+	 * The free page hinting doesn't add much value if page poisoning
+	 * is enabled. So we only enable the feature if page poisoning is
+	 * no present.
+	 */
+	if (!page_poisoning_enabled() &&
+	    kvm_para_has_feature(KVM_FEATURE_PV_UNUSED_PAGE_HINT))
+		static_branch_enable(&pv_free_page_hint_enabled);
+
 #ifdef CONFIG_SMP
 	smp_ops.smp_prepare_cpus = kvm_smp_prepare_cpus;
 	smp_ops.smp_prepare_boot_cpu = kvm_smp_prepare_boot_cpu;

@@ -762,6 +772,19 @@ static __init int kvm_setup_pv_tlb_flush(void)
 }
 arch_initcall(kvm_setup_pv_tlb_flush);
 
+void __arch_free_page(struct page *page, unsigned int order)
+{
+	/*
+	 * Limit hints to blocks no smaller than pageblock in
+	 * size to limit the cost for the hypercalls.
+	 */
+	if (order < KVM_PV_UNUSED_PAGE_HINT_MIN_ORDER)
+		return;
+
+	kvm_hypercall2(KVM_HC_UNUSED_PAGE_HINT, page_to_phys(page),
+		       PAGE_SIZE << order);
+}
+
 #ifdef CONFIG_PARAVIRT_SPINLOCKS
 
 /* Kick a cpu by its apicid. Used to wake up a halted vcpu */

Quoted 6 lines · Alexander Duyck

From: Alexander Duyck <[email protected]>

Add guest support for providing free memory hints to the KVM hypervisor for freed pages huge TLB size or larger. I am restricting the size to huge TLB order and larger because the hypercalls are too expensive to be performing one per 4K page.

Even 2M pages start to get expensive with a TB guest.

Really it seems we want a virtio ring so we can pass a batch of these. E.g. 256 entries, 2M each - that's more like it.

Quoted 8 lines

Using the huge TLB order became the obvious
choice for the order to use as it allows us to avoid fragmentation of higher order memory on the host.

I have limited the functionality so that it doesn't work when page poisoning is enabled. I did this because a write to the page after doing an MADV_DONTNEED would effectively negate the hint, so it would be wasting cycles to do so.

Again that's leaking host implementation detail into guest interface.

We are giving guest page hints to host that makes sense, weird interactions with other features due to host implementation details should be handled by host.

Quoted 9 lines · Michael S. Tsirkin

Quoted 6 lines · Alexander Duyck

From: Alexander Duyck <[email protected]>

Add guest support for providing free memory hints to the KVM hypervisor for freed pages huge TLB size or larger. I am restricting the size to huge TLB order and larger because the hypercalls are too expensive to be performing one per 4K page.

Even 2M pages start to get expensive with a TB guest.

Agreed.

Quoted 2 lines

Really it seems we want a virtio ring so we can pass a batch of these. E.g. 256 entries, 2M each - that's more like it.

The only issue I see with doing that is that we then have to defer the freeing. Doing that is going to introduce issues in the guest as we are going to have pages going unused for some period of time while we wait for the hint to complete, and we cannot just pull said pages back. I'm not really a fan of the asynchronous nature of Nitesh's patches for this reason.

Quoted 11 lines

Quoted 8 lines

Using the huge TLB order became the obvious
choice for the order to use as it allows us to avoid fragmentation of higher order memory on the host.

I have limited the functionality so that it doesn't work when page poisoning is enabled. I did this because a write to the page after doing an MADV_DONTNEED would effectively negate the hint, so it would be wasting cycles to do so.

Again that's leaking host implementation detail into guest interface.

Quoted 3 lines

We are giving guest page hints to host that makes sense, weird interactions with other features due to host implementation details should be handled by host.

I don't view this as a host implementation detail, this is guest feature making use of all pages for debugging. If we are placing poison values in the page then I wouldn't consider them an unused page, it is being actively used to store the poison value.

If we can achieve this
and free the page back to the host then even better, but until the features can coexist we should not use the page hinting while page poisoning is enabled.

This is one of the reasons why I was opposed to just disabling page poisoning when this feature was enabled in Nitesh's patches. If the guest has page poisoning enabled it is doing something with the page. It shouldn't be prevented from doing that because the host wants to have the option to free the pages.

Quoted 16 lines · Alexander Duyck

Quoted 9 lines · Michael S. Tsirkin

Quoted 6 lines · Alexander Duyck

From: Alexander Duyck <[email protected]>

Add guest support for providing free memory hints to the KVM hypervisor for freed pages huge TLB size or larger. I am restricting the size to huge TLB order and larger because the hypercalls are too expensive to be performing one per 4K page.

Even 2M pages start to get expensive with a TB guest.

Agreed.

Quoted 2 lines

Really it seems we want a virtio ring so we can pass a batch of these. E.g. 256 entries, 2M each - that's more like it.

Quoted 6 lines

The only issue I see with doing that is that we then have to defer the freeing. Doing that is going to introduce issues in the guest as we are going to have pages going unused for some period of time while we wait for the hint to complete, and we cannot just pull said pages back. I'm not really a fan of the asynchronous nature of Nitesh's patches for this reason.

Well nothing prevents us from doing an extra exit to the hypervisor if we want. The asynchronous nature is there as an optimization to allow hypervisor to do its thing on a separate CPU.
Why not proceed doing other things meanwhile?
And if the reason is that we are short on memory, then maybe we should be less aggressive in hinting?

E.g. if we just have 2 pages:

hint page 1
page 1 hint processed?
yes - proceed to page 2
no - wait for interrupt

get interrupt that page 1 hint is processed
hint page 2

If hypervisor happens to be running on same CPU it can process things synchronously and we never enter the no branch.

Quoted 11 lines

Quoted 11 lines

Quoted 8 lines

Using the huge TLB order became the obvious
choice for the order to use as it allows us to avoid fragmentation of higher order memory on the host.

I have limited the functionality so that it doesn't work when page poisoning is enabled. I did this because a write to the page after doing an MADV_DONTNEED would effectively negate the hint, so it would be wasting cycles to do so.

Again that's leaking host implementation detail into guest interface.

Quoted 8 lines

Quoted 3 lines

We are giving guest page hints to host that makes sense, weird interactions with other features due to host implementation details should be handled by host.

I don't view this as a host implementation detail, this is guest feature making use of all pages for debugging. If we are placing poison values in the page then I wouldn't consider them an unused page, it is being actively used to store the poison value.

Well I guess it's a valid point of view for a kernel hacker, but they are unused from application's point of view.
However poisoning is transparent to users and most distro users are not aware of it going on. They just know that debug kernels are slower.
User loading a debug kernel and immediately breaking overcommit is an unpleasant experience.

Quoted 4 lines

If we can achieve this
and free the page back to the host then even better, but until the features can coexist we should not use the page hinting while page poisoning is enabled.

Existing hinting in balloon allows them to coexist so I think we need to set the bar just as high for any new variant.

Quoted 5 lines

This is one of the reasons why I was opposed to just disabling page poisoning when this feature was enabled in Nitesh's patches. If the guest has page poisoning enabled it is doing something with the page. It shouldn't be prevented from doing that because the host wants to have the option to free the pages.

I agree but I think the decision belongs on the host. I.e. hint the page but tell the host it needs to be careful about the poison value. It might also mean we
need to make sure poisoning happens after the hinting, not before.

Quoted 38 lines · Michael S. Tsirkin

Quoted 16 lines · Alexander Duyck

Quoted 9 lines · Michael S. Tsirkin

Quoted 6 lines · Alexander Duyck

From: Alexander Duyck <[email protected]>

Add guest support for providing free memory hints to the KVM hypervisor for freed pages huge TLB size or larger. I am restricting the size to huge TLB order and larger because the hypercalls are too expensive to be performing one per 4K page.

Even 2M pages start to get expensive with a TB guest.

Agreed.

Quoted 2 lines

Really it seems we want a virtio ring so we can pass a batch of these. E.g. 256 entries, 2M each - that's more like it.

Quoted 6 lines

The only issue I see with doing that is that we then have to defer the freeing. Doing that is going to introduce issues in the guest as we are going to have pages going unused for some period of time while we wait for the hint to complete, and we cannot just pull said pages back. I'm not really a fan of the asynchronous nature of Nitesh's patches for this reason.

Well nothing prevents us from doing an extra exit to the hypervisor if we want. The asynchronous nature is there as an optimization to allow hypervisor to do its thing on a separate CPU.
Why not proceed doing other things meanwhile?
And if the reason is that we are short on memory, then maybe we should be less aggressive in hinting?

E.g. if we just have 2 pages:

hint page 1
page 1 hint processed?
yes - proceed to page 2
no - wait for interrupt

Quoted 8 lines

get interrupt that page 1 hint is processed
hint page 2

If hypervisor happens to be running on same CPU it can process things synchronously and we never enter the no branch.

Another concern I would have about processing this asynchronously is that we have the potential for multiple guest CPUs to become bottlenecked by a single host CPU. I am not sure if that is something that would be desirable.

Quoted 20 lines

Quoted 11 lines

Quoted 11 lines

Quoted 8 lines

Using the huge TLB order became the obvious
choice for the order to use as it allows us to avoid fragmentation of higher order memory on the host.

I have limited the functionality so that it doesn't work when page poisoning is enabled. I did this because a write to the page after doing an MADV_DONTNEED would effectively negate the hint, so it would be wasting cycles to do so.

Again that's leaking host implementation detail into guest interface.

Quoted 8 lines

Quoted 3 lines

We are giving guest page hints to host that makes sense, weird interactions with other features due to host implementation details should be handled by host.

I don't view this as a host implementation detail, this is guest feature making use of all pages for debugging. If we are placing poison values in the page then I wouldn't consider them an unused page, it is being actively used to store the poison value.

Quoted 7 lines

Well I guess it's a valid point of view for a kernel hacker, but they are unused from application's point of view.
However poisoning is transparent to users and most distro users are not aware of it going on. They just know that debug kernels are slower.
User loading a debug kernel and immediately breaking overcommit is an unpleasant experience.

How would that be any different then a user loading an older kernel that doesn't have this feature and breaking overcommit as a result?

I still think it would be better if we left the poisoning enabled in such a case and just displayed a warning message if nothing else that hinting is disabled because of page poisoning.

One other thought I had on this is that one side effect of page poisoning is probably that KSM would be able to merge all of the poison pages together into a single page since they are all set to the same values. So even with the poisoned pages it would be possible to reduce total memory overhead.

Quoted 7 lines

Quoted 4 lines

If we can achieve this
and free the page back to the host then even better, but until the features can coexist we should not use the page hinting while page poisoning is enabled.

Existing hinting in balloon allows them to coexist so I think we need to set the bar just as high for any new variant.

That is what I heard. I will have to look into this.

Quoted 10 lines

Quoted 5 lines

This is one of the reasons why I was opposed to just disabling page poisoning when this feature was enabled in Nitesh's patches. If the guest has page poisoning enabled it is doing something with the page. It shouldn't be prevented from doing that because the host wants to have the option to free the pages.

I agree but I think the decision belongs on the host. I.e. hint the page but tell the host it needs to be careful about the poison value. It might also mean we
need to make sure poisoning happens after the hinting, not before.

The only issue with poisoning after instead of before is that the hint is ignored and we end up triggering a page fault and zero as a result. It might make more sense to have an architecture specific call that can be paravirtualized to handle the case of poisoning the page for us if we have the unused page hint enabled. Otherwise the write to the page is a given to invalidate the hint.

Quoted 48 lines · Alexander Duyck

Quoted 38 lines · Michael S. Tsirkin

Quoted 16 lines · Alexander Duyck

Quoted 9 lines · Michael S. Tsirkin

Quoted 6 lines · Alexander Duyck

From: Alexander Duyck <[email protected]>

Add guest support for providing free memory hints to the KVM hypervisor for freed pages huge TLB size or larger. I am restricting the size to huge TLB order and larger because the hypercalls are too expensive to be performing one per 4K page.

Even 2M pages start to get expensive with a TB guest.

Agreed.

Quoted 2 lines

Really it seems we want a virtio ring so we can pass a batch of these. E.g. 256 entries, 2M each - that's more like it.

Quoted 6 lines

The only issue I see with doing that is that we then have to defer the freeing. Doing that is going to introduce issues in the guest as we are going to have pages going unused for some period of time while we wait for the hint to complete, and we cannot just pull said pages back. I'm not really a fan of the asynchronous nature of Nitesh's patches for this reason.

Well nothing prevents us from doing an extra exit to the hypervisor if we want. The asynchronous nature is there as an optimization to allow hypervisor to do its thing on a separate CPU.
Why not proceed doing other things meanwhile?
And if the reason is that we are short on memory, then maybe we should be less aggressive in hinting?

E.g. if we just have 2 pages:

hint page 1
page 1 hint processed?
yes - proceed to page 2
no - wait for interrupt

Quoted 8 lines

get interrupt that page 1 hint is processed
hint page 2

If hypervisor happens to be running on same CPU it can process things synchronously and we never enter the no branch.

Quoted 4 lines

Another concern I would have about processing this asynchronously is that we have the potential for multiple guest CPUs to become bottlenecked by a single host CPU. I am not sure if that is something that would be desirable.

Well with a hypercall per page the fix is to block VCPU completely which is also not for everyone.

If you can't push a free page hint to host, then
ideally you just won't. That's a nice property of
hinting we have upstream right now.
Host too busy - hinting is just skipped.

Quoted 28 lines

Quoted 27 lines

Quoted 11 lines

Quoted 11 lines

Quoted 8 lines

Using the huge TLB order became the obvious
choice for the order to use as it allows us to avoid fragmentation of higher order memory on the host.

I have limited the functionality so that it doesn't work when page poisoning is enabled. I did this because a write to the page after doing an MADV_DONTNEED would effectively negate the hint, so it would be wasting cycles to do so.

Again that's leaking host implementation detail into guest interface.

Quoted 8 lines

Quoted 3 lines

We are giving guest page hints to host that makes sense, weird interactions with other features due to host implementation details should be handled by host.

I don't view this as a host implementation detail, this is guest feature making use of all pages for debugging. If we are placing poison values in the page then I wouldn't consider them an unused page, it is being actively used to store the poison value.

Well I guess it's a valid point of view for a kernel hacker, but they are unused from application's point of view.
However poisoning is transparent to users and most distro users are not aware of it going on. They just know that debug kernels are slower.
User loading a debug kernel and immediately breaking overcommit is an unpleasant experience.

Quoted 2 lines

How would that be any different then a user loading an older kernel that doesn't have this feature and breaking overcommit as a result?

Well old kernel does not have the feature so nothing to debug. When we have a new feature that goes away in the debug kernel, that's a big support problem since this leads to heisenbugs.

Quoted 9 lines

I still think it would be better if we left the poisoning enabled in such a case and just displayed a warning message if nothing else that hinting is disabled because of page poisoning.

One other thought I had on this is that one side effect of page poisoning is probably that KSM would be able to merge all of the poison pages together into a single page since they are all set to the same values. So even with the poisoned pages it would be possible to reduce total memory overhead.

Right. And BTW one thing that host can do is pass
the hinted area to KSM for merging.
That requires an alloc hook to free it though.

Or we could add a per-VMA byte with the poison value and use that on host to populate pages on fault.

Quoted 5 lines

Quoted 5 lines

Quoted 4 lines

If we can achieve this
and free the page back to the host then even better, but until the features can coexist we should not use the page hinting while page poisoning is enabled.

Quoted 4 lines

Quoted 2 lines

Existing hinting in balloon allows them to coexist so I think we need to set the bar just as high for any new variant.

That is what I heard. I will have to look into this.

It's not doing anything smart right now, just checks that poison == 0 and skips freeing if not. But it can be enhanced transparently to guests.

Quoted 11 lines

Quoted 10 lines

Quoted 5 lines

This is one of the reasons why I was opposed to just disabling page poisoning when this feature was enabled in Nitesh's patches. If the guest has page poisoning enabled it is doing something with the page. It shouldn't be prevented from doing that because the host wants to have the option to free the pages.

I agree but I think the decision belongs on the host. I.e. hint the page but tell the host it needs to be careful about the poison value. It might also mean we
need to make sure poisoning happens after the hinting, not before.

Quoted 6 lines

The only issue with poisoning after instead of before is that the hint is ignored and we end up triggering a page fault and zero as a result. It might make more sense to have an architecture specific call that can be paravirtualized to handle the case of poisoning the page for us if we have the unused page hint enabled. Otherwise the write to the page is a given to invalidate the hint.

Sounds interesting. So the arch hook will first poison and then pass the page to the host?

Or we can also ask the host to poison for us, problem is this forces host to either always write into page, or call MADV_DONTNEED, without it could do MADV_FREE. Maybe that is not a big issue.

Quoted 54 lines · Michael S. Tsirkin

Quoted 48 lines · Alexander Duyck

Quoted 38 lines · Michael S. Tsirkin

Quoted 16 lines · Alexander Duyck

Quoted 9 lines · Michael S. Tsirkin

Quoted 6 lines · Alexander Duyck

From: Alexander Duyck <[email protected]>

Add guest support for providing free memory hints to the KVM hypervisor for freed pages huge TLB size or larger. I am restricting the size to huge TLB order and larger because the hypercalls are too expensive to be performing one per 4K page.

Even 2M pages start to get expensive with a TB guest.

Agreed.

Quoted 2 lines

Really it seems we want a virtio ring so we can pass a batch of these. E.g. 256 entries, 2M each - that's more like it.

Quoted 6 lines

The only issue I see with doing that is that we then have to defer the freeing. Doing that is going to introduce issues in the guest as we are going to have pages going unused for some period of time while we wait for the hint to complete, and we cannot just pull said pages back. I'm not really a fan of the asynchronous nature of Nitesh's patches for this reason.

Well nothing prevents us from doing an extra exit to the hypervisor if we want. The asynchronous nature is there as an optimization to allow hypervisor to do its thing on a separate CPU.
Why not proceed doing other things meanwhile?
And if the reason is that we are short on memory, then maybe we should be less aggressive in hinting?

E.g. if we just have 2 pages:

hint page 1
page 1 hint processed?
yes - proceed to page 2
no - wait for interrupt

Quoted 8 lines

get interrupt that page 1 hint is processed
hint page 2

If hypervisor happens to be running on same CPU it can process things synchronously and we never enter the no branch.

Quoted 4 lines

Another concern I would have about processing this asynchronously is that we have the potential for multiple guest CPUs to become bottlenecked by a single host CPU. I am not sure if that is something that would be desirable.

Quoted 7 lines

Well with a hypercall per page the fix is to block VCPU completely which is also not for everyone.

If you can't push a free page hint to host, then
ideally you just won't. That's a nice property of
hinting we have upstream right now.
Host too busy - hinting is just skipped.

Right, but if you do that then there is a potential to end up missing hints for a large portion of memory. It seems like you would end up with even bigger issues since then at that point you have essentially leaked memory.
I would think you would need a way to resync the host and the guest after something like that. Otherwise you can have memory that will just go unused for an extended period if a guest just goes idle.

Quoted 32 lines

Quoted 31 lines

Quoted 21 lines

Quoted 12 lines

Quoted 12 lines

Quoted 9 lines

Using the huge TLB order became the obvious
choice for the order to use as it allows us to avoid fragmentation of higher order memory on the host.

I have limited the functionality so that it doesn't work when page poisoning is enabled. I did this because a write to the page after doing an MADV_DONTNEED would effectively negate the hint, so it would be wasting cycles to do so.

Again that's leaking host implementation detail into guest interface.

Quoted 8 lines

Quoted 3 lines

We are giving guest page hints to host that makes sense, weird interactions with other features due to host implementation details should be handled by host.

I don't view this as a host implementation detail, this is guest feature making use of all pages for debugging. If we are placing poison values in the page then I wouldn't consider them an unused page, it is being actively used to store the poison value.

Quoted 7 lines

Well I guess it's a valid point of view for a kernel hacker, but they are unused from application's point of view.
However poisoning is transparent to users and most distro users are not aware of it going on. They just know that debug kernels are slower.
User loading a debug kernel and immediately breaking overcommit is an unpleasant experience.

How would that be any different then a user loading an older kernel that doesn't have this feature and breaking overcommit as a result?

Quoted 3 lines

Well old kernel does not have the feature so nothing to debug. When we have a new feature that goes away in the debug kernel, that's a big support problem since this leads to heisenbugs.

Trying to debug host features from the guest would be a pain anyway as a guest shouldn't even really know what the underlying setup of the guest is supposed to be.

Quoted 23 lines

Quoted 9 lines

I still think it would be better if we left the poisoning enabled in such a case and just displayed a warning message if nothing else that hinting is disabled because of page poisoning.

One other thought I had on this is that one side effect of page poisoning is probably that KSM would be able to merge all of the poison pages together into a single page since they are all set to the same values. So even with the poisoned pages it would be possible to reduce total memory overhead.

Right. And BTW one thing that host can do is pass
the hinted area to KSM for merging.
That requires an alloc hook to free it though.

Or we could add a per-VMA byte with the poison value and use that on host to populate pages on fault.

Quoted 5 lines

Quoted 5 lines

Quoted 4 lines

If we can achieve this
and free the page back to the host then even better, but until the features can coexist we should not use the page hinting while page poisoning is enabled.

Quoted 8 lines

Quoted 4 lines

Quoted 2 lines

Existing hinting in balloon allows them to coexist so I think we need to set the bar just as high for any new variant.

That is what I heard. I will have to look into this.

It's not doing anything smart right now, just checks that poison == 0 and skips freeing if not. But it can be enhanced transparently to guests.

Okay, so it probably should be extended to add something like poison page that could replace the zero page for reads to a page that has been unmapped.

Quoted 18 lines

Quoted 11 lines

Quoted 10 lines

Quoted 5 lines

This is one of the reasons why I was opposed to just disabling page poisoning when this feature was enabled in Nitesh's patches. If the guest has page poisoning enabled it is doing something with the page. It shouldn't be prevented from doing that because the host wants to have the option to free the pages.

I agree but I think the decision belongs on the host. I.e. hint the page but tell the host it needs to be careful about the poison value. It might also mean we
need to make sure poisoning happens after the hinting, not before.

Quoted 6 lines

The only issue with poisoning after instead of before is that the hint is ignored and we end up triggering a page fault and zero as a result. It might make more sense to have an architecture specific call that can be paravirtualized to handle the case of poisoning the page for us if we have the unused page hint enabled. Otherwise the write to the page is a given to invalidate the hint.

Quoted 6 lines

Sounds interesting. So the arch hook will first poison and then pass the page to the host?

Or we can also ask the host to poison for us, problem is this forces host to either always write into page, or call MADV_DONTNEED, without it could do MADV_FREE. Maybe that is not a big issue.

I would think we would ask the host to poison for us. If I am not mistaken both solutions right now are using MADV_DONTNEED. I would tend to lean that way if we are doing page poisoning since the cost for zeroing/poisoning the page on the host could be canceled out by dropping the page poisoning on the guest.

Then again since we are doing higher order pages only, and the poisoning is supposed to happen before we get into __free_one_page we would probably have to do both the poisoning, and the poison on fault.

Quoted 63 lines · Alexander Duyck

Quoted 54 lines · Michael S. Tsirkin

Quoted 48 lines · Alexander Duyck

Quoted 38 lines · Michael S. Tsirkin

Quoted 16 lines · Alexander Duyck

Quoted 9 lines · Michael S. Tsirkin

Quoted 6 lines · Alexander Duyck

From: Alexander Duyck <[email protected]>

Add guest support for providing free memory hints to the KVM hypervisor for freed pages huge TLB size or larger. I am restricting the size to huge TLB order and larger because the hypercalls are too expensive to be performing one per 4K page.

Even 2M pages start to get expensive with a TB guest.

Agreed.

Quoted 2 lines

Really it seems we want a virtio ring so we can pass a batch of these. E.g. 256 entries, 2M each - that's more like it.

Quoted 6 lines

The only issue I see with doing that is that we then have to defer the freeing. Doing that is going to introduce issues in the guest as we are going to have pages going unused for some period of time while we wait for the hint to complete, and we cannot just pull said pages back. I'm not really a fan of the asynchronous nature of Nitesh's patches for this reason.

Well nothing prevents us from doing an extra exit to the hypervisor if we want. The asynchronous nature is there as an optimization to allow hypervisor to do its thing on a separate CPU.
Why not proceed doing other things meanwhile?
And if the reason is that we are short on memory, then maybe we should be less aggressive in hinting?

E.g. if we just have 2 pages:

hint page 1
page 1 hint processed?
yes - proceed to page 2
no - wait for interrupt

Quoted 8 lines

get interrupt that page 1 hint is processed
hint page 2

If hypervisor happens to be running on same CPU it can process things synchronously and we never enter the no branch.

Quoted 4 lines

Another concern I would have about processing this asynchronously is that we have the potential for multiple guest CPUs to become bottlenecked by a single host CPU. I am not sure if that is something that would be desirable.

Quoted 7 lines

Well with a hypercall per page the fix is to block VCPU completely which is also not for everyone.

If you can't push a free page hint to host, then
ideally you just won't. That's a nice property of
hinting we have upstream right now.
Host too busy - hinting is just skipped.

Quoted 7 lines

Right, but if you do that then there is a potential to end up missing hints for a large portion of memory. It seems like you would end up with even bigger issues since then at that point you have essentially leaked memory.
I would think you would need a way to resync the host and the guest after something like that. Otherwise you can have memory that will just go unused for an extended period if a guest just goes idle.

Yes and that is my point. Existing hints code will just take a page off the free list in that case so it resyncs using the free list.

Something like this could work then: mark up hinted pages with a flag (its easy to find unused flags for free pages) then when you get an interrupt because outstanding hints have been consumed, get unflagged/unhinted pages from buddy and pass them to host.

Quoted 36 lines

Quoted 35 lines

Quoted 29 lines

Quoted 21 lines

Quoted 12 lines

Quoted 12 lines

Quoted 9 lines

Using the huge TLB order became the obvious
choice for the order to use as it allows us to avoid fragmentation of higher order memory on the host.

I have limited the functionality so that it doesn't work when page poisoning is enabled. I did this because a write to the page after doing an MADV_DONTNEED would effectively negate the hint, so it would be wasting cycles to do so.

Again that's leaking host implementation detail into guest interface.

Quoted 8 lines

Quoted 3 lines

We are giving guest page hints to host that makes sense, weird interactions with other features due to host implementation details should be handled by host.

I don't view this as a host implementation detail, this is guest feature making use of all pages for debugging. If we are placing poison values in the page then I wouldn't consider them an unused page, it is being actively used to store the poison value.

Quoted 7 lines

Well I guess it's a valid point of view for a kernel hacker, but they are unused from application's point of view.
However poisoning is transparent to users and most distro users are not aware of it going on. They just know that debug kernels are slower.
User loading a debug kernel and immediately breaking overcommit is an unpleasant experience.

Quoted 2 lines

How would that be any different then a user loading an older kernel that doesn't have this feature and breaking overcommit as a result?

Well old kernel does not have the feature so nothing to debug. When we have a new feature that goes away in the debug kernel, that's a big support problem since this leads to heisenbugs.

Quoted 3 lines

Trying to debug host features from the guest would be a pain anyway as a guest shouldn't even really know what the underlying setup of the guest is supposed to be.

I'm talking about debugging the guest though.

Quoted 67 lines

Quoted 23 lines

Quoted 9 lines

I still think it would be better if we left the poisoning enabled in such a case and just displayed a warning message if nothing else that hinting is disabled because of page poisoning.

One other thought I had on this is that one side effect of page poisoning is probably that KSM would be able to merge all of the poison pages together into a single page since they are all set to the same values. So even with the poisoned pages it would be possible to reduce total memory overhead.

Right. And BTW one thing that host can do is pass
the hinted area to KSM for merging.
That requires an alloc hook to free it though.

Or we could add a per-VMA byte with the poison value and use that on host to populate pages on fault.

Quoted 5 lines

Quoted 5 lines

Quoted 4 lines

If we can achieve this
and free the page back to the host then even better, but until the features can coexist we should not use the page hinting while page poisoning is enabled.

Quoted 8 lines

Quoted 4 lines

Quoted 2 lines

Existing hinting in balloon allows them to coexist so I think we need to set the bar just as high for any new variant.

That is what I heard. I will have to look into this.

It's not doing anything smart right now, just checks that poison == 0 and skips freeing if not. But it can be enhanced transparently to guests.

Okay, so it probably should be extended to add something like poison page that could replace the zero page for reads to a page that has been unmapped.

Quoted 18 lines

Quoted 11 lines

Quoted 10 lines

Quoted 5 lines

This is one of the reasons why I was opposed to just disabling page poisoning when this feature was enabled in Nitesh's patches. If the guest has page poisoning enabled it is doing something with the page. It shouldn't be prevented from doing that because the host wants to have the option to free the pages.

I agree but I think the decision belongs on the host. I.e. hint the page but tell the host it needs to be careful about the poison value. It might also mean we
need to make sure poisoning happens after the hinting, not before.

Quoted 6 lines

The only issue with poisoning after instead of before is that the hint is ignored and we end up triggering a page fault and zero as a result. It might make more sense to have an architecture specific call that can be paravirtualized to handle the case of poisoning the page for us if we have the unused page hint enabled. Otherwise the write to the page is a given to invalidate the hint.

Quoted 6 lines

Sounds interesting. So the arch hook will first poison and then pass the page to the host?

Or we can also ask the host to poison for us, problem is this forces host to either always write into page, or call MADV_DONTNEED, without it could do MADV_FREE. Maybe that is not a big issue.

I would think we would ask the host to poison for us. If I am not mistaken both solutions right now are using MADV_DONTNEED. I would tend to lean that way if we are doing page poisoning since the cost for zeroing/poisoning the page on the host could be canceled out by dropping the page poisoning on the guest.

Quoted 3 lines

Then again since we are doing higher order pages only, and the poisoning is supposed to happen before we get into __free_one_page we would probably have to do both the poisoning, and the poison on fault.

Oh that's a nice trick. So in fact if we just make sure we never report PAGE_SIZE pages then poisoning will automatically happen before reporting?
So we just need to teach host to poison on fault. Sounds cool and we can always optimize further later.

Quoted 75 lines · Michael S. Tsirkin

Quoted 63 lines · Alexander Duyck

Quoted 54 lines · Michael S. Tsirkin

Quoted 48 lines · Alexander Duyck

Quoted 38 lines · Michael S. Tsirkin

Quoted 16 lines · Alexander Duyck

Quoted 9 lines · Michael S. Tsirkin

Quoted 6 lines · Alexander Duyck

From: Alexander Duyck <[email protected]>

Add guest support for providing free memory hints to the KVM hypervisor for freed pages huge TLB size or larger. I am restricting the size to huge TLB order and larger because the hypercalls are too expensive to be performing one per 4K page.

Even 2M pages start to get expensive with a TB guest.

Agreed.

Quoted 2 lines

Really it seems we want a virtio ring so we can pass a batch of these. E.g. 256 entries, 2M each - that's more like it.

Quoted 6 lines

The only issue I see with doing that is that we then have to defer the freeing. Doing that is going to introduce issues in the guest as we are going to have pages going unused for some period of time while we wait for the hint to complete, and we cannot just pull said pages back. I'm not really a fan of the asynchronous nature of Nitesh's patches for this reason.

Well nothing prevents us from doing an extra exit to the hypervisor if we want. The asynchronous nature is there as an optimization to allow hypervisor to do its thing on a separate CPU.
Why not proceed doing other things meanwhile?
And if the reason is that we are short on memory, then maybe we should be less aggressive in hinting?

E.g. if we just have 2 pages:

hint page 1
page 1 hint processed?
yes - proceed to page 2
no - wait for interrupt

Quoted 8 lines

get interrupt that page 1 hint is processed
hint page 2

If hypervisor happens to be running on same CPU it can process things synchronously and we never enter the no branch.

Quoted 4 lines

Another concern I would have about processing this asynchronously is that we have the potential for multiple guest CPUs to become bottlenecked by a single host CPU. I am not sure if that is something that would be desirable.

Quoted 7 lines

Well with a hypercall per page the fix is to block VCPU completely which is also not for everyone.

If you can't push a free page hint to host, then
ideally you just won't. That's a nice property of
hinting we have upstream right now.
Host too busy - hinting is just skipped.

Quoted 7 lines

Right, but if you do that then there is a potential to end up missing hints for a large portion of memory. It seems like you would end up with even bigger issues since then at that point you have essentially leaked memory.
I would think you would need a way to resync the host and the guest after something like that. Otherwise you can have memory that will just go unused for an extended period if a guest just goes idle.

Yes and that is my point. Existing hints code will just take a page off the free list in that case so it resyncs using the free list.

Quoted 6 lines

Something like this could work then: mark up hinted pages with a flag (its easy to find unused flags for free pages) then when you get an interrupt because outstanding hints have been consumed, get unflagged/unhinted pages from buddy and pass them to host.

Ugh. This is beginning to sound like yet another daemon that will have to be running to handle missed sync events.

I really think that taking an async approach for this will be nothing but trouble. You are going to have a difficult time maintaining any sort of coherency no the freelist without the daemon having to take the zone lock and then notify the host of what is free and what isn't.

Quoted 40 lines

Quoted 39 lines

Quoted 29 lines

Quoted 29 lines

Quoted 28 lines

Quoted 12 lines

Quoted 12 lines

Quoted 9 lines

Using the huge TLB order became the obvious
choice for the order to use as it allows us to avoid fragmentation of higher order memory on the host.

I have limited the functionality so that it doesn't work when page poisoning is enabled. I did this because a write to the page after doing an MADV_DONTNEED would effectively negate the hint, so it would be wasting cycles to do so.

Again that's leaking host implementation detail into guest interface.

Quoted 8 lines

Quoted 3 lines

We are giving guest page hints to host that makes sense, weird interactions with other features due to host implementation details should be handled by host.

I don't view this as a host implementation detail, this is guest feature making use of all pages for debugging. If we are placing poison values in the page then I wouldn't consider them an unused page, it is being actively used to store the poison value.

Well I guess it's a valid point of view for a kernel hacker, but they are unused from application's point of view.
However poisoning is transparent to users and most distro users are not aware of it going on. They just know that debug kernels are slower.
User loading a debug kernel and immediately breaking overcommit is an unpleasant experience.

Quoted 6 lines

Quoted 2 lines

How would that be any different then a user loading an older kernel that doesn't have this feature and breaking overcommit as a result?

Well old kernel does not have the feature so nothing to debug. When we have a new feature that goes away in the debug kernel, that's a big support problem since this leads to heisenbugs.

Trying to debug host features from the guest would be a pain anyway as a guest shouldn't even really know what the underlying setup of the guest is supposed to be.

Quoted 1 line

I'm talking about debugging the guest though.

Right. But my point is if it is a guest feature related to memory that you are debugging, then disabling the page hinting would probably be an advisable step anyway since it would have the potential for memory corruptions itself due to its nature.

Quoted 72 lines

Quoted 67 lines

Quoted 23 lines

Quoted 9 lines

I still think it would be better if we left the poisoning enabled in such a case and just displayed a warning message if nothing else that hinting is disabled because of page poisoning.

One other thought I had on this is that one side effect of page poisoning is probably that KSM would be able to merge all of the poison pages together into a single page since they are all set to the same values. So even with the poisoned pages it would be possible to reduce total memory overhead.

Right. And BTW one thing that host can do is pass
the hinted area to KSM for merging.
That requires an alloc hook to free it though.

Or we could add a per-VMA byte with the poison value and use that on host to populate pages on fault.

Quoted 5 lines

Quoted 5 lines

Quoted 4 lines

If we can achieve this
and free the page back to the host then even better, but until the features can coexist we should not use the page hinting while page poisoning is enabled.

Quoted 8 lines

Quoted 4 lines

Quoted 2 lines

Existing hinting in balloon allows them to coexist so I think we need to set the bar just as high for any new variant.

That is what I heard. I will have to look into this.

It's not doing anything smart right now, just checks that poison == 0 and skips freeing if not. But it can be enhanced transparently to guests.

Okay, so it probably should be extended to add something like poison page that could replace the zero page for reads to a page that has been unmapped.

Quoted 18 lines

Quoted 11 lines

Quoted 10 lines

Quoted 5 lines

This is one of the reasons why I was opposed to just disabling page poisoning when this feature was enabled in Nitesh's patches. If the guest has page poisoning enabled it is doing something with the page. It shouldn't be prevented from doing that because the host wants to have the option to free the pages.

I agree but I think the decision belongs on the host. I.e. hint the page but tell the host it needs to be careful about the poison value. It might also mean we
need to make sure poisoning happens after the hinting, not before.

Quoted 6 lines

The only issue with poisoning after instead of before is that the hint is ignored and we end up triggering a page fault and zero as a result. It might make more sense to have an architecture specific call that can be paravirtualized to handle the case of poisoning the page for us if we have the unused page hint enabled. Otherwise the write to the page is a given to invalidate the hint.

Quoted 6 lines

Sounds interesting. So the arch hook will first poison and then pass the page to the host?

Or we can also ask the host to poison for us, problem is this forces host to either always write into page, or call MADV_DONTNEED, without it could do MADV_FREE. Maybe that is not a big issue.

I would think we would ask the host to poison for us. If I am not mistaken both solutions right now are using MADV_DONTNEED. I would tend to lean that way if we are doing page poisoning since the cost for zeroing/poisoning the page on the host could be canceled out by dropping the page poisoning on the guest.

Quoted 3 lines

Then again since we are doing higher order pages only, and the poisoning is supposed to happen before we get into __free_one_page we would probably have to do both the poisoning, and the poison on fault.

Quoted 5 lines

Oh that's a nice trick. So in fact if we just make sure we never report PAGE_SIZE pages then poisoning will automatically happen before reporting?
So we just need to teach host to poison on fault. Sounds cool and we can always optimize further later.

That is kind of what I was thinking. In the grand scheme of things I figure most of the expense is in the fault and page zeroing bits of the code path. I have done a bit of testing today with the patch that just drops the messages if a device is assigned, and just the hypercall bits are only causing about a 2.5% regression in performance on a will-it- scale/page-fault1 test. However if I commit to the full setup with the madvise, page fault, and zeroing then I am seeing an 11.5% drop in performance.

I think in order to really make this pay off we may need to look into avoiding zeroing or poisoning the page in both the host and the guest. I will have to look into some things as it looks like there was somebody from Intel may have been working on doing some work to address that based on the presentation I found at the link below:

https://www.lfasiallc.com/wp-content/uploads/2017/11/Use-Hyper-V-Enlightenments-to-Increase-KVM-VM-Performance_Density_Chao-Peng.pdf

Quoted 6 lines · Alexander Duyck

From: Alexander Duyck <[email protected]>

Add guest support for providing free memory hints to the KVM hypervisor for freed pages huge TLB size or larger. I am restricting the size to huge TLB order and larger because the hypercalls are too expensive to be performing one per 4K page.

Even 2M pages start to get expensive with a TB guest.

Really it seems we want a virtio ring so we can pass a batch of these. E.g. 256 entries, 2M each - that's more like it.

Quoted 8 lines · Michael S. Tsirkin

Quoted 6 lines · Alexander Duyck

From: Alexander Duyck <[email protected]>

Add guest support for providing free memory hints to the KVM hypervisor for freed pages huge TLB size or larger. I am restricting the size to huge TLB order and larger because the hypercalls are too expensive to be performing one per 4K page.

Even 2M pages start to get expensive with a TB guest.

Yeah, but we don't allocate and free TB's of memory at a high frequency.

Quoted 2 lines

Really it seems we want a virtio ring so we can pass a batch of these. E.g. 256 entries, 2M each - that's more like it.

That only makes sense for a system that's doing high-frequency, discontiguous frees of 2M pages. Right now, a 2M free/realloc cycle (THP or hugetlb) is *not* super-high frequency just because of the latency for zeroing the page.

A virtio ring seems like an overblown solution to a non-existent problem.

Quoted 6 lines

Quoted 2 lines

Really it seems we want a virtio ring so we can pass a batch of these. E.g. 256 entries, 2M each - that's more like it.

That only makes sense for a system that's doing high-frequency, discontiguous frees of 2M pages. Right now, a 2M free/realloc cycle (THP or hugetlb) is *not* super-high frequency just because of the latency for zeroing the page.

Heh but with a ton of free memory, and a thread zeroing some of it out in the background, will this still be the case? It could be that we'll be able to find clean pages
at all times.

Quoted 1 line

A virtio ring seems like an overblown solution to a non-existent problem.

It would be nice to see some traces to help us decide one way or the other.

Quoted 10 lines · Michael S. Tsirkin

Quoted 6 lines

Quoted 2 lines

Really it seems we want a virtio ring so we can pass a batch of these. E.g. 256 entries, 2M each - that's more like it.

That only makes sense for a system that's doing high-frequency, discontiguous frees of 2M pages. Right now, a 2M free/realloc cycle (THP or hugetlb) is *not* super-high frequency just because of the latency for zeroing the page.

Heh but with a ton of free memory, and a thread zeroing some of it out in the background, will this still be the case? It could be that we'll be able to find clean pages
at all times.

In a systems where we have some asynchrounous zeroing of memory where freed, non-zeroed memory is sequestered out of the allocator, yeah, that could make sense.

But, that's not what we have today.

Quoted 2 lines

Quoted 1 line

A virtio ring seems like an overblown solution to a non-existent problem.

It would be nice to see some traces to help us decide one way or the other.

Yeah, agreed. Sounds like we need some more testing to see if these approaches hit bottlenecks anywhere.

arch/x86/kernel/kvm.c +17 -0

+void __arch_merge_page(struct zone *zone, struct page *page,
+		       unsigned int order)
+{
+	/*
+	 * The merging logic has merged a set of buddies up to the
+	 * KVM_PV_UNUSED_PAGE_HINT_MIN_ORDER. Since that is the case, take
+	 * advantage of this moment to notify the hypervisor of the free
+	 * memory.
+	 */
+	if (order != KVM_PV_UNUSED_PAGE_HINT_MIN_ORDER)
+		return;
+
+	/*
+	 * Drop zone lock while processing the hypercall. This
+	 * should be safe as the page has not yet been added
+	 * to the buddy list as of yet and all the pages that
+	 * were merged have had their buddy/guard flags cleared

Quoted 21 lines

arch/x86/kernel/kvm.c +21 -0

+void __arch_merge_page(struct zone *zone, struct page *page,
+		       unsigned int order)
+{
+	/*
+	 * The merging logic has merged a set of buddies up to the
+	 * KVM_PV_UNUSED_PAGE_HINT_MIN_ORDER. Since that is the case, take
+	 * advantage of this moment to notify the hypervisor of the free
+	 * memory.
+	 */
+	if (order != KVM_PV_UNUSED_PAGE_HINT_MIN_ORDER)
+		return;
+
+	/*
+	 * Drop zone lock while processing the hypercall. This
+	 * should be safe as the page has not yet been added
+	 * to the buddy list as of yet and all the pages that
+	 * were merged have had their buddy/guard flags cleared
+	 * and their order reset to 0.
+	 */
+	spin_unlock(&zone->lock);
+

arch/x86/kernel/kvm.c void __arch_free_page(struct page *page, unsigned int order) · line 814

Quoted 6 lines

arch/x86/kernel/kvm.c +6 -0

+	kvm_hypercall2(KVM_HC_UNUSED_PAGE_HINT, page_to_phys(page),
+		       PAGE_SIZE << order);
+
+	/* reacquire lock and resume freeing memory */
+	spin_lock(&zone->lock);
+}

Why do the lock-dropping on merge but not free? What's the difference?

This makes me really nervous. You at *least* want to document this at the arch_merge_page() call-site, and perhaps even the __free_one_page() call-sites because they're near where the zone lock is taken.

The place you are calling arch_merge_page() looks OK to me, today. But, it can't get moved around without careful consideration. That also needs to be documented to warn off folks who might move code around.

The interaction between the free and merge hooks is also really implementation-specific. If an architecture is getting order-0 arch_free_page() notifications, it's probably worth at least documenting that they'll *also* get arch_merge_page() notifications.

The reason x86 doesn't double-hypercall on those is not broached in the descriptions. That seems to be problematic.

From: Alexander Duyck <[email protected]>

Because the implementation was limiting itself to only providing hints on pages huge TLB order sized or larger we introduced the possibility for free pages to slip past us because they are freed as something less then huge TLB in size and aggregated with buddies later.

To address that I am adding a new call arch_merge_page which is called after __free_one_page has merged a pair of pages to create a higher order page. By doing this I am able to fill the gap and provide full coverage for all of the pages huge TLB order or larger.

Signed-off-by Alexander Duyck <[email protected]>

---
 arch/x86/include/asm/page.h |   12 ++++++++++++
 arch/x86/kernel/kvm.c       |   28 ++++++++++++++++++++++++++++
 include/linux/gfp.h         |    4 ++++
 mm/page_alloc.c             |    2 ++
 4 files changed, 46 insertions(+)

arch/x86/include/asm/page.h +12 -0

index 4487ad7a3385..9540a97c9997 100644
@@ -29,6 +29,18 @@ static inline void arch_free_page(struct page *page, unsigned int order)
 	if (static_branch_unlikely(&pv_free_page_hint_enabled))
 		__arch_free_page(page, order);
 }
+
+struct zone;
+
+#define HAVE_ARCH_MERGE_PAGE
+void __arch_merge_page(struct zone *zone, struct page *page,
+		       unsigned int order);
+static inline void arch_merge_page(struct zone *zone, struct page *page,
+				   unsigned int order)
+{
+	if (static_branch_unlikely(&pv_free_page_hint_enabled))
+		__arch_merge_page(zone, page, order);
+}
 #endif
 
 #include <linux/range.h>

arch/x86/kernel/kvm.c +28 -0

index 09c91641c36c..957bb4f427bb 100644
@@ -785,6 +785,34 @@ void __arch_free_page(struct page *page, unsigned int order)
 		       PAGE_SIZE << order);
 }
 
+void __arch_merge_page(struct zone *zone, struct page *page,
+		       unsigned int order)
+{
+	/*
+	 * The merging logic has merged a set of buddies up to the
+	 * KVM_PV_UNUSED_PAGE_HINT_MIN_ORDER. Since that is the case, take
+	 * advantage of this moment to notify the hypervisor of the free
+	 * memory.
+	 */
+	if (order != KVM_PV_UNUSED_PAGE_HINT_MIN_ORDER)
+		return;
+
+	/*
+	 * Drop zone lock while processing the hypercall. This
+	 * should be safe as the page has not yet been added
+	 * to the buddy list as of yet and all the pages that
+	 * were merged have had their buddy/guard flags cleared
+	 * and their order reset to 0.
+	 */
+	spin_unlock(&zone->lock);
+
+	kvm_hypercall2(KVM_HC_UNUSED_PAGE_HINT, page_to_phys(page),
+		       PAGE_SIZE << order);
+
+	/* reacquire lock and resume freeing memory */
+	spin_lock(&zone->lock);
+}
+
 #ifdef CONFIG_PARAVIRT_SPINLOCKS
 
 /* Kick a cpu by its apicid. Used to wake up a halted vcpu */

include/linux/gfp.h +4 -0

index fdab7de7490d..4746d5560193 100644
@@ -459,6 +459,10 @@ static inline struct zonelist *node_zonelist(int nid, gfp_t flags)
 #ifndef HAVE_ARCH_FREE_PAGE
 static inline void arch_free_page(struct page *page, int order) { }
 #endif
+#ifndef HAVE_ARCH_MERGE_PAGE
+static inline void
+arch_merge_page(struct zone *zone, struct page *page, int order) { }
+#endif
 #ifndef HAVE_ARCH_ALLOC_PAGE
 static inline void arch_alloc_page(struct page *page, int order) { }
 #endif

mm/page_alloc.c +2 -0

index c954f8c1fbc4..7a1309b0b7c5 100644
@@ -913,6 +913,8 @@ static inline void __free_one_page(struct page *page,
 		page = page + (combined_pfn - pfn);
 		pfn = combined_pfn;
 		order++;
+
+		arch_merge_page(zone, page, order);
 	}
 	if (max_order < MAX_ORDER) {
 		/* If we are here, it means order is >= pageblock_order.

Quoted 7 lines · Alexander Duyck

From: Alexander Duyck <[email protected]>

Because the implementation was limiting itself to only providing hints on pages huge TLB order sized or larger we introduced the possibility for free pages to slip past us because they are freed as something less then huge TLB in size and aggregated with buddies later.

Quoted 6 lines

To address that I am adding a new call arch_merge_page which is called after __free_one_page has merged a pair of pages to create a higher order page. By doing this I am able to fill the gap and provide full coverage for all of the pages huge TLB order or larger.

Signed-off-by Alexander Duyck <[email protected]>

Looks like this will be helpful whenever active free page hints are added. So I think it's a good idea to add a hook.

However, could you split adding the hook to a separate patch from the KVM hypercall based implementation?

Then e.g. Nilal's patches could reuse it too.

Quoted 99 lines

---
 arch/x86/include/asm/page.h |   12 ++++++++++++
 arch/x86/kernel/kvm.c       |   28 ++++++++++++++++++++++++++++
 include/linux/gfp.h         |    4 ++++
 mm/page_alloc.c             |    2 ++
 4 files changed, 46 insertions(+)

arch/x86/include/asm/page.h +12 -0

index 4487ad7a3385..9540a97c9997 100644
@@ -29,6 +29,18 @@ static inline void arch_free_page(struct page *page, unsigned int order)
 	if (static_branch_unlikely(&pv_free_page_hint_enabled))
 		__arch_free_page(page, order);
 }
+
+struct zone;
+
+#define HAVE_ARCH_MERGE_PAGE
+void __arch_merge_page(struct zone *zone, struct page *page,
+		       unsigned int order);
+static inline void arch_merge_page(struct zone *zone, struct page *page,
+				   unsigned int order)
+{
+	if (static_branch_unlikely(&pv_free_page_hint_enabled))
+		__arch_merge_page(zone, page, order);
+}
 #endif
 
 #include <linux/range.h>

arch/x86/kernel/kvm.c +28 -0

index 09c91641c36c..957bb4f427bb 100644
@@ -785,6 +785,34 @@ void __arch_free_page(struct page *page, unsigned int order)
 		       PAGE_SIZE << order);
 }
 
+void __arch_merge_page(struct zone *zone, struct page *page,
+		       unsigned int order)
+{
+	/*
+	 * The merging logic has merged a set of buddies up to the
+	 * KVM_PV_UNUSED_PAGE_HINT_MIN_ORDER. Since that is the case, take
+	 * advantage of this moment to notify the hypervisor of the free
+	 * memory.
+	 */
+	if (order != KVM_PV_UNUSED_PAGE_HINT_MIN_ORDER)
+		return;
+
+	/*
+	 * Drop zone lock while processing the hypercall. This
+	 * should be safe as the page has not yet been added
+	 * to the buddy list as of yet and all the pages that
+	 * were merged have had their buddy/guard flags cleared
+	 * and their order reset to 0.
+	 */
+	spin_unlock(&zone->lock);
+
+	kvm_hypercall2(KVM_HC_UNUSED_PAGE_HINT, page_to_phys(page),
+		       PAGE_SIZE << order);
+
+	/* reacquire lock and resume freeing memory */
+	spin_lock(&zone->lock);
+}
+
 #ifdef CONFIG_PARAVIRT_SPINLOCKS
 
 /* Kick a cpu by its apicid. Used to wake up a halted vcpu */

include/linux/gfp.h +4 -0

index fdab7de7490d..4746d5560193 100644
@@ -459,6 +459,10 @@ static inline struct zonelist *node_zonelist(int nid, gfp_t flags)
 #ifndef HAVE_ARCH_FREE_PAGE
 static inline void arch_free_page(struct page *page, int order) { }
 #endif
+#ifndef HAVE_ARCH_MERGE_PAGE
+static inline void
+arch_merge_page(struct zone *zone, struct page *page, int order) { }
+#endif
 #ifndef HAVE_ARCH_ALLOC_PAGE
 static inline void arch_alloc_page(struct page *page, int order) { }
 #endif

mm/page_alloc.c +2 -0

index c954f8c1fbc4..7a1309b0b7c5 100644
@@ -913,6 +913,8 @@ static inline void __free_one_page(struct page *page,
 		page = page + (combined_pfn - pfn);
 		pfn = combined_pfn;
 		order++;
+
+		arch_merge_page(zone, page, order);
 	}
 	if (max_order < MAX_ORDER) {
 		/* If we are here, it means order is >= pageblock_order.

Quoted 21 lines · Michael S. Tsirkin

Quoted 7 lines · Alexander Duyck

From: Alexander Duyck <[email protected]>

Because the implementation was limiting itself to only providing hints on pages huge TLB order sized or larger we introduced the possibility for free pages to slip past us because they are freed as something less then huge TLB in size and aggregated with buddies later.

Quoted 6 lines

To address that I am adding a new call arch_merge_page which is called after __free_one_page has merged a pair of pages to create a higher order page. By doing this I am able to fill the gap and provide full coverage for all of the pages huge TLB order or larger.

Signed-off-by Alexander Duyck <[email protected]>

Looks like this will be helpful whenever active free page hints are added. So I think it's a good idea to add a hook.

However, could you split adding the hook to a separate patch from the KVM hypercall based implementation?

Quoted 1 line

Then e.g. Nilal's patches could reuse it too.

With the current design of my patch-set, if I use this hook to report free pages. I will end up making redundant hints for the same pfns.

This is because the pages once freed by the host, are returned back to the buddy.

Quoted 99 lines

Quoted 99 lines

---
 arch/x86/include/asm/page.h |   12 ++++++++++++
 arch/x86/kernel/kvm.c       |   28 ++++++++++++++++++++++++++++
 include/linux/gfp.h         |    4 ++++
 mm/page_alloc.c             |    2 ++
 4 files changed, 46 insertions(+)

arch/x86/include/asm/page.h +12 -0

index 4487ad7a3385..9540a97c9997 100644
@@ -29,6 +29,18 @@ static inline void arch_free_page(struct page *page, unsigned int order)
 	if (static_branch_unlikely(&pv_free_page_hint_enabled))
 		__arch_free_page(page, order);
 }
+
+struct zone;
+
+#define HAVE_ARCH_MERGE_PAGE
+void __arch_merge_page(struct zone *zone, struct page *page,
+		       unsigned int order);
+static inline void arch_merge_page(struct zone *zone, struct page *page,
+				   unsigned int order)
+{
+	if (static_branch_unlikely(&pv_free_page_hint_enabled))
+		__arch_merge_page(zone, page, order);
+}
 #endif
 
 #include <linux/range.h>

arch/x86/kernel/kvm.c +28 -0

index 09c91641c36c..957bb4f427bb 100644
@@ -785,6 +785,34 @@ void __arch_free_page(struct page *page, unsigned int order)
 		       PAGE_SIZE << order);
 }
 
+void __arch_merge_page(struct zone *zone, struct page *page,
+		       unsigned int order)
+{
+	/*
+	 * The merging logic has merged a set of buddies up to the
+	 * KVM_PV_UNUSED_PAGE_HINT_MIN_ORDER. Since that is the case, take
+	 * advantage of this moment to notify the hypervisor of the free
+	 * memory.
+	 */
+	if (order != KVM_PV_UNUSED_PAGE_HINT_MIN_ORDER)
+		return;
+
+	/*
+	 * Drop zone lock while processing the hypercall. This
+	 * should be safe as the page has not yet been added
+	 * to the buddy list as of yet and all the pages that
+	 * were merged have had their buddy/guard flags cleared
+	 * and their order reset to 0.
+	 */
+	spin_unlock(&zone->lock);
+
+	kvm_hypercall2(KVM_HC_UNUSED_PAGE_HINT, page_to_phys(page),
+		       PAGE_SIZE << order);
+
+	/* reacquire lock and resume freeing memory */
+	spin_lock(&zone->lock);
+}
+
 #ifdef CONFIG_PARAVIRT_SPINLOCKS
 
 /* Kick a cpu by its apicid. Used to wake up a halted vcpu */

include/linux/gfp.h +4 -0

index fdab7de7490d..4746d5560193 100644
@@ -459,6 +459,10 @@ static inline struct zonelist *node_zonelist(int nid, gfp_t flags)
 #ifndef HAVE_ARCH_FREE_PAGE
 static inline void arch_free_page(struct page *page, int order) { }
 #endif
+#ifndef HAVE_ARCH_MERGE_PAGE
+static inline void
+arch_merge_page(struct zone *zone, struct page *page, int order) { }
+#endif
 #ifndef HAVE_ARCH_ALLOC_PAGE
 static inline void arch_alloc_page(struct page *page, int order) { }
 #endif

mm/page_alloc.c +2 -0

index c954f8c1fbc4..7a1309b0b7c5 100644
@@ -913,6 +913,8 @@ static inline void __free_one_page(struct page *page,
 		page = page + (combined_pfn - pfn);
 		pfn = combined_pfn;
 		order++;
+
+		arch_merge_page(zone, page, order);
 	}
 	if (max_order < MAX_ORDER) {
 		/* If we are here, it means order is >= pageblock_order.

Signature · 2 lines

Regards
Nitesh

Quoted 22 lines · Nitesh Narayan Lal

Quoted 21 lines · Michael S. Tsirkin

Quoted 7 lines · Alexander Duyck

From: Alexander Duyck <[email protected]>

Because the implementation was limiting itself to only providing hints on pages huge TLB order sized or larger we introduced the possibility for free pages to slip past us because they are freed as something less then huge TLB in size and aggregated with buddies later.

Quoted 6 lines

To address that I am adding a new call arch_merge_page which is called after __free_one_page has merged a pair of pages to create a higher order page. By doing this I am able to fill the gap and provide full coverage for all of the pages huge TLB order or larger.

Signed-off-by Alexander Duyck <[email protected]>

Looks like this will be helpful whenever active free page hints are added. So I think it's a good idea to add a hook.

However, could you split adding the hook to a separate patch from the KVM hypercall based implementation?

Quoted 6 lines

Quoted 1 line

Then e.g. Nilal's patches could reuse it too.

With the current design of my patch-set, if I use this hook to report free pages. I will end up making redundant hints for the same pfns.

This is because the pages once freed by the host, are returned back to the buddy.

Suggestions on how you'd like to fix this? You do need this if you introduce a size cut-off right?

Quoted 103 lines

Quoted 99 lines

Quoted 99 lines

---
 arch/x86/include/asm/page.h |   12 ++++++++++++
 arch/x86/kernel/kvm.c       |   28 ++++++++++++++++++++++++++++
 include/linux/gfp.h         |    4 ++++
 mm/page_alloc.c             |    2 ++
 4 files changed, 46 insertions(+)

arch/x86/include/asm/page.h +12 -0

index 4487ad7a3385..9540a97c9997 100644
@@ -29,6 +29,18 @@ static inline void arch_free_page(struct page *page, unsigned int order)
 	if (static_branch_unlikely(&pv_free_page_hint_enabled))
 		__arch_free_page(page, order);
 }
+
+struct zone;
+
+#define HAVE_ARCH_MERGE_PAGE
+void __arch_merge_page(struct zone *zone, struct page *page,
+		       unsigned int order);
+static inline void arch_merge_page(struct zone *zone, struct page *page,
+				   unsigned int order)
+{
+	if (static_branch_unlikely(&pv_free_page_hint_enabled))
+		__arch_merge_page(zone, page, order);
+}
 #endif
 
 #include <linux/range.h>

arch/x86/kernel/kvm.c +28 -0

index 09c91641c36c..957bb4f427bb 100644
@@ -785,6 +785,34 @@ void __arch_free_page(struct page *page, unsigned int order)
 		       PAGE_SIZE << order);
 }
 
+void __arch_merge_page(struct zone *zone, struct page *page,
+		       unsigned int order)
+{
+	/*
+	 * The merging logic has merged a set of buddies up to the
+	 * KVM_PV_UNUSED_PAGE_HINT_MIN_ORDER. Since that is the case, take
+	 * advantage of this moment to notify the hypervisor of the free
+	 * memory.
+	 */
+	if (order != KVM_PV_UNUSED_PAGE_HINT_MIN_ORDER)
+		return;
+
+	/*
+	 * Drop zone lock while processing the hypercall. This
+	 * should be safe as the page has not yet been added
+	 * to the buddy list as of yet and all the pages that
+	 * were merged have had their buddy/guard flags cleared
+	 * and their order reset to 0.
+	 */
+	spin_unlock(&zone->lock);
+
+	kvm_hypercall2(KVM_HC_UNUSED_PAGE_HINT, page_to_phys(page),
+		       PAGE_SIZE << order);
+
+	/* reacquire lock and resume freeing memory */
+	spin_lock(&zone->lock);
+}
+
 #ifdef CONFIG_PARAVIRT_SPINLOCKS
 
 /* Kick a cpu by its apicid. Used to wake up a halted vcpu */

include/linux/gfp.h +4 -0

index fdab7de7490d..4746d5560193 100644
@@ -459,6 +459,10 @@ static inline struct zonelist *node_zonelist(int nid, gfp_t flags)
 #ifndef HAVE_ARCH_FREE_PAGE
 static inline void arch_free_page(struct page *page, int order) { }
 #endif
+#ifndef HAVE_ARCH_MERGE_PAGE
+static inline void
+arch_merge_page(struct zone *zone, struct page *page, int order) { }
+#endif
 #ifndef HAVE_ARCH_ALLOC_PAGE
 static inline void arch_alloc_page(struct page *page, int order) { }
 #endif

mm/page_alloc.c +2 -0

index c954f8c1fbc4..7a1309b0b7c5 100644
@@ -913,6 +913,8 @@ static inline void __free_one_page(struct page *page,
 		page = page + (combined_pfn - pfn);
 		pfn = combined_pfn;
 		order++;
+
+		arch_merge_page(zone, page, order);
 	}
 	if (max_order < MAX_ORDER) {
 		/* If we are here, it means order is >= pageblock_order.

Signature · 2 lines

Regards
Nitesh

Quoted 29 lines · Michael S. Tsirkin

Quoted 28 lines · Nitesh Narayan Lal

Quoted 14 lines · Michael S. Tsirkin

Quoted 7 lines · Alexander Duyck

From: Alexander Duyck <[email protected]>

Because the implementation was limiting itself to only providing hints on pages huge TLB order sized or larger we introduced the possibility for free pages to slip past us because they are freed as something less then huge TLB in size and aggregated with buddies later.

Quoted 6 lines

To address that I am adding a new call arch_merge_page which is called after __free_one_page has merged a pair of pages to create a higher order page. By doing this I am able to fill the gap and provide full coverage for all of the pages huge TLB order or larger.

Signed-off-by Alexander Duyck <[email protected]>

Quoted 8 lines

Looks like this will be helpful whenever active free page hints are added. So I think it's a good idea to add a hook.

However, could you split adding the hook to a separate patch from the KVM hypercall based implementation?

Then e.g. Nilal's patches could reuse it too.

With the current design of my patch-set, if I use this hook to report free pages. I will end up making redundant hints for the same pfns.

This is because the pages once freed by the host, are returned back to the buddy.

Quoted 2 lines

Suggestions on how you'd like to fix this? You do need this if you introduce a size cut-off right?

I do, there are two ways to go about it.

One is to  use this and have a flag in the page structure indicating whether that page has been freed/used or not.

Though I am not sure if
this will be acceptable upstream.
Second is to find another place to invoke guest_free_page() post buddy merging.

Quoted 105 lines

Quoted 103 lines

Quoted 99 lines

Quoted 99 lines

---
 arch/x86/include/asm/page.h |   12 ++++++++++++
 arch/x86/kernel/kvm.c       |   28 ++++++++++++++++++++++++++++
 include/linux/gfp.h         |    4 ++++
 mm/page_alloc.c             |    2 ++
 4 files changed, 46 insertions(+)

arch/x86/include/asm/page.h +12 -0

index 4487ad7a3385..9540a97c9997 100644
@@ -29,6 +29,18 @@ static inline void arch_free_page(struct page *page, unsigned int order)
 	if (static_branch_unlikely(&pv_free_page_hint_enabled))
 		__arch_free_page(page, order);
 }
+
+struct zone;
+
+#define HAVE_ARCH_MERGE_PAGE
+void __arch_merge_page(struct zone *zone, struct page *page,
+		       unsigned int order);
+static inline void arch_merge_page(struct zone *zone, struct page *page,
+				   unsigned int order)
+{
+	if (static_branch_unlikely(&pv_free_page_hint_enabled))
+		__arch_merge_page(zone, page, order);
+}
 #endif
 
 #include <linux/range.h>

arch/x86/kernel/kvm.c +28 -0

index 09c91641c36c..957bb4f427bb 100644
@@ -785,6 +785,34 @@ void __arch_free_page(struct page *page, unsigned int order)
 		       PAGE_SIZE << order);
 }
 
+void __arch_merge_page(struct zone *zone, struct page *page,
+		       unsigned int order)
+{
+	/*
+	 * The merging logic has merged a set of buddies up to the
+	 * KVM_PV_UNUSED_PAGE_HINT_MIN_ORDER. Since that is the case, take
+	 * advantage of this moment to notify the hypervisor of the free
+	 * memory.
+	 */
+	if (order != KVM_PV_UNUSED_PAGE_HINT_MIN_ORDER)
+		return;
+
+	/*
+	 * Drop zone lock while processing the hypercall. This
+	 * should be safe as the page has not yet been added
+	 * to the buddy list as of yet and all the pages that
+	 * were merged have had their buddy/guard flags cleared
+	 * and their order reset to 0.
+	 */
+	spin_unlock(&zone->lock);
+
+	kvm_hypercall2(KVM_HC_UNUSED_PAGE_HINT, page_to_phys(page),
+		       PAGE_SIZE << order);
+
+	/* reacquire lock and resume freeing memory */
+	spin_lock(&zone->lock);
+}
+
 #ifdef CONFIG_PARAVIRT_SPINLOCKS
 
 /* Kick a cpu by its apicid. Used to wake up a halted vcpu */

include/linux/gfp.h +4 -0

index fdab7de7490d..4746d5560193 100644
@@ -459,6 +459,10 @@ static inline struct zonelist *node_zonelist(int nid, gfp_t flags)
 #ifndef HAVE_ARCH_FREE_PAGE
 static inline void arch_free_page(struct page *page, int order) { }
 #endif
+#ifndef HAVE_ARCH_MERGE_PAGE
+static inline void
+arch_merge_page(struct zone *zone, struct page *page, int order) { }
+#endif
 #ifndef HAVE_ARCH_ALLOC_PAGE
 static inline void arch_alloc_page(struct page *page, int order) { }
 #endif

mm/page_alloc.c +2 -0

index c954f8c1fbc4..7a1309b0b7c5 100644
@@ -913,6 +913,8 @@ static inline void __free_one_page(struct page *page,
 		page = page + (combined_pfn - pfn);
 		pfn = combined_pfn;
 		order++;
+
+		arch_merge_page(zone, page, order);
 	}
 	if (max_order < MAX_ORDER) {
 		/* If we are here, it means order is >= pageblock_order.

Signature · 2 lines

Regards
Nitesh

Signature · 2 lines

Regards
Nitesh

Quoted 30 lines · Nitesh Narayan Lal

Quoted 29 lines · Michael S. Tsirkin

Quoted 28 lines · Nitesh Narayan Lal

Quoted 14 lines · Michael S. Tsirkin

Quoted 7 lines · Alexander Duyck

From: Alexander Duyck <[email protected]>

Because the implementation was limiting itself to only providing hints on pages huge TLB order sized or larger we introduced the possibility for free pages to slip past us because they are freed as something less then huge TLB in size and aggregated with buddies later.

Quoted 6 lines

To address that I am adding a new call arch_merge_page which is called after __free_one_page has merged a pair of pages to create a higher order page. By doing this I am able to fill the gap and provide full coverage for all of the pages huge TLB order or larger.

Signed-off-by Alexander Duyck <[email protected]>

Quoted 8 lines

Looks like this will be helpful whenever active free page hints are added. So I think it's a good idea to add a hook.

However, could you split adding the hook to a separate patch from the KVM hypercall based implementation?

Then e.g. Nilal's patches could reuse it too.

With the current design of my patch-set, if I use this hook to report free pages. I will end up making redundant hints for the same pfns.

This is because the pages once freed by the host, are returned back to the buddy.

Quoted 6 lines

Quoted 2 lines

Suggestions on how you'd like to fix this? You do need this if you introduce a size cut-off right?

I do, there are two ways to go about it.

One is to  use this and have a flag in the page structure indicating whether that page has been freed/used or not.

Not sure what do you mean. The refcount does this right?

Quoted 5 lines

Though I am not sure if
this will be acceptable upstream.
Second is to find another place to invoke guest_free_page() post buddy merging.

Might be easier.

Quoted 106 lines

Quoted 102 lines

Quoted 101 lines

Quoted 97 lines

Quoted 97 lines

---
 arch/x86/include/asm/page.h |   12 ++++++++++++
 arch/x86/kernel/kvm.c       |   28 ++++++++++++++++++++++++++++
 include/linux/gfp.h         |    4 ++++
 mm/page_alloc.c             |    2 ++
 4 files changed, 46 insertions(+)

arch/x86/include/asm/page.h +12 -0

index 4487ad7a3385..9540a97c9997 100644
@@ -29,6 +29,18 @@ static inline void arch_free_page(struct page *page, unsigned int order)
 	if (static_branch_unlikely(&pv_free_page_hint_enabled))
 		__arch_free_page(page, order);
 }
+
+struct zone;
+
+#define HAVE_ARCH_MERGE_PAGE
+void __arch_merge_page(struct zone *zone, struct page *page,
+		       unsigned int order);
+static inline void arch_merge_page(struct zone *zone, struct page *page,
+				   unsigned int order)
+{
+	if (static_branch_unlikely(&pv_free_page_hint_enabled))
+		__arch_merge_page(zone, page, order);
+}
 #endif
 
 #include <linux/range.h>

arch/x86/kernel/kvm.c +28 -0

index 09c91641c36c..957bb4f427bb 100644
@@ -785,6 +785,34 @@ void __arch_free_page(struct page *page, unsigned int order)
 		       PAGE_SIZE << order);
 }
 
+void __arch_merge_page(struct zone *zone, struct page *page,
+		       unsigned int order)
+{
+	/*
+	 * The merging logic has merged a set of buddies up to the
+	 * KVM_PV_UNUSED_PAGE_HINT_MIN_ORDER. Since that is the case, take
+	 * advantage of this moment to notify the hypervisor of the free
+	 * memory.
+	 */
+	if (order != KVM_PV_UNUSED_PAGE_HINT_MIN_ORDER)
+		return;
+
+	/*
+	 * Drop zone lock while processing the hypercall. This
+	 * should be safe as the page has not yet been added
+	 * to the buddy list as of yet and all the pages that
+	 * were merged have had their buddy/guard flags cleared
+	 * and their order reset to 0.
+	 */
+	spin_unlock(&zone->lock);
+
+	kvm_hypercall2(KVM_HC_UNUSED_PAGE_HINT, page_to_phys(page),
+		       PAGE_SIZE << order);
+
+	/* reacquire lock and resume freeing memory */
+	spin_lock(&zone->lock);
+}
+
 #ifdef CONFIG_PARAVIRT_SPINLOCKS
 
 /* Kick a cpu by its apicid. Used to wake up a halted vcpu */

include/linux/gfp.h +4 -0

index fdab7de7490d..4746d5560193 100644
@@ -459,6 +459,10 @@ static inline struct zonelist *node_zonelist(int nid, gfp_t flags)
 #ifndef HAVE_ARCH_FREE_PAGE
 static inline void arch_free_page(struct page *page, int order) { }
 #endif
+#ifndef HAVE_ARCH_MERGE_PAGE
+static inline void
+arch_merge_page(struct zone *zone, struct page *page, int order) { }
+#endif
 #ifndef HAVE_ARCH_ALLOC_PAGE
 static inline void arch_alloc_page(struct page *page, int order) { }
 #endif

mm/page_alloc.c +2 -0

index c954f8c1fbc4..7a1309b0b7c5 100644
@@ -913,6 +913,8 @@ static inline void __free_one_page(struct page *page,
 		page = page + (combined_pfn - pfn);
 		pfn = combined_pfn;
 		order++;
+
+		arch_merge_page(zone, page, order);
 	}
 	if (max_order < MAX_ORDER) {
 		/* If we are here, it means order is >= pageblock_order.

Signature · 2 lines

Regards
Nitesh

Signature · 2 lines

Regards
Nitesh

From: Alexander Duyck <[email protected]>

Because the implementation was limiting itself to only providing hints on pages huge TLB order sized or larger we introduced the possibility for free pages to slip past us because they are freed as something less then huge TLB in size and aggregated with buddies later.

To address that I am adding a new call arch_merge_page which is called after __free_one_page has merged a pair of pages to create a higher order page. By doing this I am able to fill the gap and provide full coverage for all of the pages huge TLB order or larger.

Signed-off-by Alexander Duyck <[email protected]>

---
 arch/x86/include/asm/page.h |   12 ++++++++++++
 arch/x86/kernel/kvm.c       |   28 ++++++++++++++++++++++++++++
 include/linux/gfp.h         |    4 ++++
 mm/page_alloc.c             |    2 ++
 4 files changed, 46 insertions(+)

arch/x86/include/asm/page.h +12 -0

index 4487ad7a3385..9540a97c9997 100644
@@ -29,6 +29,18 @@ static inline void arch_free_page(struct page *page, unsigned int order)
 	if (static_branch_unlikely(&pv_free_page_hint_enabled))
 		__arch_free_page(page, order);
 }
+
+struct zone;
+
+#define HAVE_ARCH_MERGE_PAGE
+void __arch_merge_page(struct zone *zone, struct page *page,
+		       unsigned int order);
+static inline void arch_merge_page(struct zone *zone, struct page *page,
+				   unsigned int order)
+{
+	if (static_branch_unlikely(&pv_free_page_hint_enabled))
+		__arch_merge_page(zone, page, order);
+}
 #endif
 
 #include <linux/range.h>

arch/x86/kernel/kvm.c +28 -0

index 09c91641c36c..957bb4f427bb 100644
@@ -785,6 +785,34 @@ void __arch_free_page(struct page *page, unsigned int order)
 		       PAGE_SIZE << order);
 }
 
+void __arch_merge_page(struct zone *zone, struct page *page,
+		       unsigned int order)
+{
+	/*
+	 * The merging logic has merged a set of buddies up to the
+	 * KVM_PV_UNUSED_PAGE_HINT_MIN_ORDER. Since that is the case, take
+	 * advantage of this moment to notify the hypervisor of the free
+	 * memory.
+	 */
+	if (order != KVM_PV_UNUSED_PAGE_HINT_MIN_ORDER)
+		return;
+
+	/*
+	 * Drop zone lock while processing the hypercall. This
+	 * should be safe as the page has not yet been added
+	 * to the buddy list as of yet and all the pages that
+	 * were merged have had their buddy/guard flags cleared
+	 * and their order reset to 0.
+	 */
+	spin_unlock(&zone->lock);
+
+	kvm_hypercall2(KVM_HC_UNUSED_PAGE_HINT, page_to_phys(page),
+		       PAGE_SIZE << order);
+
+	/* reacquire lock and resume freeing memory */
+	spin_lock(&zone->lock);
+}
+
 #ifdef CONFIG_PARAVIRT_SPINLOCKS
 
 /* Kick a cpu by its apicid. Used to wake up a halted vcpu */

include/linux/gfp.h +4 -0

index fdab7de7490d..4746d5560193 100644
@@ -459,6 +459,10 @@ static inline struct zonelist *node_zonelist(int nid, gfp_t flags)
 #ifndef HAVE_ARCH_FREE_PAGE
 static inline void arch_free_page(struct page *page, int order) { }
 #endif
+#ifndef HAVE_ARCH_MERGE_PAGE
+static inline void
+arch_merge_page(struct zone *zone, struct page *page, int order) { }
+#endif
 #ifndef HAVE_ARCH_ALLOC_PAGE
 static inline void arch_alloc_page(struct page *page, int order) { }
 #endif

mm/page_alloc.c +0 -0

index c954f8c1fbc4..7a1309b0b7c5 100644

mm/page_alloc.c +0 -0

 	}
 	if (max_order < MAX_ORDER) {
 		/* If we are here, it means order is >= pageblock_order.

Quoted 102 lines · Alexander Duyck

From: Alexander Duyck <[email protected]>

Because the implementation was limiting itself to only providing hints on pages huge TLB order sized or larger we introduced the possibility for free pages to slip past us because they are freed as something less then huge TLB in size and aggregated with buddies later.

To address that I am adding a new call arch_merge_page which is called after __free_one_page has merged a pair of pages to create a higher order page. By doing this I am able to fill the gap and provide full coverage for all of the pages huge TLB order or larger.

Signed-off-by Alexander Duyck <[email protected]>

---
 arch/x86/include/asm/page.h |   12 ++++++++++++
 arch/x86/kernel/kvm.c       |   28 ++++++++++++++++++++++++++++
 include/linux/gfp.h         |    4 ++++
 mm/page_alloc.c             |    2 ++
 4 files changed, 46 insertions(+)

arch/x86/include/asm/page.h +12 -0

index 4487ad7a3385..9540a97c9997 100644
@@ -29,6 +29,18 @@ static inline void arch_free_page(struct page *page, unsigned int order)
 	if (static_branch_unlikely(&pv_free_page_hint_enabled))
 		__arch_free_page(page, order);
 }
+
+struct zone;
+
+#define HAVE_ARCH_MERGE_PAGE
+void __arch_merge_page(struct zone *zone, struct page *page,
+		       unsigned int order);
+static inline void arch_merge_page(struct zone *zone, struct page *page,
+				   unsigned int order)
+{
+	if (static_branch_unlikely(&pv_free_page_hint_enabled))
+		__arch_merge_page(zone, page, order);
+}
 #endif
 
 #include <linux/range.h>

arch/x86/kernel/kvm.c +28 -0

index 09c91641c36c..957bb4f427bb 100644
@@ -785,6 +785,34 @@ void __arch_free_page(struct page *page, unsigned int order)
 		       PAGE_SIZE << order);
 }
 
+void __arch_merge_page(struct zone *zone, struct page *page,
+		       unsigned int order)
+{
+	/*
+	 * The merging logic has merged a set of buddies up to the
+	 * KVM_PV_UNUSED_PAGE_HINT_MIN_ORDER. Since that is the case, take
+	 * advantage of this moment to notify the hypervisor of the free
+	 * memory.
+	 */
+	if (order != KVM_PV_UNUSED_PAGE_HINT_MIN_ORDER)
+		return;
+
+	/*
+	 * Drop zone lock while processing the hypercall. This
+	 * should be safe as the page has not yet been added
+	 * to the buddy list as of yet and all the pages that
+	 * were merged have had their buddy/guard flags cleared
+	 * and their order reset to 0.
+	 */
+	spin_unlock(&zone->lock);
+
+	kvm_hypercall2(KVM_HC_UNUSED_PAGE_HINT, page_to_phys(page),
+		       PAGE_SIZE << order);
+
+	/* reacquire lock and resume freeing memory */
+	spin_lock(&zone->lock);
+}
+
 #ifdef CONFIG_PARAVIRT_SPINLOCKS
 
 /* Kick a cpu by its apicid. Used to wake up a halted vcpu */

include/linux/gfp.h +4 -0

index fdab7de7490d..4746d5560193 100644
@@ -459,6 +459,10 @@ static inline struct zonelist *node_zonelist(int nid, gfp_t flags)
 #ifndef HAVE_ARCH_FREE_PAGE
 static inline void arch_free_page(struct page *page, int order) { }
 #endif
+#ifndef HAVE_ARCH_MERGE_PAGE
+static inline void
+arch_merge_page(struct zone *zone, struct page *page, int order) { }
+#endif
 #ifndef HAVE_ARCH_ALLOC_PAGE
 static inline void arch_alloc_page(struct page *page, int order) { }
 #endif

mm/page_alloc.c +0 -0

index c954f8c1fbc4..7a1309b0b7c5 100644

mm/page_alloc.c static inline void __free_one_page(struct page *page, · line 917

Quoted 5 lines

mm/page_alloc.c +2 -0

@@ -913,6 +913,8 @@ static inline void __free_one_page(struct page *page,
 		page = page + (combined_pfn - pfn);
 		pfn = combined_pfn;
 		order++;
+
+		arch_merge_page(zone, page, order);

Not a proper place AFAICS.

Assume we have an order-8 page being sent here for merge and its order-8 buddy is also free, then order++ became 9 and arch_merge_page() will do the hint to host on this page as an order-9 page, no problem so far. Then the next round, assume the now order-9 page's buddy is also free, order++ will become 10 and arch_merge_page() will again hint to host on this page as an order-10 page. The first hint to host became redundant.

I think the proper place is after the done_merging tag.

BTW, with arch_merge_page() at the proper place, I don't think patch3/4 is necessary - any freed page will go through merge anyway, we won't lose any hint opportunity. Or do I miss anything?

Quoted 111 lines · Aaron Lu

Quoted 102 lines · Alexander Duyck

From: Alexander Duyck <[email protected]>

Because the implementation was limiting itself to only providing hints on pages huge TLB order sized or larger we introduced the possibility for free pages to slip past us because they are freed as something less then huge TLB in size and aggregated with buddies later.

To address that I am adding a new call arch_merge_page which is called after __free_one_page has merged a pair of pages to create a higher order page. By doing this I am able to fill the gap and provide full coverage for all of the pages huge TLB order or larger.

Signed-off-by Alexander Duyck <[email protected]>

---
 arch/x86/include/asm/page.h |   12 ++++++++++++
 arch/x86/kernel/kvm.c       |   28 ++++++++++++++++++++++++++++
 include/linux/gfp.h         |    4 ++++
 mm/page_alloc.c             |    2 ++
 4 files changed, 46 insertions(+)

arch/x86/include/asm/page.h +12 -0

index 4487ad7a3385..9540a97c9997 100644
@@ -29,6 +29,18 @@ static inline void arch_free_page(struct page *page, unsigned int order)
 	if (static_branch_unlikely(&pv_free_page_hint_enabled))
 		__arch_free_page(page, order);
 }
+
+struct zone;
+
+#define HAVE_ARCH_MERGE_PAGE
+void __arch_merge_page(struct zone *zone, struct page *page,
+		       unsigned int order);
+static inline void arch_merge_page(struct zone *zone, struct page *page,
+				   unsigned int order)
+{
+	if (static_branch_unlikely(&pv_free_page_hint_enabled))
+		__arch_merge_page(zone, page, order);
+}
 #endif
 
 #include <linux/range.h>

arch/x86/kernel/kvm.c +28 -0

index 09c91641c36c..957bb4f427bb 100644
@@ -785,6 +785,34 @@ void __arch_free_page(struct page *page, unsigned int order)
 		       PAGE_SIZE << order);
 }
 
+void __arch_merge_page(struct zone *zone, struct page *page,
+		       unsigned int order)
+{
+	/*
+	 * The merging logic has merged a set of buddies up to the
+	 * KVM_PV_UNUSED_PAGE_HINT_MIN_ORDER. Since that is the case, take
+	 * advantage of this moment to notify the hypervisor of the free
+	 * memory.
+	 */
+	if (order != KVM_PV_UNUSED_PAGE_HINT_MIN_ORDER)
+		return;
+
+	/*
+	 * Drop zone lock while processing the hypercall. This
+	 * should be safe as the page has not yet been added
+	 * to the buddy list as of yet and all the pages that
+	 * were merged have had their buddy/guard flags cleared
+	 * and their order reset to 0.
+	 */
+	spin_unlock(&zone->lock);
+
+	kvm_hypercall2(KVM_HC_UNUSED_PAGE_HINT, page_to_phys(page),
+		       PAGE_SIZE << order);
+
+	/* reacquire lock and resume freeing memory */
+	spin_lock(&zone->lock);
+}
+
 #ifdef CONFIG_PARAVIRT_SPINLOCKS
 
 /* Kick a cpu by its apicid. Used to wake up a halted vcpu */

include/linux/gfp.h +4 -0

index fdab7de7490d..4746d5560193 100644
@@ -459,6 +459,10 @@ static inline struct zonelist *node_zonelist(int nid, gfp_t flags)
 #ifndef HAVE_ARCH_FREE_PAGE
 static inline void arch_free_page(struct page *page, int order) { }
 #endif
+#ifndef HAVE_ARCH_MERGE_PAGE
+static inline void
+arch_merge_page(struct zone *zone, struct page *page, int order) { }
+#endif
 #ifndef HAVE_ARCH_ALLOC_PAGE
 static inline void arch_alloc_page(struct page *page, int order) { }
 #endif

mm/page_alloc.c +0 -0

index c954f8c1fbc4..7a1309b0b7c5 100644

mm/page_alloc.c static inline void __free_one_page(struct page *page, · line 917

Quoted 5 lines

mm/page_alloc.c +2 -0

@@ -913,6 +913,8 @@ static inline void __free_one_page(struct page *page,
 		page = page + (combined_pfn - pfn);
 		pfn = combined_pfn;
 		order++;
+
+		arch_merge_page(zone, page, order);

Not a proper place AFAICS.

Quoted 6 lines

Assume we have an order-8 page being sent here for merge and its order-8 buddy is also free, then order++ became 9 and arch_merge_page() will do the hint to host on this page as an order-9 page, no problem so far. Then the next round, assume the now order-9 page's buddy is also free, order++ will become 10 and arch_merge_page() will again hint to host on this page as an order-10 page. The first hint to host became redundant.

Actually the problem is even worse the other way around. My concern was pages being incrementally freed.

With this setup I can catch when we have crossed the threshold from order 8 to 9, and specifically for that case provide the hint. This allows me to ignore orders above and below 9.

If I move the hint to the spot after the merging I have no way of telling if I have hinted the page as a lower order or not. As such I will hint if it is merged up to orders 9 or greater. So for example if it merges up to order 9 and stops there then done_merging will report an order 9 page, then if another page is freed and merged with this up to order 10 you would be hinting on order 10. By placing the function here I can guarantee that no more than 1 hint is provided per 2MB page.

Quoted 5 lines

I think the proper place is after the done_merging tag.

BTW, with arch_merge_page() at the proper place, I don't think patch3/4 is necessary - any freed page will go through merge anyway, we won't lose any hint opportunity. Or do I miss anything?

You can refer to my comment above. What I want to avoid is us hinting a page multiple times if we aren't using MAX_ORDER - 1 as the limit. What

I am avoiding by placing this where I did is us doing a hint on orders greater than our target hint order. So with this way I only perform one hint per 2MB page, otherwise I would be performing multiple hints per 2MB page as every order above that would also trigger hints.

Quoted 119 lines · Alexander Duyck

Quoted 117 lines · Aaron Lu

Quoted 102 lines · Alexander Duyck

From: Alexander Duyck <[email protected]>

Because the implementation was limiting itself to only providing hints on pages huge TLB order sized or larger we introduced the possibility for free pages to slip past us because they are freed as something less then huge TLB in size and aggregated with buddies later.

To address that I am adding a new call arch_merge_page which is called after __free_one_page has merged a pair of pages to create a higher order page. By doing this I am able to fill the gap and provide full coverage for all of the pages huge TLB order or larger.

Signed-off-by Alexander Duyck <[email protected]>

---
 arch/x86/include/asm/page.h |   12 ++++++++++++
 arch/x86/kernel/kvm.c       |   28 ++++++++++++++++++++++++++++
 include/linux/gfp.h         |    4 ++++
 mm/page_alloc.c             |    2 ++
 4 files changed, 46 insertions(+)

arch/x86/include/asm/page.h +12 -0

index 4487ad7a3385..9540a97c9997 100644
@@ -29,6 +29,18 @@ static inline void arch_free_page(struct page *page, unsigned int order)
 	if (static_branch_unlikely(&pv_free_page_hint_enabled))
 		__arch_free_page(page, order);
 }
+
+struct zone;
+
+#define HAVE_ARCH_MERGE_PAGE
+void __arch_merge_page(struct zone *zone, struct page *page,
+		       unsigned int order);
+static inline void arch_merge_page(struct zone *zone, struct page *page,
+				   unsigned int order)
+{
+	if (static_branch_unlikely(&pv_free_page_hint_enabled))
+		__arch_merge_page(zone, page, order);
+}
 #endif
 
 #include <linux/range.h>

arch/x86/kernel/kvm.c +28 -0

index 09c91641c36c..957bb4f427bb 100644
@@ -785,6 +785,34 @@ void __arch_free_page(struct page *page, unsigned int order)
 		       PAGE_SIZE << order);
 }
 
+void __arch_merge_page(struct zone *zone, struct page *page,
+		       unsigned int order)
+{
+	/*
+	 * The merging logic has merged a set of buddies up to the
+	 * KVM_PV_UNUSED_PAGE_HINT_MIN_ORDER. Since that is the case, take
+	 * advantage of this moment to notify the hypervisor of the free
+	 * memory.
+	 */
+	if (order != KVM_PV_UNUSED_PAGE_HINT_MIN_ORDER)
+		return;
+
+	/*
+	 * Drop zone lock while processing the hypercall. This
+	 * should be safe as the page has not yet been added
+	 * to the buddy list as of yet and all the pages that
+	 * were merged have had their buddy/guard flags cleared
+	 * and their order reset to 0.
+	 */
+	spin_unlock(&zone->lock);
+
+	kvm_hypercall2(KVM_HC_UNUSED_PAGE_HINT, page_to_phys(page),
+		       PAGE_SIZE << order);
+
+	/* reacquire lock and resume freeing memory */
+	spin_lock(&zone->lock);
+}
+
 #ifdef CONFIG_PARAVIRT_SPINLOCKS
 
 /* Kick a cpu by its apicid. Used to wake up a halted vcpu */

include/linux/gfp.h +4 -0

index fdab7de7490d..4746d5560193 100644
@@ -459,6 +459,10 @@ static inline struct zonelist *node_zonelist(int nid, gfp_t flags)
 #ifndef HAVE_ARCH_FREE_PAGE
 static inline void arch_free_page(struct page *page, int order) { }
 #endif
+#ifndef HAVE_ARCH_MERGE_PAGE
+static inline void
+arch_merge_page(struct zone *zone, struct page *page, int order) { }
+#endif
 #ifndef HAVE_ARCH_ALLOC_PAGE
 static inline void arch_alloc_page(struct page *page, int order) { }
 #endif

mm/page_alloc.c +0 -0

index c954f8c1fbc4..7a1309b0b7c5 100644

mm/page_alloc.c static inline void __free_one_page(struct page *page, · line 917

Quoted 5 lines

mm/page_alloc.c +2 -0

@@ -913,6 +913,8 @@ static inline void __free_one_page(struct page *page,
 		page = page + (combined_pfn - pfn);
 		pfn = combined_pfn;
 		order++;
+
+		arch_merge_page(zone, page, order);

Not a proper place AFAICS.

Assume we have an order-8 page being sent here for merge and its order-8 buddy is also free, then order++ became 9 and arch_merge_page() will do the hint to host on this page as an order-9 page, no problem so far. Then the next round, assume the now order-9 page's buddy is also free, order++ will become 10 and arch_merge_page() will again hint to host on this page as an order-10 page. The first hint to host became redundant.