bpf_prog_pack followup

arch/x86/kernel/alternative.c +4 -0

+
+       memset(dst, c, len);
+}
+

arch/x86/kernel/alternative.c static inline void unuse_temporary_mm(temp_mm_state_t prev_state) · line 1004

Quoted 3 lines · Song Liu

arch/x86/kernel/alternative.c +3 -0

+static void text_poke_memset(void *dst, const void *src, size_t len)
+{
+       int c = *(int *)src;

It casts away the const unnecessarily. It could be *(const int *)src.

Quoted 5 lines

arch/x86/kernel/alternative.c +4 -0

+
+       memset(dst, c, len);
+}
+

Introduce a memset like API for text_poke. This will be used to fill the unused RX memory with illegal instructions.

Suggested-by Peter Zijlstra <[email protected]>

Signed-off-by Song Liu <[email protected]>

---
arch/x86/include/asm/text-patching.h | 1 +
arch/x86/kernel/alternative.c | 70 ++++++++++++++++++++++++----

2 files changed, 61 insertions(+), 10 deletions(-)

arch/x86/include/asm/text-patching.h +1 -0

index d20ab0921480..1cc15528ce29 100644
@@ -45,6 +45,7 @@ extern void *text_poke(void *addr, const void *opcode, size_t len);
 extern void text_poke_sync(void);
 extern void *text_poke_kgdb(void *addr, const void *opcode, size_t len);
 extern void *text_poke_copy(void *addr, const void *opcode, size_t len);
+extern void *text_poke_set(void *addr, int c, size_t len);
 extern int poke_int3_handler(struct pt_regs *regs);
 extern void text_poke_bp(void *addr, const void *opcode, size_t len, const void *emulate);
 

arch/x86/kernel/alternative.c +60 -10

index d374cb3cf024..732814065389 100644
@@ -994,7 +994,21 @@ static inline void unuse_temporary_mm(temp_mm_state_t prev_state)
 __ro_after_init struct mm_struct *poking_mm;
 __ro_after_init unsigned long poking_addr;
 
-static void *__text_poke(void *addr, const void *opcode, size_t len)
+static void text_poke_memcpy(void *dst, const void *src, size_t len)
+{
+	memcpy(dst, src, len);
+}
+
+static void text_poke_memset(void *dst, const void *src, size_t len)
+{
+	int c = *(int *)src;
+
+	memset(dst, c, len);
+}
+
+typedef void text_poke_f(void *dst, const void *src, size_t len);
+
+static void *__text_poke(text_poke_f func, void *addr, const void *src, size_t len)
 {
 	bool cross_page_boundary = offset_in_page(addr) + len > PAGE_SIZE;
 	struct page *pages[2] = {NULL};

@@ -1059,7 +1073,7 @@ static void *__text_poke(void *addr, const void *opcode, size_t len)
 	prev = use_temporary_mm(poking_mm);
 
 	kasan_disable_current();
-	memcpy((u8 *)poking_addr + offset_in_page(addr), opcode, len);
+	func((u8 *)poking_addr + offset_in_page(addr), src, len);
 	kasan_enable_current();
 
 	/*

@@ -1087,11 +1101,13 @@ static void *__text_poke(void *addr, const void *opcode, size_t len)
 			   (cross_page_boundary ? 2 : 1) * PAGE_SIZE,
 			   PAGE_SHIFT, false);
 
-	/*
-	 * If the text does not match what we just wrote then something is
-	 * fundamentally screwy; there's nothing we can really do about that.
-	 */
-	BUG_ON(memcmp(addr, opcode, len));
+	if (func == text_poke_memcpy) {
+		/*
+		 * If the text does not match what we just wrote then something is
+		 * fundamentally screwy; there's nothing we can really do about that.
+		 */
+		BUG_ON(memcmp(addr, src, len));
+	}
 
 	local_irq_restore(flags);
 	pte_unmap_unlock(ptep, ptl);

@@ -1118,7 +1134,7 @@ void *text_poke(void *addr, const void *opcode, size_t len)
 {
 	lockdep_assert_held(&text_mutex);
 
-	return __text_poke(addr, opcode, len);
+	return __text_poke(text_poke_memcpy, addr, opcode, len);
 }
 
 /**

@@ -1137,7 +1153,7 @@ void *text_poke(void *addr, const void *opcode, size_t len)
 */
 void *text_poke_kgdb(void *addr, const void *opcode, size_t len)
 {
-	return __text_poke(addr, opcode, len);
+	return __text_poke(text_poke_memcpy, addr, opcode, len);
 }
 
 /**

@@ -1167,7 +1183,41 @@ void *text_poke_copy(void *addr, const void *opcode, size_t len)
 
 		s = min_t(size_t, PAGE_SIZE * 2 - offset_in_page(ptr), len - patched);
 
-		__text_poke((void *)ptr, opcode + patched, s);
+		__text_poke(text_poke_memcpy, (void *)ptr, opcode + patched, s);
+		patched += s;
+	}
+	mutex_unlock(&text_mutex);
+	return addr;
+}
+
+/**
+ * text_poke_set - memset into (an unused part of) RX memory
+ * @addr: address to modify
+ * @c: the byte to fill the area with
+ * @len: length to copy, could be more than 2x PAGE_SIZE
+ *
+ * Not safe against concurrent execution; useful for JITs to dump
+ * new code blocks into unused regions of RX memory. Can be used in
+ * conjunction with synchronize_rcu_tasks() to wait for existing
+ * execution to quiesce after having made sure no existing functions
+ * pointers are live.
+ */
+void *text_poke_set(void *addr, int c, size_t len)
+{
+	unsigned long start = (unsigned long)addr;
+	size_t patched = 0;
+
+	if (WARN_ON_ONCE(core_kernel_text(start)))
+		return NULL;
+
+	mutex_lock(&text_mutex);
+	while (patched < len) {
+		unsigned long ptr = start + patched;
+		size_t s;
+
+		s = min_t(size_t, PAGE_SIZE * 2 - offset_in_page(ptr), len - patched);
+
+		__text_poke(text_poke_memset, (void *)ptr, (void *)&c, s);
 		patched += s;
 	}
 	mutex_unlock(&text_mutex);

Signature · 1 line

2.30.2

Hi Peter,

Quoted 7 lines · Song Liu

Introduce a memset like API for text_poke. This will be used to fill the unused RX memory with illegal instructions.

Suggested-by Peter Zijlstra <[email protected]>

Signed-off-by Song Liu <[email protected]>

Could you please share your comments on this?

Quoted 7 lines · Song Liu

Hi Peter,

Quoted 5 lines · Song Liu

Introduce a memset like API for text_poke. This will be used to fill the unused RX memory with illegal instructions.

Quoted 4 lines

Quoted 2 lines

Suggested-by Peter Zijlstra <[email protected]>

Signed-off-by Song Liu <[email protected]>

Could you please share your comments on this?

I wrote it; it must be good! What specifically you want to know?

Introduce a memset like API for text_poke. This will be used to fill the unused RX memory with illegal instructions.

arch/x86/kernel/alternative.c +4 -0

+	return addr;
+}
+
+/**

+ */

Quoted 2 lines · Song Liu

Introduce a memset like API for text_poke. This will be used to fill the unused RX memory with illegal instructions.

FWIW, you're going to use it to set INT3 (0xCC), that's not an illegal instruction. INTO (0xCE) would be an illegal instruction (in 64bit mode).

Quoted 11 lines

arch/x86/kernel/alternative.c +9 -0

+	return addr;
+}
+
+/**
+ * text_poke_set - memset into (an unused part of) RX memory
+ * @addr: address to modify
+ * @c: the byte to fill the area with
+ * @len: length to copy, could be more than 2x PAGE_SIZE
+ *

Quoted 5 lines

+ * Not safe against concurrent execution; useful for JITs to dump
+ * new code blocks into unused regions of RX memory. Can be used in
+ * conjunction with synchronize_rcu_tasks() to wait for existing
+ * execution to quiesce after having made sure no existing functions
+ * pointers are live.

That comment suffers from copy-pasta and needs an update because it clearly isn't correct.

Quoted 2 lines

+ */

Other than that, seems fine.

Acked-by Peter Zijlstra (Intel) <[email protected]>

Quoted 8 lines · Peter Zijlstra

Quoted 2 lines · Song Liu

Introduce a memset like API for text_poke. This will be used to fill the unused RX memory with illegal instructions.

Quoted 3 lines

FWIW, you're going to use it to set INT3 (0xCC), that's not an illegal instruction. INTO (0xCE) would be an illegal instruction (in 64bit mode).

Hmm… we have been using INT3 as illegal/invalid/special instructions in the JIT. I guess they are equally good for this job?

Introduce module_alloc_huge, which allocates huge page backed memory in module memory space. The primary user of this memory is bpf_prog_pack (multiple BPF programs sharing a huge page).

Cc Stephen Rothwell <[email protected]>

Cc Luis Chamberlain <[email protected]>

Cc Linus Torvalds <[email protected]>

Signed-off-by Song Liu <[email protected]>

---
Note: This conflicts with the module.c => module/ split in modules-next. Current patch is for module.c in the bpf-next tree. After the split, __weak module_alloc_huge() should be added to kernel/module/main.c.

---

arch/x86/kernel/module.c     | 21 +++++++++++++++++++++
include/linux/moduleloader.h |  5 +++++
kernel/module.c              |  8 ++++++++
3 files changed, 34 insertions(+)

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

index b98ffcf4d250..63f6a16c70dc 100644
@@ -86,6 +86,27 @@ void *module_alloc(unsigned long size)
 	return p;
 }
 
+void *module_alloc_huge(unsigned long size)
+{
+	gfp_t gfp_mask = GFP_KERNEL;
+	void *p;
+
+	if (PAGE_ALIGN(size) > MODULES_LEN)
+		return NULL;
+
+	p = __vmalloc_node_range(size, MODULE_ALIGN,
+				 MODULES_VADDR + get_module_load_offset(),
+				 MODULES_END, gfp_mask, PAGE_KERNEL,
+				 VM_DEFER_KMEMLEAK | VM_ALLOW_HUGE_VMAP,
+				 NUMA_NO_NODE, __builtin_return_address(0));
+	if (p && (kasan_alloc_module_shadow(p, size, gfp_mask) < 0)) {
+		vfree(p);
+		return NULL;
+	}
+
+	return p;
+}
+
 #ifdef CONFIG_X86_32
 int apply_relocate(Elf32_Shdr *sechdrs,
 		   const char *strtab,

include/linux/moduleloader.h +5 -0

index 9e09d11ffe5b..d34743a88938 100644
@@ -26,6 +26,11 @@ unsigned int arch_mod_section_prepend(struct module *mod, unsigned int section);
   sections.  Returns NULL on failure. */
 void *module_alloc(unsigned long size);
 
+/* Allocator used for allocating memory in module memory space. If size is
+ * greater than PMD_SIZE, allow using huge pages. Returns NULL on failure.
+ */
+void *module_alloc_huge(unsigned long size);
+
 /* Free memory returned from module_alloc. */
 void module_memfree(void *module_region);
 

kernel/module.c +8 -0

index 6cea788fd965..2af20ac3209c 100644
@@ -2839,6 +2839,14 @@ void * __weak module_alloc(unsigned long size)
 			NUMA_NO_NODE, __builtin_return_address(0));
 }
 
+void * __weak module_alloc_huge(unsigned long size)
+{
+	return __vmalloc_node_range(size, 1, VMALLOC_START, VMALLOC_END,
+				    GFP_KERNEL, PAGE_KERNEL_EXEC,
+				    VM_FLUSH_RESET_PERMS | VM_ALLOW_HUGE_VMAP,
+				    NUMA_NO_NODE, __builtin_return_address(0));
+}
+
 bool __weak module_init_section(const char *name)
 {
 	return strstarts(name, ".init");

Signature · 1 line

2.30.2

Use module_alloc_huge for bpf_prog_pack so that BPF programs sit on PMD_SIZE pages. This benefits system performance by reducing iTLB miss
rate. Benchmark of a real web service workload shows this change gives
another ~0.2% performance boost on top of PAGE_SIZE bpf_prog_pack (which improve system throughput by ~0.5%).

Also, remove set_vm_flush_reset_perms() from alloc_new_pack() and use set_memory_[nx|rw] in bpf_prog_pack_free(). This is because VM_FLUSH_RESET_PERMS does not work with huge pages yet. [1]

[1]
https://lore.kernel.org/bpf/[email protected]/

Suggested-by Rick Edgecombe <[email protected]>

Signed-off-by Song Liu <[email protected]>

---
 kernel/bpf/core.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

kernel/bpf/core.c +0 -0

index cacd8684c3c4..b64d91fcb0ba 100644

kernel/bpf/core.c +0 -0

 	/* Test whether we can get huge pages. If not just use

PAGE_SIZE

kernel/bpf/core.c +1 -1

 	 * packs.

@@ -881,7 +881,7 @@ static struct bpf_prog_pack
 *alloc_new_pack(bpf_jit_fill_hole_t bpf_fill_ill_ins
 		       GFP_KERNEL);
 	if (!pack)
 		return NULL;
-	pack->ptr = module_alloc(bpf_prog_pack_size);
+	pack->ptr = module_alloc_huge(bpf_prog_pack_size);
 	if (!pack->ptr) {
 		kfree(pack);

kernel/bpf/core.c +0 -1

 		return NULL;

@@ -890,7 +890,6 @@ static struct bpf_prog_pack
 *alloc_new_pack(bpf_jit_fill_hole_t bpf_fill_ill_ins
 	bitmap_zero(pack->bitmap, bpf_prog_pack_size /
 BPF_PROG_CHUNK_SIZE);
 	list_add_tail(&pack->list, &pack_list);
 
-	set_vm_flush_reset_perms(pack->ptr);
 	set_memory_ro((unsigned long)pack->ptr, bpf_prog_pack_size /

PAGE_SIZE);
set_memory_x((unsigned long)pack->ptr, bpf_prog_pack_size /
PAGE_SIZE);

kernel/bpf/core.c +0 -0

 	return pack;

@@ -909,10 +908,9 @@ static void *bpf_prog_pack_alloc(u32 size,

bpf_jit_fill_hole_t bpf_fill_ill_insn

kernel/bpf/core.c +1 -2

 	if (size > bpf_prog_pack_size) {
 		size = round_up(size, PAGE_SIZE);
-		ptr = module_alloc(size);
+		ptr = module_alloc_huge(size);
 		if (ptr) {
 			bpf_fill_ill_insns(ptr, size);
-			set_vm_flush_reset_perms(ptr);
 			set_memory_ro((unsigned long)ptr, size /

PAGE_SIZE);
set_memory_x((unsigned long)ptr, size /
PAGE_SIZE);

kernel/bpf/core.c +2 -0

 		}

@@ -949,6 +947,8 @@ static void bpf_prog_pack_free(struct
 bpf_binary_header *hdr)
 
 	mutex_lock(&pack_mutex);
 	if (hdr->size > bpf_prog_pack_size) {
+		set_memory_nx((unsigned long)hdr, hdr->size /
 PAGE_SIZE);
+		set_memory_rw((unsigned long)hdr, hdr->size /
 PAGE_SIZE);

kernel/bpf/core.c +1 -0

 		module_memfree(hdr);
 		goto out;
 	}

@@ -975,6 +975,8 @@ static void bpf_prog_pack_free(struct
 bpf_binary_header *hdr)
 	if (bitmap_find_next_zero_area(pack->bitmap,
 bpf_prog_chunk_count(), 0,
 				       bpf_prog_chunk_count(), 0) == 0)
 {
 		list_del(&pack->list);
+		set_memory_nx((unsigned long)pack->ptr,

bpf_prog_pack_size / PAGE_SIZE);

kernel/bpf/core.c +1 -0

+		set_memory_rw((unsigned long)pack->ptr,

bpf_prog_pack_size / PAGE_SIZE);

kernel/bpf/core.c +0 -0

 		module_memfree(pack->ptr);
 		kfree(pack);
 	}

Quoted 7 lines · Song Liu

Use module_alloc_huge for bpf_prog_pack so that BPF programs sit on PMD_SIZE pages. This benefits system performance by reducing iTLB miss
rate. Benchmark of a real web service workload shows this change gives
another ~0.2% performance boost on top of PAGE_SIZE bpf_prog_pack (which improve system throughput by ~0.5%).

0.7% sounds good as a whole. How sure are you of that +0.2%? Was this a big averaged test?

Quoted 9 lines

Also, remove set_vm_flush_reset_perms() from alloc_new_pack() and use set_memory_[nx|rw] in bpf_prog_pack_free(). This is because VM_FLUSH_RESET_PERMS does not work with huge pages yet. [1]

[1]
https://lore.kernel.org/bpf/[email protected]/

Suggested-by Rick Edgecombe <[email protected]>

As I said before, I think this will work functionally. But I meant it as a quick fix when we were talking about patching this up to keep it enabled upstream.

So now, should we make VM_FLUSH_RESET_PERMS work properly with huge pages? The main benefit would be to keep the tear down of these types of allocations consistent for correctness reasons. The TLB flush minimizing differences are probably less impactful given the caching introduced here. At the very least though, we should have (or have already had) some WARN if people try to use it with huge pages.

Quoted 13 lines

Signed-off-by Song Liu <[email protected]>

---
kernel/bpf/core.c | 12 +++++++-----

1 file changed, 7 insertions(+), 5 deletions(-)

kernel/bpf/core.c +0 -0

index cacd8684c3c4..b64d91fcb0ba 100644
@@ -857,7 +857,7 @@ static size_t select_bpf_prog_pack_size(void)
 	void *ptr;
 

kernel/bpf/core.c static size_t select_bpf_prog_pack_size(void) · line 860

Quoted 3 lines

kernel/bpf/core.c +1 -1

 	size = BPF_HPAGE_SIZE * num_online_nodes();
-	ptr = module_alloc(size);
+	ptr = module_alloc_huge(size);

This select_bpf_prog_pack_size() function always seemed weird - doing a big allocation and then immediately freeing. Can't it check a config for vmalloc huge page support?

Quoted 68 lines

kernel/bpf/core.c +0 -0

 	/* Test whether we can get huge pages. If not just use

PAGE_SIZE
* packs.

kernel/bpf/core.c +1 -1

@@ -881,7 +881,7 @@ static struct bpf_prog_pack
 *alloc_new_pack(bpf_jit_fill_hole_t bpf_fill_ill_ins
 		       GFP_KERNEL);
 	if (!pack)
 		return NULL;
-	pack->ptr = module_alloc(bpf_prog_pack_size);
+	pack->ptr = module_alloc_huge(bpf_prog_pack_size);
 	if (!pack->ptr) {
 		kfree(pack);

		return NULL;

kernel/bpf/core.c +0 -1

@@ -890,7 +890,6 @@ static struct bpf_prog_pack
 *alloc_new_pack(bpf_jit_fill_hole_t bpf_fill_ill_ins
 	bitmap_zero(pack->bitmap, bpf_prog_pack_size /
 BPF_PROG_CHUNK_SIZE);
 	list_add_tail(&pack->list, &pack_list);
 
-	set_vm_flush_reset_perms(pack->ptr);
 	set_memory_ro((unsigned long)pack->ptr, bpf_prog_pack_size /

PAGE_SIZE);
	set_memory_x((unsigned long)pack->ptr, bpf_prog_pack_size /
PAGE_SIZE);
	return pack;

kernel/bpf/core.c +0 -0

@@ -909,10 +908,9 @@ static void *bpf_prog_pack_alloc(u32 size,

bpf_jit_fill_hole_t bpf_fill_ill_insn

	if (size > bpf_prog_pack_size) {
		size = round_up(size, PAGE_SIZE);

kernel/bpf/core.c +1 -1

-		ptr = module_alloc(size);
+		ptr = module_alloc_huge(size);

		if (ptr) {
			bpf_fill_ill_insns(ptr, size);

kernel/bpf/core.c +0 -1

-			set_vm_flush_reset_perms(ptr);

set_memory_ro((unsigned long)ptr, size /
PAGE_SIZE);
set_memory_x((unsigned long)ptr, size /

PAGE_SIZE);
		}

kernel/bpf/core.c +2 -0

@@ -949,6 +947,8 @@ static void bpf_prog_pack_free(struct
 bpf_binary_header *hdr)
 
 	mutex_lock(&pack_mutex);
 	if (hdr->size > bpf_prog_pack_size) {
+		set_memory_nx((unsigned long)hdr, hdr->size /
 PAGE_SIZE);
+		set_memory_rw((unsigned long)hdr, hdr->size /
 PAGE_SIZE);

		module_memfree(hdr);
		goto out;
	}

kernel/bpf/core.c +1 -0

@@ -975,6 +975,8 @@ static void bpf_prog_pack_free(struct
 bpf_binary_header *hdr)
 	if (bitmap_find_next_zero_area(pack->bitmap,
 bpf_prog_chunk_count(), 0,
 				       bpf_prog_chunk_count(), 0) == 0)
 {
 		list_del(&pack->list);
+		set_memory_nx((unsigned long)pack->ptr,

bpf_prog_pack_size / PAGE_SIZE);

kernel/bpf/core.c +1 -0

+		set_memory_rw((unsigned long)pack->ptr,

bpf_prog_pack_size / PAGE_SIZE);
		module_memfree(pack->ptr);
		kfree(pack);
	}

Quoted 13 lines

On May 17, 2022, at 12:15 PM, Edgecombe, Rick P <
[email protected]> wrote:

Quoted 8 lines · Song Liu

Use module_alloc_huge for bpf_prog_pack so that BPF programs sit on
PMD_SIZE pages. This benefits system performance by reducing iTLB miss
rate. Benchmark of a real web service workload shows this change gives
another ~0.2% performance boost on top of PAGE_SIZE bpf_prog_pack (which improve system throughput by ~0.5%).

Quoted 3 lines

0.7% sounds good as a whole. How sure are you of that +0.2%? Was this a
big averaged test?

Yes, this was a test between two tiers with 10+ servers on each tier.
We took the average performance over a few hours of shadow workload.

Quoted 10 lines

Quoted 10 lines

Also, remove set_vm_flush_reset_perms() from alloc_new_pack() and use
set_memory_[nx|rw] in bpf_prog_pack_free(). This is because VM_FLUSH_RESET_PERMS does not work with huge pages yet. [1]

[1]

Quoted 8 lines

Quoted 1 line

Suggested-by Rick Edgecombe <[email protected]>

As I said before, I think this will work functionally. But I meant it
as a quick fix when we were talking about patching this up to keep it
enabled upstream.

Quoted 8 lines

So now, should we make VM_FLUSH_RESET_PERMS work properly with huge pages? The main benefit would be to keep the tear down of these types
of allocations consistent for correctness reasons. The TLB flush minimizing differences are probably less impactful given the caching
introduced here. At the very least though, we should have (or have already had) some WARN if people try to use it with huge pages.

I am not quite sure the exact work needed here. Rick, would you have time to enable VM_FLUSH_RESET_PERMS for huge pages? Given the merge window is coming soon, I guess we need current work around in 5.19.

Quoted 14 lines

Quoted 14 lines

Signed-off-by Song Liu <[email protected]>

---
kernel/bpf/core.c | 12 +++++++-----

1 file changed, 7 insertions(+), 5 deletions(-)

kernel/bpf/core.c +0 -0

index cacd8684c3c4..b64d91fcb0ba 100644
@@ -857,7 +857,7 @@ static size_t select_bpf_prog_pack_size(void)
      void *ptr;
 

Quoted 9 lines

kernel/bpf/core.c static size_t select_bpf_prog_pack_size(void) · line 860

Quoted 3 lines

kernel/bpf/core.c +1 -1

      size = BPF_HPAGE_SIZE * num_online_nodes();
-    ptr = module_alloc(size);
+    ptr = module_alloc_huge(size);

This select_bpf_prog_pack_size() function always seemed weird - doing a
big allocation and then immediately freeing. Can't it check a config
for vmalloc huge page support?

Yes, it is weird. Checking a config is not enough here. We also need to
check vmap_allow_huge, which is controlled by boot parameter nohugeiomap.
I haven’t got a better solution for this.

Quoted 13 lines · Song Liu

Quoted 13 lines

On May 17, 2022, at 12:15 PM, Edgecombe, Rick P <
[email protected]> wrote:

Quoted 8 lines · Song Liu

Use module_alloc_huge for bpf_prog_pack so that BPF programs sit on
PMD_SIZE pages. This benefits system performance by reducing iTLB miss
rate. Benchmark of a real web service workload shows this change gives
another ~0.2% performance boost on top of PAGE_SIZE bpf_prog_pack (which improve system throughput by ~0.5%).

Quoted 7 lines

Quoted 3 lines

0.7% sounds good as a whole. How sure are you of that +0.2%? Was this a
big averaged test?

Yes, this was a test between two tiers with 10+ servers on each tier.
We took the average performance over a few hours of shadow workload.

Awesome. Sounds great.

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

Also, remove set_vm_flush_reset_perms() from alloc_new_pack() and use
set_memory_[nx|rw] in bpf_prog_pack_free(). This is because VM_FLUSH_RESET_PERMS does not work with huge pages yet. [1]

[1]

https://lore.kernel.org/bpf/[email protected]/

Quoted 17 lines

Quoted 16 lines

Quoted 1 line

Suggested-by Rick Edgecombe <[email protected]>

As I said before, I think this will work functionally. But I meant it
as a quick fix when we were talking about patching this up to keep it
enabled upstream.

So now, should we make VM_FLUSH_RESET_PERMS work properly with huge pages? The main benefit would be to keep the tear down of these types
of allocations consistent for correctness reasons. The TLB flush minimizing differences are probably less impactful given the caching
introduced here. At the very least though, we should have (or have already had) some WARN if people try to use it with huge pages.

Quoted 3 lines

I am not quite sure the exact work needed here. Rick, would you have time to enable VM_FLUSH_RESET_PERMS for huge pages? Given the merge window is coming soon, I guess we need current work around in 5.19.

I would have hard time squeezing that in now. The vmalloc part is easy, I think I already posted a diff. But first hibernate needs to be changed to not care about direct map page sizes.

Quoted 25 lines

Quoted 24 lines

Quoted 13 lines

Signed-off-by Song Liu <[email protected]>

---
kernel/bpf/core.c | 12 +++++++-----

1 file changed, 7 insertions(+), 5 deletions(-)

kernel/bpf/core.c +0 -0

index cacd8684c3c4..b64d91fcb0ba 100644

kernel/bpf/core.c static size_t select_bpf_prog_pack_size(void) · line 860

Quoted 5 lines

kernel/bpf/core.c +1 -1

@@ -857,7 +857,7 @@ static size_t select_bpf_prog_pack_size(void)
     void *ptr;
 
     size = BPF_HPAGE_SIZE * num_online_nodes();
-    ptr = module_alloc(size);
+    ptr = module_alloc_huge(size);

This select_bpf_prog_pack_size() function always seemed weird - doing a
big allocation and then immediately freeing. Can't it check a config
for vmalloc huge page support?

Quoted 5 lines

Yes, it is weird. Checking a config is not enough here. We also need to
check vmap_allow_huge, which is controlled by boot parameter nohugeiomap.
I haven’t got a better solution for this.

It's too weird. We should expose whats needed in vmalloc. huge_vmalloc_supported() or something.

I'm also not clear why we wouldn't want to use the prog pack allocator even if vmalloc huge pages was disabled. Doesn't it improve performance even with small page sizes, per your benchmarks? What is the downside to just always using it?

Quoted 13 lines · Song Liu

Quoted 13 lines

On May 17, 2022, at 12:15 PM, Edgecombe, Rick P <
[email protected]> wrote:

Quoted 8 lines · Song Liu

Use module_alloc_huge for bpf_prog_pack so that BPF programs sit on
PMD_SIZE pages. This benefits system performance by reducing iTLB miss
rate. Benchmark of a real web service workload shows this change gives
another ~0.2% performance boost on top of PAGE_SIZE bpf_prog_pack (which improve system throughput by ~0.5%).

Quoted 7 lines

Quoted 3 lines

0.7% sounds good as a whole. How sure are you of that +0.2%? Was this a
big averaged test?

Yes, this was a test between two tiers with 10+ servers on each tier.
We took the average performance over a few hours of shadow workload.

Awesome. Sounds great.

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

Also, remove set_vm_flush_reset_perms() from alloc_new_pack() and use
set_memory_[nx|rw] in bpf_prog_pack_free(). This is because VM_FLUSH_RESET_PERMS does not work with huge pages yet. [1]

[1]

https://lore.kernel.org/bpf/[email protected]/

Quoted 17 lines

Quoted 16 lines

Quoted 1 line

Suggested-by Rick Edgecombe <[email protected]>

As I said before, I think this will work functionally. But I meant it
as a quick fix when we were talking about patching this up to keep it
enabled upstream.

So now, should we make VM_FLUSH_RESET_PERMS work properly with huge pages? The main benefit would be to keep the tear down of these types
of allocations consistent for correctness reasons. The TLB flush minimizing differences are probably less impactful given the caching
introduced here. At the very least though, we should have (or have already had) some WARN if people try to use it with huge pages.

Quoted 3 lines

I am not quite sure the exact work needed here. Rick, would you have time to enable VM_FLUSH_RESET_PERMS for huge pages? Given the merge window is coming soon, I guess we need current work around in 5.19.

I would have hard time squeezing that in now. The vmalloc part is easy, I think I already posted a diff. But first hibernate needs to be changed to not care about direct map page sizes.

Quoted 25 lines

Quoted 24 lines

Quoted 13 lines

Signed-off-by Song Liu <[email protected]>

---
kernel/bpf/core.c | 12 +++++++-----

1 file changed, 7 insertions(+), 5 deletions(-)

kernel/bpf/core.c +0 -0

index cacd8684c3c4..b64d91fcb0ba 100644

kernel/bpf/core.c static size_t select_bpf_prog_pack_size(void) · line 860

Quoted 5 lines

kernel/bpf/core.c +1 -1

@@ -857,7 +857,7 @@ static size_t select_bpf_prog_pack_size(void)
     void *ptr;
 
     size = BPF_HPAGE_SIZE * num_online_nodes();
-    ptr = module_alloc(size);
+    ptr = module_alloc_huge(size);

This select_bpf_prog_pack_size() function always seemed weird - doing a
big allocation and then immediately freeing. Can't it check a config
for vmalloc huge page support?

Quoted 5 lines

Yes, it is weird. Checking a config is not enough here. We also need to
check vmap_allow_huge, which is controlled by boot parameter nohugeiomap.
I haven’t got a better solution for this.

It's too weird. We should expose whats needed in vmalloc. huge_vmalloc_supported() or something.

I'm also not clear why we wouldn't want to use the prog pack allocator even if vmalloc huge pages was disabled. Doesn't it improve performance even with small page sizes, per your benchmarks? What is the downside to just always using it?

Quoted 7 lines

Quoted 6 lines

I am not quite sure the exact work needed here. Rick, would you have
time to enable VM_FLUSH_RESET_PERMS for huge pages? Given the merge
window is coming soon, I guess we need current work around in 5.19.

Quoted 4 lines

I would have hard time squeezing that in now. The vmalloc part is easy,
I think I already posted a diff. But first hibernate needs to be changed to not care about direct map page sizes.

I guess I missed the diff, could you please send a link to it?

Quoted 34 lines

Quoted 27 lines

Quoted 17 lines

Quoted 17 lines

Signed-off-by Song Liu <[email protected]>

---
kernel/bpf/core.c | 12 +++++++-----

1 file changed, 7 insertions(+), 5 deletions(-)

kernel/bpf/core.c +0 -0

index cacd8684c3c4..b64d91fcb0ba 100644
@@ -857,7 +857,7 @@ static size_t
 select_bpf_prog_pack_size(void)
      void *ptr;
 

Quoted 9 lines

kernel/bpf/core.c static size_t select_bpf_prog_pack_size(void) · line 860

Quoted 3 lines

kernel/bpf/core.c +1 -1

      size = BPF_HPAGE_SIZE * num_online_nodes();
-    ptr = module_alloc(size);
+    ptr = module_alloc_huge(size);

This select_bpf_prog_pack_size() function always seemed weird - doing a
big allocation and then immediately freeing. Can't it check a config
for vmalloc huge page support?

Quoted 6 lines

Yes, it is weird. Checking a config is not enough here. We also need
to
check vmap_allow_huge, which is controlled by boot parameter nohugeiomap.
I haven’t got a better solution for this.

Quoted 2 lines

It's too weird. We should expose whats needed in vmalloc. huge_vmalloc_supported() or something.

Yeah, this should work. I will get something like this in the next version.

Quoted 8 lines

I'm also not clear why we wouldn't want to use the prog pack allocator
even if vmalloc huge pages was disabled. Doesn't it improve performance
even with small page sizes, per your benchmarks? What is the downside
to just always using it?

With current version, when huge page is disabled, the prog pack allocator
will use 4kB pages for each pack. We still get about 0.5% performance improvement with 4kB prog packs.

Quoted 7 lines · Song Liu

Quoted 7 lines

Quoted 6 lines

I am not quite sure the exact work needed here. Rick, would you have
time to enable VM_FLUSH_RESET_PERMS for huge pages? Given the merge
window is coming soon, I guess we need current work around in 5.19.

Quoted 6 lines

Quoted 4 lines

I would have hard time squeezing that in now. The vmalloc part is easy,
I think I already posted a diff. But first hibernate needs to be changed to not care about direct map page sizes.

I guess I missed the diff, could you please send a link to it?

https://lore.kernel.org/lkml/[email protected]/

The remaining problem is that hibernate may encounter NP pages when saving memory to disk. It resets them with CPA calls 4k at a time. So if a page is NP, hibernate needs it to be already be 4k or it might need to split. I think hibernate should just utilize a different mapping to get at the page when it encounters this rare scenario. In that diff I put some locking so that hibernate couldn't race with a huge NP page, but then I thought we should just change hibernate.

Quoted 16 lines

Quoted 8 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 7 lines

I'm also not clear why we wouldn't want to use the prog pack allocator
even if vmalloc huge pages was disabled. Doesn't it improve performance
even with small page sizes, per your benchmarks? What is the downside
to just always using it?

Quoted 4 lines

With current version, when huge page is disabled, the prog pack allocator
will use 4kB pages for each pack. We still get about 0.5% performance improvement with 4kB prog packs.

Oh, I thought you were comparing a 2MB sized, small page mapped allocation to a 2MB sized, huge page mapped allocation.

It looks like the logic is to free a pack if it is empty, so then for smaller packs you are more likely to let the pages go back to the page allocator. Then future allocations would break more pages.

So I think that is not a fully apples to apples test of huge mapping benefits. I'd be surprised if there really was no huge mapping benefit, since its been seen with core kernel text. Did you notice if the direct map breakage was different between the tests?

Quoted 24 lines

Quoted 23 lines

Quoted 12 lines

Signed-off-by Song Liu <[email protected]>

---
kernel/bpf/core.c | 12 +++++++-----

1 file changed, 7 insertions(+), 5 deletions(-)

kernel/bpf/core.c +0 -0

index cacd8684c3c4..b64d91fcb0ba 100644

kernel/bpf/core.c static size_t select_bpf_prog_pack_size(void) · line 860

Quoted 5 lines

kernel/bpf/core.c +1 -1

@@ -857,7 +857,7 @@ static size_t select_bpf_prog_pack_size(void)
     void *ptr;
 
     size = BPF_HPAGE_SIZE * num_online_nodes();
-    ptr = module_alloc(size);
+    ptr = module_alloc_huge(size);

This select_bpf_prog_pack_size() function always seemed weird - doing a
big allocation and then immediately freeing. Can't it check a config
for vmalloc huge page support?

Quoted 5 lines

Yes, it is weird. Checking a config is not enough here. We also need to
check vmap_allow_huge, which is controlled by boot parameter nohugeiomap.
I haven’t got a better solution for this.

It's too weird. We should expose whats needed in vmalloc. huge_vmalloc_supported() or something.

I'm also not clear why we wouldn't want to use the prog pack allocator even if vmalloc huge pages was disabled. Doesn't it improve performance even with small page sizes, per your benchmarks? What is the downside to just always using it?

Thinking more on this. Even huge page is not supported, we can allocate
2MB worth of 4kB pages and keep using it. This would help direct map fragmentation. And the code would also be simpler.

Rick, I guess this is inline with some of your ideas?

Quoted 5 lines · Song Liu

Thinking more on this. Even huge page is not supported, we can allocate
2MB worth of 4kB pages and keep using it. This would help direct map fragmentation. And the code would also be simpler.

Quoted 1 line

Rick, I guess this is inline with some of your ideas?

Yea, that is what I wondering. Potential benefits are just speculative though. There is a memory overhead cost, so it's not free.

As for the other question of whether to fix VM_FLUSH_RESET_PERMS. If there really is an intention to create a more general module_alloc() replacement soon, then I think it is ok to side step it. An optimal replacement might not need it and it could be removed in that case. Let's at least add a WARN about it not working with huge pages though.

I also think the benchmarking so far is not sufficient to make the case that huge page mappings help your workload since the direct map splits were also different between the tests. I was expecting it to help though. Others were the ones that asked for that, so just commenting my analysis here.