The atomic RMW instructions, for example, ldadd, actually does load +
add + store in one instruction, it will trigger two page faults per the
ARM64 architecture spec, the first fault is a read fault, the second
fault is a write fault.
Some applications use atomic RMW instructions to populate memory, for
example, openjdk uses atomic-add-0 to do pretouch (populate heap memory
at launch time) between v18 and v22 in order to permit use of memory
concurrently with pretouch.
But the double page fault has some problems:
1. Noticeable TLB overhead. The kernel actually installs zero page with
readonly PTE for the read fault. The write fault will trigger a
write-protection fault (CoW). The CoW will allocate a new page and
make the PTE point to the new page, this needs TLB invalidations. The
tlb invalidation and the mandatory memory barriers may incur
significant overhead, particularly on the machines with many cores.
2. Break up huge pages. If THP is on the read fault will install huge
zero pages. The later CoW will break up the huge page and allocate
base pages instead of huge page. The applications have to rely on
khugepaged (kernel thread) to collapse huge pages asynchronously.
This also incurs noticeable performance penalty.
3. 512x page faults with huge page. Due to #2, the applications have to
have page faults for every 4K area for the write, this makes the speed
up by using huge page actually gone.
So it sounds pointless to have two page faults since we know the memory
will be definitely written very soon. Forcing write fault for atomic RMW
instruction makes some sense and it can solve the aforementioned problems:
Firstly, it just allocates zero'ed page, no tlb invalidation and memory
barriers anymore.
Secondly, it can populate writable huge pages in the first place and
don't break them up. Just one page fault is needed for 2M area instrad
of 512 faults and also save cpu time by not using khugepaged.
A simple micro benchmark which populates 1G memory shows the number of
page faults is reduced by half and the time spent by system is reduced
by 60% on a VM running on Ampere Altra platform.
And the benchmark for anonymous read fault on 1G memory, file read fault
on 1G file (cold page cache and warm page cache) don't show noticeable
regression.
Exclude unallocated instructions and LD64B/LDAPR instructions.
Some other architectures also have code inspection in page fault path,
for example, SPARC and x86.
Signed-off-by: Yang Shi <[email protected]>
---
arch/arm64/include/asm/insn.h | 17 ++++++++++++++
arch/arm64/mm/fault.c | 43 +++++++++++++++++++++++++++++++++++
2 files changed, 60 insertions(+)
v3: Exclude unallocated insns and LD64B/LDAPR per Catalin. And thanks
for D Scott help figure out the minimum conditions.
v2: 1. Made commit log more precise per Anshuman and Catalin
2. Made pagefault_disable/enable window narrower per Anshuman
3. Covered CAS and CASP variants per Catalin
4. Put instruction fetching and decoding into a helper function and
take into account endianess per Catalin
5. Don't fetch and decode insn for 32 bit mode (compat) per Catalin
6. More performance tests and exec-only test per Anshuman and Catalin
diff --git a/arch/arm64/include/asm/insn.h b/arch/arm64/include/asm/insn.h
index 8c0a36f72d6f..4e0aa6738579 100644
--- a/arch/arm64/include/asm/insn.h
+++ b/arch/arm64/include/asm/insn.h
@@ -325,6 +325,7 @@ static __always_inline u32 aarch64_insn_get_##abbr##_value(void) \
* "-" means "don't care"
*/
__AARCH64_INSN_FUNCS(class_branch_sys, 0x1c000000, 0x14000000)
+__AARCH64_INSN_FUNCS(class_atomic, 0x3b200c00, 0x38200000)
__AARCH64_INSN_FUNCS(adr, 0x9F000000, 0x10000000)
__AARCH64_INSN_FUNCS(adrp, 0x9F000000, 0x90000000)
@@ -345,6 +346,7 @@ __AARCH64_INSN_FUNCS(ldeor, 0x3F20FC00, 0x38202000)
__AARCH64_INSN_FUNCS(ldset, 0x3F20FC00, 0x38203000)
__AARCH64_INSN_FUNCS(swp, 0x3F20FC00, 0x38208000)
__AARCH64_INSN_FUNCS(cas, 0x3FA07C00, 0x08A07C00)
+__AARCH64_INSN_FUNCS(casp, 0xBFA07C00, 0x08207C00)
__AARCH64_INSN_FUNCS(ldr_reg, 0x3FE0EC00, 0x38606800)
__AARCH64_INSN_FUNCS(signed_ldr_reg, 0X3FE0FC00, 0x38A0E800)
__AARCH64_INSN_FUNCS(ldr_imm, 0x3FC00000, 0x39400000)
@@ -549,6 +551,21 @@ static __always_inline bool aarch64_insn_uses_literal(u32 insn)
aarch64_insn_is_prfm_lit(insn);
}
+static __always_inline bool aarch64_insn_is_class_cas(u32 insn)
+{
+ return aarch64_insn_is_cas(insn) ||
+ aarch64_insn_is_casp(insn);
+}
+
+/* Exclude unallocated atomic instructions and LD64B/LDAPR */
+static __always_inline bool aarch64_atomic_insn_has_wr_perm(u32 insn)
+{
+ return (((insn & 0x3f207c00) == 0x38200000) |
+ ((insn & 0x3f208c00) == 0x38200000) |
+ ((insn & 0x7fe06c00) == 0x78202000) |
+ ((insn & 0xbf204c00) == 0x38200000));
+}
+
enum aarch64_insn_encoding_class aarch64_get_insn_class(u32 insn);
u64 aarch64_insn_decode_immediate(enum aarch64_insn_imm_type type, u32 insn);
u32 aarch64_insn_encode_immediate(enum aarch64_insn_imm_type type,
diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
index 8251e2fea9c7..1ed1b061ee8f 100644
--- a/arch/arm64/mm/fault.c
+++ b/arch/arm64/mm/fault.c
@@ -519,6 +519,35 @@ static bool is_write_abort(unsigned long esr)
return (esr & ESR_ELx_WNR) && !(esr & ESR_ELx_CM);
}
+static bool is_el0_atomic_instr(struct pt_regs *regs)
+{
+ u32 insn;
+ __le32 insn_le;
+ unsigned long pc = instruction_pointer(regs);
+
+ if (!user_mode(regs) || compat_user_mode(regs))
+ return false;
+
+ pagefault_disable();
+ if (get_user(insn_le, (__le32 __user *)pc)) {
+ pagefault_enable();
+ return false;
+ }
+ pagefault_enable();
+
+ insn = le32_to_cpu(insn_le);
+
+ if (aarch64_insn_is_class_atomic(insn)) {
+ if (aarch64_atomic_insn_has_wr_perm(insn))
+ return true;
+ }
+
+ if (aarch64_insn_is_class_cas(insn))
+ return true;
+
+ return false;
+}
+
static int __kprobes do_page_fault(unsigned long far, unsigned long esr,
struct pt_regs *regs)
{
@@ -529,6 +558,7 @@ static int __kprobes do_page_fault(unsigned long far, unsigned long esr,
unsigned int mm_flags = FAULT_FLAG_DEFAULT;
unsigned long addr = untagged_addr(far);
struct vm_area_struct *vma;
+ bool force_write = false;
if (kprobe_page_fault(regs, esr))
return 0;
@@ -557,6 +587,11 @@ static int __kprobes do_page_fault(unsigned long far, unsigned long esr,
/* It was write fault */
vm_flags = VM_WRITE;
mm_flags |= FAULT_FLAG_WRITE;
+ } else if (is_el0_atomic_instr(regs)) {
+ /* Force write fault */
+ vm_flags = VM_WRITE;
+ mm_flags |= FAULT_FLAG_WRITE;
+ force_write = true;
} else {
/* It was read fault */
vm_flags = VM_READ;
@@ -586,6 +621,14 @@ static int __kprobes do_page_fault(unsigned long far, unsigned long esr,
if (!vma)
goto lock_mmap;
+ /* vma flags don't allow write, undo force write */
+ if (force_write && !(vma->vm_flags & VM_WRITE)) {
+ vm_flags |= VM_READ;
+ if (!alternative_has_cap_unlikely(ARM64_HAS_EPAN))
+ vm_flags |= VM_EXEC;
+ mm_flags &= ~FAULT_FLAG_WRITE;
+ }
+
if (!(vma->vm_flags & vm_flags)) {
vma_end_read(vma);
goto lock_mmap;
--
2.41.0
On Tue, Jun 04, 2024 at 10:15:16AM -0700, Yang Shi wrote:
> diff --git a/arch/arm64/include/asm/insn.h b/arch/arm64/include/asm/insn.h
> index 8c0a36f72d6f..4e0aa6738579 100644
> --- a/arch/arm64/include/asm/insn.h
> +++ b/arch/arm64/include/asm/insn.h
> @@ -325,6 +325,7 @@ static __always_inline u32 aarch64_insn_get_##abbr##_value(void) \
> * "-" means "don't care"
> */
> __AARCH64_INSN_FUNCS(class_branch_sys, 0x1c000000, 0x14000000)
> +__AARCH64_INSN_FUNCS(class_atomic, 0x3b200c00, 0x38200000)
>
> __AARCH64_INSN_FUNCS(adr, 0x9F000000, 0x10000000)
> __AARCH64_INSN_FUNCS(adrp, 0x9F000000, 0x90000000)
> @@ -345,6 +346,7 @@ __AARCH64_INSN_FUNCS(ldeor, 0x3F20FC00, 0x38202000)
> __AARCH64_INSN_FUNCS(ldset, 0x3F20FC00, 0x38203000)
> __AARCH64_INSN_FUNCS(swp, 0x3F20FC00, 0x38208000)
> __AARCH64_INSN_FUNCS(cas, 0x3FA07C00, 0x08A07C00)
> +__AARCH64_INSN_FUNCS(casp, 0xBFA07C00, 0x08207C00)
> __AARCH64_INSN_FUNCS(ldr_reg, 0x3FE0EC00, 0x38606800)
> __AARCH64_INSN_FUNCS(signed_ldr_reg, 0X3FE0FC00, 0x38A0E800)
> __AARCH64_INSN_FUNCS(ldr_imm, 0x3FC00000, 0x39400000)
> @@ -549,6 +551,21 @@ static __always_inline bool aarch64_insn_uses_literal(u32 insn)
> aarch64_insn_is_prfm_lit(insn);
> }
>
> +static __always_inline bool aarch64_insn_is_class_cas(u32 insn)
> +{
> + return aarch64_insn_is_cas(insn) ||
> + aarch64_insn_is_casp(insn);
> +}
> +
> +/* Exclude unallocated atomic instructions and LD64B/LDAPR */
> +static __always_inline bool aarch64_atomic_insn_has_wr_perm(u32 insn)
> +{
> + return (((insn & 0x3f207c00) == 0x38200000) |
> + ((insn & 0x3f208c00) == 0x38200000) |
> + ((insn & 0x7fe06c00) == 0x78202000) |
> + ((insn & 0xbf204c00) == 0x38200000));
Please use the logical || instead of the bitwise operator. You can also
remove the outer brackets.
That said, the above is pretty opaque if we want to update it in the
future. I have no idea how it was generated or whether it's correct. At
least maybe add a comment on how you got to these masks and values.
> diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
> index 8251e2fea9c7..1ed1b061ee8f 100644
> --- a/arch/arm64/mm/fault.c
> +++ b/arch/arm64/mm/fault.c
> @@ -519,6 +519,35 @@ static bool is_write_abort(unsigned long esr)
> return (esr & ESR_ELx_WNR) && !(esr & ESR_ELx_CM);
> }
>
> +static bool is_el0_atomic_instr(struct pt_regs *regs)
> +{
> + u32 insn;
> + __le32 insn_le;
> + unsigned long pc = instruction_pointer(regs);
> +
> + if (!user_mode(regs) || compat_user_mode(regs))
> + return false;
> +
> + pagefault_disable();
> + if (get_user(insn_le, (__le32 __user *)pc)) {
> + pagefault_enable();
> + return false;
> + }
> + pagefault_enable();
> +
> + insn = le32_to_cpu(insn_le);
> +
> + if (aarch64_insn_is_class_atomic(insn)) {
> + if (aarch64_atomic_insn_has_wr_perm(insn))
> + return true;
> + }
Nitpick:
if (aarch64_insn_is_class_atomic(insn) &&
aarch64_atomic_insn_has_wr_perm(insn))
return true;
(less indentation)
> @@ -557,6 +587,11 @@ static int __kprobes do_page_fault(unsigned long far, unsigned long esr,
> /* It was write fault */
> vm_flags = VM_WRITE;
> mm_flags |= FAULT_FLAG_WRITE;
> + } else if (is_el0_atomic_instr(regs)) {
> + /* Force write fault */
> + vm_flags = VM_WRITE;
> + mm_flags |= FAULT_FLAG_WRITE;
> + force_write = true;
> } else {
> /* It was read fault */
> vm_flags = VM_READ;
> @@ -586,6 +621,14 @@ static int __kprobes do_page_fault(unsigned long far, unsigned long esr,
> if (!vma)
> goto lock_mmap;
>
> + /* vma flags don't allow write, undo force write */
> + if (force_write && !(vma->vm_flags & VM_WRITE)) {
> + vm_flags |= VM_READ;
> + if (!alternative_has_cap_unlikely(ARM64_HAS_EPAN))
> + vm_flags |= VM_EXEC;
> + mm_flags &= ~FAULT_FLAG_WRITE;
> + }
Ah, this revert to the non-write flags doesn't look great as we
basically duplicate the 'else' block in the original check. So, it
probably look better as per your earlier patch to just do the
instruction read just before the !(vma->vm_flags & flags) check,
something like:
if ((vma->vm_flags & VM_WRITE) && is_el0_atomic_instr(regs)) {
vm_flags = VM_WRITE;
mm_flags |= FAULT_FLAG_WRITE;
}
This way we also only read the instruction if the vma is writeable. I
think it's fine to do this under the vma lock since we have
pagefault_disable() for the insn read.
--
Catalin
On 6/5/24 9:54 AM, Catalin Marinas wrote:
> On Tue, Jun 04, 2024 at 10:15:16AM -0700, Yang Shi wrote:
>> diff --git a/arch/arm64/include/asm/insn.h b/arch/arm64/include/asm/insn.h
>> index 8c0a36f72d6f..4e0aa6738579 100644
>> --- a/arch/arm64/include/asm/insn.h
>> +++ b/arch/arm64/include/asm/insn.h
>> @@ -325,6 +325,7 @@ static __always_inline u32 aarch64_insn_get_##abbr##_value(void) \
>> * "-" means "don't care"
>> */
>> __AARCH64_INSN_FUNCS(class_branch_sys, 0x1c000000, 0x14000000)
>> +__AARCH64_INSN_FUNCS(class_atomic, 0x3b200c00, 0x38200000)
>>
>> __AARCH64_INSN_FUNCS(adr, 0x9F000000, 0x10000000)
>> __AARCH64_INSN_FUNCS(adrp, 0x9F000000, 0x90000000)
>> @@ -345,6 +346,7 @@ __AARCH64_INSN_FUNCS(ldeor, 0x3F20FC00, 0x38202000)
>> __AARCH64_INSN_FUNCS(ldset, 0x3F20FC00, 0x38203000)
>> __AARCH64_INSN_FUNCS(swp, 0x3F20FC00, 0x38208000)
>> __AARCH64_INSN_FUNCS(cas, 0x3FA07C00, 0x08A07C00)
>> +__AARCH64_INSN_FUNCS(casp, 0xBFA07C00, 0x08207C00)
>> __AARCH64_INSN_FUNCS(ldr_reg, 0x3FE0EC00, 0x38606800)
>> __AARCH64_INSN_FUNCS(signed_ldr_reg, 0X3FE0FC00, 0x38A0E800)
>> __AARCH64_INSN_FUNCS(ldr_imm, 0x3FC00000, 0x39400000)
>> @@ -549,6 +551,21 @@ static __always_inline bool aarch64_insn_uses_literal(u32 insn)
>> aarch64_insn_is_prfm_lit(insn);
>> }
>>
>> +static __always_inline bool aarch64_insn_is_class_cas(u32 insn)
>> +{
>> + return aarch64_insn_is_cas(insn) ||
>> + aarch64_insn_is_casp(insn);
>> +}
>> +
>> +/* Exclude unallocated atomic instructions and LD64B/LDAPR */
>> +static __always_inline bool aarch64_atomic_insn_has_wr_perm(u32 insn)
>> +{
>> + return (((insn & 0x3f207c00) == 0x38200000) |
>> + ((insn & 0x3f208c00) == 0x38200000) |
>> + ((insn & 0x7fe06c00) == 0x78202000) |
>> + ((insn & 0xbf204c00) == 0x38200000));
> Please use the logical || instead of the bitwise operator. You can also
> remove the outer brackets.
OK
>
> That said, the above is pretty opaque if we want to update it in the
> future. I have no idea how it was generated or whether it's correct. At
> least maybe add a comment on how you got to these masks and values.
It was generated by a script using Python sympy module, which could help
figure out the most simplified condition.
>
>> diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
>> index 8251e2fea9c7..1ed1b061ee8f 100644
>> --- a/arch/arm64/mm/fault.c
>> +++ b/arch/arm64/mm/fault.c
>> @@ -519,6 +519,35 @@ static bool is_write_abort(unsigned long esr)
>> return (esr & ESR_ELx_WNR) && !(esr & ESR_ELx_CM);
>> }
>>
>> +static bool is_el0_atomic_instr(struct pt_regs *regs)
>> +{
>> + u32 insn;
>> + __le32 insn_le;
>> + unsigned long pc = instruction_pointer(regs);
>> +
>> + if (!user_mode(regs) || compat_user_mode(regs))
>> + return false;
>> +
>> + pagefault_disable();
>> + if (get_user(insn_le, (__le32 __user *)pc)) {
>> + pagefault_enable();
>> + return false;
>> + }
>> + pagefault_enable();
>> +
>> + insn = le32_to_cpu(insn_le);
>> +
>> + if (aarch64_insn_is_class_atomic(insn)) {
>> + if (aarch64_atomic_insn_has_wr_perm(insn))
>> + return true;
>> + }
> Nitpick:
>
> if (aarch64_insn_is_class_atomic(insn) &&
> aarch64_atomic_insn_has_wr_perm(insn))
> return true;
>
> (less indentation)
Sure
>
>> @@ -557,6 +587,11 @@ static int __kprobes do_page_fault(unsigned long far, unsigned long esr,
>> /* It was write fault */
>> vm_flags = VM_WRITE;
>> mm_flags |= FAULT_FLAG_WRITE;
>> + } else if (is_el0_atomic_instr(regs)) {
>> + /* Force write fault */
>> + vm_flags = VM_WRITE;
>> + mm_flags |= FAULT_FLAG_WRITE;
>> + force_write = true;
>> } else {
>> /* It was read fault */
>> vm_flags = VM_READ;
>> @@ -586,6 +621,14 @@ static int __kprobes do_page_fault(unsigned long far, unsigned long esr,
>> if (!vma)
>> goto lock_mmap;
>>
>> + /* vma flags don't allow write, undo force write */
>> + if (force_write && !(vma->vm_flags & VM_WRITE)) {
>> + vm_flags |= VM_READ;
>> + if (!alternative_has_cap_unlikely(ARM64_HAS_EPAN))
>> + vm_flags |= VM_EXEC;
>> + mm_flags &= ~FAULT_FLAG_WRITE;
>> + }
> Ah, this revert to the non-write flags doesn't look great as we
> basically duplicate the 'else' block in the original check. So, it
> probably look better as per your earlier patch to just do the
> instruction read just before the !(vma->vm_flags & flags) check,
> something like:
>
> if ((vma->vm_flags & VM_WRITE) && is_el0_atomic_instr(regs)) {
> vm_flags = VM_WRITE;
> mm_flags |= FAULT_FLAG_WRITE;
> }
>
> This way we also only read the instruction if the vma is writeable. I
> think it's fine to do this under the vma lock since we have
> pagefault_disable() for the insn read.
Yes, I agree.
>
>> @@ -557,6 +587,11 @@ static int __kprobes do_page_fault(unsigned long far, unsigned long esr,
>> /* It was write fault */
>> vm_flags = VM_WRITE;
>> mm_flags |= FAULT_FLAG_WRITE;
>> + } else if (is_el0_atomic_instr(regs)) {
>> + /* Force write fault */
>> + vm_flags = VM_WRITE;
>> + mm_flags |= FAULT_FLAG_WRITE;
>> + force_write = true;
>> } else {
>> /* It was read fault */
>> vm_flags = VM_READ;
>> @@ -586,6 +621,14 @@ static int __kprobes do_page_fault(unsigned long far, unsigned long esr,
>> if (!vma)
>> goto lock_mmap;
>>
>> + /* vma flags don't allow write, undo force write */
>> + if (force_write && !(vma->vm_flags & VM_WRITE)) {
>> + vm_flags |= VM_READ;
>> + if (!alternative_has_cap_unlikely(ARM64_HAS_EPAN))
>> + vm_flags |= VM_EXEC;
>> + mm_flags &= ~FAULT_FLAG_WRITE;
>> + }
> Ah, this revert to the non-write flags doesn't look great as we
> basically duplicate the 'else' block in the original check. So, it
> probably look better as per your earlier patch to just do the
> instruction read just before the !(vma->vm_flags & flags) check,
> something like:
>
> if ((vma->vm_flags & VM_WRITE) && is_el0_atomic_instr(regs)) {
> vm_flags = VM_WRITE;
> mm_flags |= FAULT_FLAG_WRITE;
> }
>
> This way we also only read the instruction if the vma is writeable. I
> think it's fine to do this under the vma lock since we have
> pagefault_disable() for the insn read.
I think we also need to skip this for write fault and instruction fault.
Some something like:
@@ -529,6 +557,7 @@ static int __kprobes do_page_fault(unsigned long
far, unsigned long esr,
unsigned int mm_flags = FAULT_FLAG_DEFAULT;
unsigned long addr = untagged_addr(far);
struct vm_area_struct *vma;
+ bool may_force_write = false;
if (kprobe_page_fault(regs, esr))
return 0;
@@ -565,6 +594,7 @@ static int __kprobes do_page_fault(unsigned long
far, unsigned long esr,
/* If EPAN is absent then exec implies read */
if (!alternative_has_cap_unlikely(ARM64_HAS_EPAN))
vm_flags |= VM_EXEC;
+ may_force_write = true;
}
if (is_ttbr0_addr(addr) && is_el1_permission_fault(addr, esr,
regs)) {
@@ -586,6 +616,12 @@ static int __kprobes do_page_fault(unsigned long
far, unsigned long esr,
if (!vma)
goto lock_mmap;
+ if (may_force_write && (vma->vm_flags & VM_WRITE) &&
+ is_el0_atomic_instr(regs)) {
+ vm_flags = VM_WRITE;
+ mm_flags |= FAULT_FLAG_WRITE;
+ }
+
if (!(vma->vm_flags & vm_flags)) {
vma_end_read(vma);
goto lock_mmap;
>