s390/mm: Batch PTE updates in lazy MMU mode

+/**
+ * lazy_mmu_mode_enable_for_pte_range() - Enable the lazy MMU mode with a speedup hint.
+ * @mm: Address space the pages are mapped into.
+ * @addr: Start address of the range.
+ * @end: End address of the range.

+ * @ptep: Page table pointer for the first entry.
+ *
+ * Enters a new lazy MMU mode section; if the mode was not already enabled,
+ * enables it and calls arch_enter_lazy_mmu_mode_for_pte_range().
+ *
+ * PTEs that fall within the specified range might observe update speedups.
+ * The PTE range must belong to the specified memory space and not cross
+ * a page table boundary.

+ *
+ * There are no requirements on the order or range completeness of PTE
+ * updates for the specified range.
+ *
+ * Must be paired with a call to lazy_mmu_mode_disable().
+ *
+ * Has no effect if called:
+ * - While paused - see lazy_mmu_mode_pause()
+ * - In interrupt context
+ */

include/linux/pgtable.h +13 -0

+static inline void lazy_mmu_mode_enable_for_pte_range(struct mm_struct *mm,
+		unsigned long addr, unsigned long end, pte_t *ptep)
+{
+	struct lazy_mmu_state *state = &current->lazy_mmu_state;
+
+	if (in_interrupt() || state->pause_count > 0)
+		return;
+
+	VM_WARN_ON_ONCE(state->enable_count == U8_MAX);
+
+	if (state->enable_count++ == 0)
+		arch_enter_lazy_mmu_mode_for_pte_range(mm, addr, end, ptep);
+}

Quoted 5 lines · Alexander Gordeev

+/**
+ * lazy_mmu_mode_enable_for_pte_range() - Enable the lazy MMU mode with a speedup hint.
+ * @mm: Address space the pages are mapped into.
+ * @addr: Start address of the range.
+ * @end: End address of the range.

Quoted 8 lines

+ * @ptep: Page table pointer for the first entry.
+ *
+ * Enters a new lazy MMU mode section; if the mode was not already enabled,
+ * enables it and calls arch_enter_lazy_mmu_mode_for_pte_range().
+ *
+ * PTEs that fall within the specified range might observe update speedups.
+ * The PTE range must belong to the specified memory space and not cross
+ * a page table boundary.

Does that mean that all PTEs mapping [addr, end) must belong to the same PTE page? I think the wording should be more specific.

LGTM otherwise:

Reviewed-by Kevin Brodsky <[email protected]>

Quoted 5 lines

Quoted 3 lines

+ * PTEs that fall within the specified range might observe update speedups.
+ * The PTE range must belong to the specified memory space and not cross
+ * a page table boundary.

Does that mean that all PTEs mapping [addr, end) must belong to the same PTE page? I think the wording should be more specific.

I tried to state that end of the range must not exceed pmd_addr_end(addr, end). Any of these sounds better?

The PTE range must belong to the specified memory space and ...
a) the address range must not cross the parent PMD address range boundary
b) the PTEs must belong to the same parent PMD

Quoted 5 lines · Alexander Gordeev

Quoted 2 lines

Does that mean that all PTEs mapping [addr, end) must belong to the same PTE page? I think the wording should be more specific.

I tried to state that end of the range must not exceed pmd_addr_end(addr, end). Any of these sounds better?

Quoted 3 lines

The PTE range must belong to the specified memory space and ...
a) the address range must not cross the parent PMD address range boundary
b) the PTEs must belong to the same parent PMD

I think b) is good, maybe "PMD entry" to be even clearer.  My previous suggestion of talking about "PTE page" isn't ideal since some architectures have page tables smaller (or larger) than a page.

include/linux/pgtable.h +14 -0

+/**
+ * lazy_mmu_mode_enable_for_pte_range() - Enable the lazy MMU mode with a speedup hint.
+ * @mm: Address space the pages are mapped into.
+ * @addr: Start address of the range.
+ * @end: End address of the range.
+ * @ptep: Page table pointer for the first entry.
+ *
+ * Enters a new lazy MMU mode section; if the mode was not already enabled,
+ * enables it and calls arch_enter_lazy_mmu_mode_for_pte_range().
+ *
+ * PTEs that fall within the specified range might observe update speedups.
+ * The PTE range must belong to the specified memory space and not cross
+ * a page table boundary.
+ *

I'm still wondering about a better name for this. "enable_for" indeed sounds like it's only for that range.

Maybe really something including a hint like you previously proposed

lazy_mmu_mode_enable_with_ptes()

lazy_mmu_mode_enable_with_pte_hints()

I think lazy_mmu_mode_enable_with_ptes() is neat.

Thoughts?

In any case, on the general approach + semantics:

Acked-by David Hildenbrand (Arm) <[email protected]>

Signature · 3 lines

Cheers,

David

Lazy MMU mode is assumed to be context-independent, in the sense that it does not need any additional information while operating. However, the s390 architecture benefits from knowing the exact page table entries being modified.

Introduce lazy_mmu_mode_enable_for_pte_range(), which is provided with the process address space and the page table being operated on. This information is required to enable s390-specific optimizations.

The function takes parameters that are typically passed to page- table level walkers, which implies that the span of PTE entries never crosses a page table boundary.

Architectures that do not require such information simply do not need to define the lazy_mmu_mode_enable_for_pte_range() callback.

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

---
 fs/proc/task_mmu.c      |  2 +-
 include/linux/pgtable.h | 47 +++++++++++++++++++++++++++++++++++++++++
 mm/madvise.c            |  8 +++----
 mm/memory.c             |  8 +++----
 mm/mprotect.c           |  2 +-
 mm/mremap.c             |  2 +-
 mm/vmalloc.c            |  6 +++---
 7 files changed, 61 insertions(+), 14 deletions(-)

+/**
+ * lazy_mmu_mode_enable_for_pte_range() - Enable the lazy MMU mode with a speedup hint.
+ * @mm: Address space the pages are mapped into.
+ * @addr: Start address of the range.

+ * @end: End address of the range.
+ * @ptep: Page table pointer for the first entry.
+ *
+ * Enters a new lazy MMU mode section; if the mode was not already enabled,
+ * enables it and calls arch_enter_lazy_mmu_mode_for_pte_range().
+ *
+ * PTEs that fall within the specified range might observe update speedups.
+ * The PTE range must belong to the specified memory space and not cross

The address provided to clear_not_present_full_ptes() is the address of the underlying memory, not address of the first PTE. The exact wording is taken from clear_ptes() comment.

Suggested-by Kevin Brodsky <[email protected]>

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

---
 include/linux/pgtable.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

include/linux/pgtable.h +0 -0

index 9ff7b78d65b1..2b82a71f13d7 100644

Talking about pages with non-present entries does not make sense. So both changes are wrong. Can you find something better and send that out separately? I don't think this is required for your series, right?

Finalize commit c33c794828f2 ("mm: ptep_get() conversion") and replace direct page table entry dereferencing with the proper accessors (ptep_get(), pmdp_get(), etc.).

Override the default getter implementations even though they are currently identical: pud_clear(), p4d_clear(), and pgd_clear() require corresponding architecture-specific getters, but these are not yet defined. This avoids a dependency loop.

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

---
 arch/s390/boot/vmem.c           | 32 +++++++------
 arch/s390/include/asm/hugetlb.h |  2 +-
 arch/s390/include/asm/pgtable.h | 60 ++++++++++++++++++------
 arch/s390/mm/hugetlbpage.c      | 12 ++---
 arch/s390/mm/pageattr.c         | 42 +++++++++--------
 arch/s390/mm/vmem.c             | 82 ++++++++++++++++++---------------
 6 files changed, 138 insertions(+), 92 deletions(-)

Was this a manual conversion, or did you use some tooling?

I'm wondering since I can spot not converted locations, e.g. one in __kernel_map_pages().

Besides that:

Acked-by Heiko Carstens <[email protected]>

Quoted 1 line · Heiko Carstens

Was this a manual conversion, or did you use some tooling?

Manual :) But I will try to recheck with a tool.

Quoted 2 lines

I'm wondering since I can spot not converted locations, e.g. one in __kernel_map_pages().

Whether this fixup is fine?

arch/s390/mm/pageattr.c +0 -0

index 3a54860cb05f..e6f788696dd1 100644

In various code paths, page_table_check_pte_clear() is called before converting a secure page, while in others it is called after. Make this consistent and always perform the conversion after the PTC hook has been called. Also make all conversion‑ eligibility condition checks look the same, and rework the one in ptep_get_and_clear_full() slightly.

Acked-by Claudio Imbrenda <[email protected]>

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

---
 arch/s390/include/asm/pgtable.h | 39 +++++++++++++++------------------
 1 file changed, 18 insertions(+), 21 deletions(-)

Quoted 7 lines · Alexander Gordeev

In various code paths, page_table_check_pte_clear() is called before converting a secure page, while in others it is called after. Make this consistent and always perform the conversion after the PTC hook has been called. Also make all conversion‑ eligibility condition checks look the same, and rework the one in ptep_get_and_clear_full() slightly.

Quoted 5 lines

Acked-by Claudio Imbrenda <[email protected]>

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

---
 arch/s390/include/asm/pgtable.h | 39 +++++++++++++++------------------
 1 file changed, 18 insertions(+), 21 deletions(-)

Why is this required for this series? It is anything but obvious to me. If it is required please add some reasoning e.g. in the cover letter or to the above changelog.

arch/s390/Kconfig +2 -0

index 7828fbe0fc42..deffc819268e 100644
@@ -732,6 +732,17 @@ config MAX_PHYSMEM_BITS
 	  Increasing the number of bits also increases the kernel image size.
 	  By default 46 bits (64TB) are supported.
 
+config IPTE_BATCH
+	def_bool y

arch/s390/include/asm/pgtable.h enum { · line 49

Quoted 8 lines

arch/s390/include/asm/pgtable.h +8 -0

+#if !defined(CONFIG_IPTE_BATCH) || defined(__DECOMPRESSOR)
+static inline
+bool ipte_batch_ptep_test_and_clear_young(struct vm_area_struct *vma,
+					  unsigned long addr, pte_t *ptep,
+					  int *res)
+{
+	return false;
+}

Why does this depend on __DECOMPRESSOR? Which code in the decompressor would generate a call to the ipte_batch functions?

Make use of the IPTE instruction's "Additional Entries" field to invalidate multiple PTEs in one go while in lazy MMU mode. This is the mode in which many memory-management system calls (like mremap(), mprotect(), etc.) update memory attributes.

To achieve that, the set_pte() and ptep_get() primitives use a per-CPU cache to store and retrieve PTE values and apply the cached values to the real page table once lazy MMU mode is left.

The same is done for memory-management platform callbacks that would otherwise cause intense per-PTE IPTE traffic, reducing the number of IPTE instructions from up to PTRS_PER_PTE to a single instruction in the best case. The average reduction is of course smaller.

Since all existing page table iterators called in lazy MMU mode handle one table at a time, the per-CPU cache does not need to be larger than PTRS_PER_PTE entries. That also naturally aligns with the IPTE instruction, which must not cross a page table boundary.

Before this change, the system calls did:

	lazy_mmu_mode_enable_pte()
	...
	<update PTEs>		// up to PTRS_PER_PTE single-IPTEs
	...
	lazy_mmu_mode_disable()

With this change, the system calls do:

lazy_mmu_mode_enable_pte()
...
<store new PTE values in the per-CPU cache>
...
lazy_mmu_mode_disable() // apply cache with one multi-IPTE

arch/s390/include/asm/pgtable.h +8 -0

+#if !defined(CONFIG_IPTE_BATCH) || defined(__DECOMPRESSOR)
+static inline
+bool ipte_batch_ptep_test_and_clear_young(struct vm_area_struct *vma,
+					  unsigned long addr, pte_t *ptep,
+					  int *res)
+{
+	return false;
+}

arch/s390/mm/Makefile +10 -0

+static int count_contiguous(pte_t *start, pte_t *end, bool *valid)
+{
+	pte_t pte = __ptep_get(start);
+	pte_t *ptep;
+
+	*valid = !(pte_val(pte) & _PAGE_INVALID);
+
+	for (ptep = start + 1; ptep < end; ptep++) {
+		pte = __ptep_get(ptep);
+		if (*valid) {

Quoted 7 lines

With this change, the system calls do:

lazy_mmu_mode_enable_pte()
...
<store new PTE values in the per-CPU cache>
...
lazy_mmu_mode_disable() // apply cache with one multi-IPTE

I think what is not necessarily immediately obvious: this approach must assure that within such a lazy mmu section there is not a single occurrence of code which doesn't use the above mentioned modified primitives to dereference page table entry pointers.

Directly dereferencing such pointers would bypass the cache and lead to incorrect results. Therefore we do need some mechanism which makes sure this cannot happen. Preferebly that would happen at compile time with static code analysis. Alternatively your Kasan implementation would be helpful to find something like that after-the-fact.

However in any case we need something to address this problem.

arch/s390/include/asm/pgtable.h enum { · line 92

Quoted 4 lines

arch/s390/include/asm/pgtable.h +3 -0

+bool ipte_batch_ptep_test_and_clear_young(struct vm_area_struct *vma,
+					  unsigned long addr, pte_t *ptep,
+					  int *res);

Could you change this to something like:

bool __ipte_batch_ptep_test_and_clear_young(struct vm_area_struct *vma,

					    unsigned long addr, pte_t *ptep,
					    int *res);

static inline
bool ipte_batch_ptep_test_and_clear_young(struct vm_area_struct *vma,
unsigned long addr, pte_t *ptep, int *res)

{
	if (__is_defined(__DECOMPRESSOR))
		return false;
	return __ipte_batch_ptep_test_and_clear_young(vma, addr, ptep, res);
}

? This avoids the ifdef and makes the code easier to read and change.

arch/s390/include/asm/pgtable.h static inline void set_pmd(pmd_t *pmdp, pmd_t pmd) · line 1055

Quoted 5 lines

arch/s390/include/asm/pgtable.h +5 -0

+static inline void set_pte(pte_t *ptep, pte_t pte)
+{
+	if (!ipte_batch_set_pte(ptep, pte))
+		__set_pte(ptep, pte);
+}

Not sure if you analyzed it, but it looks like this might be the reason why you see the fork() slowdown: every page table operation now comes with a function call, even if is not needed.

I guess it would be helpful to add an early exit to the ipte_batch() inlines. E.g. going back to the example above:

static inline
bool ipte_batch_ptep_test_and_clear_young(struct vm_area_struct *vma,
unsigned long addr, pte_t *ptep, int *res)

{
	if (__is_defined(__DECOMPRESSOR))
		return false;
--->	if (unlikely(!ipte_batch_active()))
--->		return false;
	return __ipte_batch_ptep_test_and_clear_young(vma, addr, ptep, res);
}

Where ipte_batch_active() would be an inline function which simply tests a bit in lowcore.

Quoted 1 line

+#define PTE_POISON 0

Is there a reason why the PTE_POISON value is zero? If it can be a different value, I would like to propose a PTE which has bit 11 set (the one which must be zero). If that would ever be transferred to page table, and it would be used, we would see that immediately :)

Quoted 7 lines

arch/s390/mm/ipte_batch.c new file +7 -0

+struct ipte_batch {
+	struct mm_struct *mm;
+	unsigned long base_addr;
+	unsigned long base_end;
+	pte_t *base_pte;
+	pte_t *start_pte;
+	pte_t *end_pte;

arch/s390/mm/ipte_batch.c line 18

Quoted 4 lines

arch/s390/mm/ipte_batch.c new file +4 -0

+	pte_t cache[PTRS_PER_PTE];
+};
+
+static DEFINE_PER_CPU(struct ipte_batch, ipte_range);

This is ~1MB with 512 possible CPUs. I think it would make sense to allocate and free this data structure with CPU hotplug to avoid wasting too much memory.

Quoted 14 lines

arch/s390/mm/ipte_batch.c new file +14 -0

+static int count_contiguous(pte_t *start, pte_t *end, bool *valid)
+{
+	pte_t pte = __ptep_get(start);
+	pte_t *ptep;
+
+	*valid = !(pte_val(pte) & _PAGE_INVALID);
+
+	for (ptep = start + 1; ptep < end; ptep++) {
+		pte = __ptep_get(ptep);
+		if (*valid) {
+			if (pte_val(pte) & _PAGE_INVALID)
+				break;
+		} else {
+			if (!(pte_val(pte) & _PAGE_INVALID))

arch/s390/mm/ipte_batch.c line 39

Quoted 6 lines

arch/s390/mm/ipte_batch.c new file +6 -0

+				break;
+		}
+	}
+
+	return ptep - start;
+}

Wouldn't it be possible to shorten this a bit? E.g.:

static int count_contiguous(pte_t *start, pte_t *end, bool *valid)

{
	unsigned long inv;
	pte_t *ptep;

	inv = pte_val(__ptep_get(start)) & _PAGE_INVALID;
	*valid = !inv;
	for (ptep = start + 1; ptep < end; ptep++) {
		if (pte_val(__ptep_get(ptep)) & _PAGE_INVALID != inv)
			break;
	}
	return ptep - start;
}

arch/s390/mm/ipte_batch.c line 46

Quoted 6 lines

arch/s390/mm/ipte_batch.c new file +6 -0

+static void __invalidate_pte_range(struct mm_struct *mm, unsigned long addr,
+				   int nr_ptes, pte_t *ptep)
+{
+	atomic_inc(&mm->context.flush_count);
+	if (cpu_has_tlb_lc() &&
+	    cpumask_equal(mm_cpumask(mm), cpumask_of(smp_processor_id())))

One line please. Yes, I know, this is more or less copy-pasted, but... :)

Just a general comment about the naming conventions: imho ipte_batch is not a nice choice, since the name of the facility is "ipte range". However I would abstract even more, since nobody knows if there will be a different instruction or facility to achieve all of this in a better way.

Anyway... maybe rename the file simply to mmu.c or tlb.c and change the function prefixes accordingly so we end up with shorter function names?

Quoted 25 lines · Heiko Carstens

arch/s390/include/asm/pgtable.h static inline void set_pmd(pmd_t *pmdp, pmd_t pmd) · line 1055

Quoted 5 lines

arch/s390/include/asm/pgtable.h +5 -0

+static inline void set_pte(pte_t *ptep, pte_t pte)
+{
+	if (!ipte_batch_set_pte(ptep, pte))
+		__set_pte(ptep, pte);
+}

Not sure if you analyzed it, but it looks like this might be the reason why you see the fork() slowdown: every page table operation now comes with a function call, even if is not needed.

I guess it would be helpful to add an early exit to the ipte_batch() inlines. E.g. going back to the example above:

static inline
bool ipte_batch_ptep_test_and_clear_young(struct vm_area_struct *vma,
unsigned long addr, pte_t *ptep, int *res)

{
	if (__is_defined(__DECOMPRESSOR))
		return false;
--->	if (unlikely(!ipte_batch_active()))
--->		return false;
	return __ipte_batch_ptep_test_and_clear_young(vma, addr, ptep, res);
}

Quoted 2 lines

Where ipte_batch_active() would be an inline function which simply tests a bit in lowcore.

The bit check alone would not be enough - it should be done with the preemption disabled. Then it would be something like lazy_mmu_mode(), but with one or more values in the lowcore instead of percpu variable (at least ::base_pte to check whether ptep falls into the active range).

Quoted 7 lines

Just a general comment about the naming conventions: imho ipte_batch is not a nice choice, since the name of the facility is "ipte range". However I would abstract even more, since nobody knows if there will be a different instruction or facility to achieve all of this in a better way.

Anyway... maybe rename the file simply to mmu.c or tlb.c and change the function prefixes accordingly so we end up with shorter function names?

I would in turn suggest lazy_mmu_ prefix and lazy_mmu.c source name to emphasize it implements the generic lazy mmu mode.

At the same time
keep ipte_batch (or ipte_range rather) in the implementation itself.

Quoted 7 lines

With this change, the system calls do:

lazy_mmu_mode_enable_pte()
...
<store new PTE values in the per-CPU cache>
...
lazy_mmu_mode_disable() // apply cache with one multi-IPTE

I think what is not necessarily immediately obvious: this approach must assure that within such a lazy mmu section there is not a single occurrence of code which doesn't use the above mentioned modified primitives to dereference page table entry pointers.

Directly dereferencing such pointers would bypass the cache and lead to incorrect results. Therefore we do need some mechanism which makes sure this cannot happen. Preferebly that would happen at compile time with static code analysis. Alternatively your Kasan implementation would be helpful to find something like that after-the-fact.

However in any case we need something to address this problem.

Inroduce "lazy_mmu" kernel command line parameter
to allow disabling of the lazy MMU mode on boot.

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

---
 arch/s390/mm/ipte_batch.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

static DEFINE_PER_CPU(struct ipte_batch, ipte_range);

arch/s390/mm/ipte_batch.c +16 -0

+static DEFINE_STATIC_KEY_TRUE_RO(lazy_mmu);
+
+static int __init setup_lazy_mmu(char *str)
+{
+	bool enable;
+
+	if (kstrtobool(str, &enable)) {
+		pr_warn("Failed to setup lazy MMU mode, set to enabled\n");
+	} else if (!enable) {
+		pr_warn("Disabling lazy MMU mode\n");
+		static_key_disable(&lazy_mmu.key);
+	}
+
+	return 0;
+}
+early_param("lazy_mmu", setup_lazy_mmu);
 
 static int count_contiguous(pte_t *start, pte_t *end, bool *valid)
 {

arch/s390/mm/ipte_batch.c void arch_enter_lazy_mmu_mode_for_pte_range(struct mm_struct *mm, · line 189

Quoted 6 lines

arch/s390/mm/ipte_batch.c +2 -0

@@ -169,6 +185,8 @@ void arch_enter_lazy_mmu_mode_for_pte_range(struct mm_struct *mm,
 {
 	if (!test_facility(13))
 		return;
+	if (!static_branch_likely(&lazy_mmu))
+		return;

Looks like there is an alternative chicken switch available: remove facility bit 13 from the facility list via kernel command line option.

Unless I am mistaken: that should be sufficient and avoids adding more code.

Quoted 10 lines

arch/s390/mm/ipte_batch.c void arch_enter_lazy_mmu_mode_for_pte_range(struct mm_struct *mm, · line 189

Quoted 6 lines

arch/s390/mm/ipte_batch.c +2 -0

@@ -169,6 +185,8 @@ void arch_enter_lazy_mmu_mode_for_pte_range(struct mm_struct *mm,
  {
  	if (!test_facility(13))
  		return;
+	if (!static_branch_likely(&lazy_mmu))
+		return;

Looks like there is an alternative chicken switch available: remove facility bit 13 from the facility list via kernel command line option.

Quoted 2 lines

Unless I am mistaken: that should be sufficient and avoids adding more code.

But that would also withdraw multi-IPTE from other unrelated parts of the code, e.g. from __kernel_map_pages().