arm64/mm: Elide TLB flush in certain pte protection transitions
PATCH 1/1 arm64/mm: Elide TLB flush in certain pte protection transitions
Currently arm64 does an unconditional TLB flush in mprotect(). This is not required for some cases, for example, when changing from PROT_NONE to PROT_READ | PROT_WRITE (a real usecase - glibc malloc does this to emulate growing into the non-main heaps), and unsetting uffd-wp in a range.
Therefore, implement pte_needs_flush() for arm64, which is already implemented by some other arches as well.
Running a userspace program changing permissions back and forth between PROT_NONE and PROT_READ | PROT_WRITE, and measuring the average time taken for the none->rw transition, I get a reduction from 3.2 microseconds to 2.95 microseconds, giving an 8.5% improvement.
---
mm-selftests pass. Based on 6.17-rc6.
arch/arm64/include/asm/tlbflush.h | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+)
arch/arm64/include/asm/tlbflush.h +29 -0
@@ -524,6 +524,35 @@ static inline void arch_tlbbatch_add_pending(struct arch_tlbflush_unmap_batch *b {__flush_tlb_range_nosync (mm, start, end,PAGE_SIZE , true,3 ); } + +static inline bool __pte_flags_need_flush (pteval_t oldval,pteval_t newval) +{ +pteval_t diff = oldval ^ newval; + +/* invalid to valid transition requires no flush */ +if (!(oldval &PTE_VALID ) || (oldval &PTE_PRESENT_INVALID )) +return false; + +/* Transition in the SW bits and access flag requires no flush */ + diff &= ~(PTE_SWBITS_MASK |PTE_AF ); + +if (!diff) +return false; +return true; +} + +static inline bool pte_needs_flush (pte_t oldpte,pte_t newpte) +{ +return __pte_flags_need_flush (pte_val (oldpte),pte_val (newpte)); +} +#define pte_needs_flush pte_needs_flush + +static inline bool huge_pmd_needs_flush (pmd_t oldpmd,pmd_t newpmd) +{ +return __pte_flags_need_flush (pmd_val (oldpmd),pmd_val (newpmd)); +} +#define huge_pmd_needs_flush huge_pmd_needs_flush +#endif #endif
Signature · 1 line
2.30.2
Quoted 5 lines · Dev Jain
Currently arm64 does an unconditional TLB flush in
mprotect(). This is not required for some cases, for example, when changing fromPROT_NONEtoPROT_READ|PROT_WRITE(a real usecase - glibc malloc does this to emulate growing into the non-main heaps), and unsetting uffd-wp in a range.
Quoted 8 lines
Therefore, implement
pte_needs_flush()for arm64, which is already implemented by some other arches as well.Running a userspace program changing permissions back and forth between
PROT_NONEandPROT_READ|PROT_WRITE, and measuring the average time taken for thenone->rwtransition, I get a reduction from 3.2 microseconds to 2.95 microseconds, giving an 8.5% improvement.
Hi Dev，
Quoted 22 lines
Signed-off-by Dev Jain <[email protected]>---
mm-selftests pass. Based on 6.17-rc6.arch/arm64/include/asm/tlbflush.h | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+)arch/arm64/include/asm/tlbflush.h +7
-0@@ -524,6 +524,35 @@ static inline void arch_tlbbatch_add_pending(struct arch_tlbflush_unmap_batch *b {__flush_tlb_range_nosync (mm, start, end,PAGE_SIZE , true,3 ); } + +static inline bool __pte_flags_need_flush (pteval_t oldval,pteval_t newval) +{ +pteval_t diff = oldval ^ newval; + +/* invalid to valid transition requires no flush */ +if (!(oldval &PTE_VALID ) || (oldval &PTE_PRESENT_INVALID ))
arch/arm64/include/asm/tlbflush.h static inline void arch_tlbbatch_add_pending(struct arch_tlbflush_unmap_batch *b · line 543
Quoted 10 lines
arch/arm64/include/asm/tlbflush.h +10
-0+return false; + +/* Transition in the SW bits and access flag requires no flush */ + diff &= ~(PTE_SWBITS_MASK |PTE_AF ); + +if (!diff) +return false; +return true; +} +
LibMicro mprotect testcase show 3~5% improvement with different size in
old kernel(we did this before, but only check PTE_VALID and PTE_PROT_NONE in our kernel), it seems that no one change other sw bit by mprotect?
Anyway, Reviewed-by: Kefeng Wang <[email protected]>
Quoted 15 lines
arch/arm64/include/asm/tlbflush.h +12
-0+static inline bool pte_needs_flush (pte_t oldpte,pte_t newpte) +{ +return __pte_flags_need_flush (pte_val (oldpte),pte_val (newpte)); +} +#define pte_needs_flush pte_needs_flush + +static inline bool huge_pmd_needs_flush (pmd_t oldpmd,pmd_t newpmd) +{ +return __pte_flags_need_flush (pmd_val (oldpmd),pmd_val (newpmd)); +} +#define huge_pmd_needs_flush huge_pmd_needs_flush +#endif #endif
Quoted 48 lines · Kefeng Wang
Quoted 5 lines · Dev Jain
Currently arm64 does an unconditional TLB flush in
mprotect(). This is not required for some cases, for example, when changing fromPROT_NONEtoPROT_READ|PROT_WRITE(a real usecase - glibc malloc does this to emulate growing into the non-main heaps), and unsetting uffd-wp in a range.Quoted 8 lines
Therefore, implement
pte_needs_flush()for arm64, which is already implemented by some other arches as well.Running a userspace program changing permissions back and forth between
PROT_NONEandPROT_READ|PROT_WRITE, and measuring the average time taken for thenone->rwtransition, I get a reduction from 3.2 microseconds to 2.95 microseconds, giving an 8.5% improvement.Hi Dev，
Quoted 29 lines
Signed-off-by Dev Jain <[email protected]>---
mm-selftests pass. Based on 6.17-rc6.
arch/arm64/include/asm/tlbflush.h| 29 +++++++++++++++++++++++++++++1 file changed, 29 insertions(+)arch/arm64/include/asm/tlbflush.h +0
-0@@ -524,6 +524,35 @@ static inline voidarch_tlbbatch_add_pending(struct
arch_tlbflush_unmap_batch*b {
__flush_tlb_range_nosync(mm, start, end, PAGE_SIZE, true, 3); }arch/arm64/include/asm/tlbflush.h +2
-0+ +static inline bool __pte_flags_need_flush(pteval_t oldval, pteval_tnewval)
arch/arm64/include/asm/tlbflush.h +10
-0+{ + pteval_t diff = oldval ^ newval; + + /* invalid to valid transition requires no flush */ + if (!(oldval & PTE_VALID) || (oldval & PTE_PRESENT_INVALID)) + return false; + + /* Transition in the SW bits and access flag requires no flush */ + diff &= ~(PTE_SWBITS_MASK | PTE_AF); +
Quoted 8 lines
arch/arm64/include/asm/tlbflush.h
Quoted 5 lines
+ if (!diff)
+ return false;
+ return true;arch/arm64/include/asm/tlbflush.h +2
-0+} +LibMicro mprotect testcase show 3~5% improvement with different size in
oldkernel(we did this before, but only check PTE_VALID and PTE_PROT_NONE in our kernel), it seems that no one change other sw bit by mprotect?
Not mprotect, but when unsetting uffd-wp, we do mwriteprotect_range -> uffd_wp_range -> change_protection() with MM_CP_UFFD_WP_RESOLVE set.
Quoted 2 lines
Anyway, Reviewed-by: Kefeng Wang <[email protected]>
Thanks!
Quoted 16 lines
Quoted 15 lines
arch/arm64/include/asm/tlbflush.h +12
-0+static inline bool pte_needs_flush (pte_t oldpte,pte_t newpte) +{ + return __pte_flags_need_flush(pte_val(oldpte), pte_val(newpte)); +} +#define pte_needs_flush pte_needs_flush + +static inline bool huge_pmd_needs_flush (pmd_t oldpmd,pmd_t newpmd) +{ + return __pte_flags_need_flush(pmd_val(oldpmd), pmd_val(newpmd)); +} +#define huge_pmd_needs_flush huge_pmd_needs_flush +#endif
#endif
Quoted 25 lines · Dev Jain
Currently arm64 does an unconditional TLB flush in
mprotect(). This is not required for some cases, for example, when changing fromPROT_NONEtoPROT_READ|PROT_WRITE(a real usecase - glibc malloc does this to emulate growing into the non-main heaps), and unsetting uffd-wp in a range.Therefore, implement
pte_needs_flush()for arm64, which is already implemented by some other arches as well.Running a userspace program changing permissions back and forth between
PROT_NONEandPROT_READ|PROT_WRITE, and measuring the average time taken for thenone->rwtransition, I get a reduction from 3.2 microseconds to 2.95 microseconds, giving an 8.5% improvement.Signed-off-by Dev Jain <[email protected]>---
mm-selftests pass. Based on 6.17-rc6.arch/arm64/include/asm/tlbflush.h | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+)arch/arm64/include/asm/tlbflush.h +0
-0
arch/arm64/include/asm/tlbflush.h static inline void arch_tlbbatch_add_pending(struct arch_tlbflush_unmap_batch *b · line 528
Quoted 5 lines
arch/arm64/include/asm/tlbflush.h +2
-0@@ -524,6 +524,35 @@ static inline void arch_tlbbatch_add_pending(struct arch_tlbflush_unmap_batch *b {__flush_tlb_range_nosync (mm, start, end,PAGE_SIZE , true,3 ); } + +static inline bool __pte_flags_need_flush (pteval_t oldval,pteval_t newval)
ptdesc_t is the preferred any-level type.
arch/arm64/include/asm/tlbflush.h static inline void arch_tlbbatch_add_pending(struct arch_tlbflush_unmap_batch *b · line 533
Quoted 5 lines
arch/arm64/include/asm/tlbflush.h +5
-0+{ +pteval_t diff = oldval ^ newval; + +/* invalid to valid transition requires no flush */ +if (!(oldval &PTE_VALID ) || (oldval &PTE_PRESENT_INVALID ))
Is the PTE_PRESENT_INVALID really required? If the oldval was invalid, there can't be a TLB entry for it, so no flush is required; that's it, I think?
In fact, PTE_PRESENT_INVALID is overlaid with PTE_NG; it only means PTE_PRESENT_INVALID when PTE_INVALID=0, so I think this is broken as is. Valid user-space PTEs always have PTE_NG set, so you will never flush.
arch/arm64/include/asm/tlbflush.h static inline void arch_tlbbatch_add_pending(struct arch_tlbflush_unmap_batch *b · line 537
Quoted 4 lines
arch/arm64/include/asm/tlbflush.h +4
-0+return false; + +/* Transition in the SW bits and access flag requires no flush */ + diff &= ~(PTE_SWBITS_MASK |PTE_AF );
Could you explain your thinking on why PTE_AF changes don't need a flush? I would have thought if we want to clear the access flag, that would definitely require a flush? Otherwise how would the MMU know to set the acccess bit on next access if it already has a TLB entry?
arch/arm64/include/asm/tlbflush.h static inline void arch_tlbbatch_add_pending(struct arch_tlbflush_unmap_batch *b · line 541
Quoted 4 lines
arch/arm64/include/asm/tlbflush.h +4
-0+ +if (!diff) +return false; +return true;
Perhaps just "return !!diff;" here?
Thanks,
Ryan
Quoted 17 lines
arch/arm64/include/asm/tlbflush.h +14
-0+} + +static inline bool pte_needs_flush (pte_t oldpte,pte_t newpte) +{ +return __pte_flags_need_flush (pte_val (oldpte),pte_val (newpte)); +} +#define pte_needs_flush pte_needs_flush + +static inline bool huge_pmd_needs_flush (pmd_t oldpmd,pmd_t newpmd) +{ +return __pte_flags_need_flush (pmd_val (oldpmd),pmd_val (newpmd)); +} +#define huge_pmd_needs_flush huge_pmd_needs_flush +#endif #endif
Quoted 21 lines · Ryan Roberts
Quoted 20 lines · Dev Jain
Currently arm64 does an unconditional TLB flush in
mprotect(). This is not required for some cases, for example, when changing fromPROT_NONEtoPROT_READ|PROT_WRITE(a real usecase - glibc malloc does this to emulate growing into the non-main heaps), and unsetting uffd-wp in a range.Therefore, implement
pte_needs_flush()for arm64, which is already implemented by some other arches as well.Running a userspace program changing permissions back and forth between
PROT_NONEandPROT_READ|PROT_WRITE, and measuring the average time taken for thenone->rwtransition, I get a reduction from 3.2 microseconds to 2.95 microseconds, giving an 8.5% improvement.Signed-off-by Dev Jain <[email protected]>---
mm-selftests pass. Based on 6.17-rc6.arch/arm64/include/asm/tlbflush.h | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+)
Quoted 11 lines
arch/arm64/include/asm/tlbflush.h static inline void arch_tlbbatch_add_pending(struct arch_tlbflush_unmap_batch *b · line 528
Quoted 10 lines
arch/arm64/include/asm/tlbflush.h +2
-0@@ -524,6 +524,35 @@ static inline void arch_tlbbatch_add_pending(struct arch_tlbflush_unmap_batch *b {__flush_tlb_range_nosync (mm, start, end,PAGE_SIZE , true,3 ); } + +static inline bool __pte_flags_need_flush (pteval_t oldval,pteval_t newval)
ptdesc_tis the preferred any-level type.
I keep forgetting this :)
Quoted 9 lines
arch/arm64/include/asm/tlbflush.h static inline void arch_tlbbatch_add_pending(struct arch_tlbflush_unmap_batch *b · line 533
Quoted 6 lines
arch/arm64/include/asm/tlbflush.h +5
-0+{ +pteval_t diff = oldval ^ newval; + +/* invalid to valid transition requires no flush */ +if (!(oldval &PTE_VALID ) || (oldval &PTE_PRESENT_INVALID ))Is the
PTE_PRESENT_INVALIDreally required? If the oldval was invalid, there can't be a TLB entry for it, so no flush is required; that's it, I think?
Quoted 3 lines
In fact,
PTE_PRESENT_INVALIDis overlaid withPTE_NG; it only meansPTE_PRESENT_INVALIDwhen PTE_INVALID=0, so I think this is broken as is. Valid user-space PTEs always havePTE_NGset, so you will never flush.
Not sure I get you. The condition as I wrote means
1. If PTE_VALID is not set, then do not flush.
2. If PTE_VALID is set, *and* PTE_PRESENT_INVALID is set, then do not flush.
So when you say "it only means PTE_PRESENT_INVALID when PTE_INVALID=0", the second condition meets that.
Quoted 9 lines
arch/arm64/include/asm/tlbflush.h static inline void arch_tlbbatch_add_pending(struct arch_tlbflush_unmap_batch *b · line 537
Quoted 5 lines
arch/arm64/include/asm/tlbflush.h +4
-0+return false; + +/* Transition in the SW bits and access flag requires no flush */ + diff &= ~(PTE_SWBITS_MASK |PTE_AF );Could you explain your thinking on why
PTE_AFchanges don't need a flush? I would have thought if we want to clear the access flag, that would definitely require a flush? Otherwise how would the MMU know to set the acccess bit on next access if it already has a TLB entry?
You are correct, but AFAIK losing access bit information is not fatal, it will only mess with page aging. So potentially reclaim will lose some accuracy.
Quoted 6 lines
arch/arm64/include/asm/tlbflush.h static inline void arch_tlbbatch_add_pending(struct arch_tlbflush_unmap_batch *b · line 541
Quoted 5 lines
arch/arm64/include/asm/tlbflush.h +4
-0+ +if (!diff) +return false; +return true;Perhaps just "return !!diff;" here?
Sure.
Quoted 22 lines
Thanks,
RyanQuoted 17 lines
arch/arm64/include/asm/tlbflush.h +14
-0+} + +static inline bool pte_needs_flush (pte_t oldpte,pte_t newpte) +{ +return __pte_flags_need_flush (pte_val (oldpte),pte_val (newpte)); +} +#define pte_needs_flush pte_needs_flush + +static inline bool huge_pmd_needs_flush (pmd_t oldpmd,pmd_t newpmd) +{ +return __pte_flags_need_flush (pmd_val (oldpmd),pmd_val (newpmd)); +} +#define huge_pmd_needs_flush huge_pmd_needs_flush +#endif #endif
Quoted 52 lines · Dev Jain
Quoted 21 lines · Ryan Roberts
Quoted 20 lines · Dev Jain
Currently arm64 does an unconditional TLB flush in
mprotect(). This is not required for some cases, for example, when changing fromPROT_NONEtoPROT_READ|PROT_WRITE(a real usecase - glibc malloc does this to emulate growing into the non-main heaps), and unsetting uffd-wp in a range.Therefore, implement
pte_needs_flush()for arm64, which is already implemented by some other arches as well.Running a userspace program changing permissions back and forth between
PROT_NONEandPROT_READ|PROT_WRITE, and measuring the average time taken for thenone->rwtransition, I get a reduction from 3.2 microseconds to 2.95 microseconds, giving an 8.5% improvement.Signed-off-by Dev Jain <[email protected]>---
mm-selftests pass. Based on 6.17-rc6.
arch/arm64/include/asm/tlbflush.h| 29 +++++++++++++++++++++++++++++1 file changed, 29 insertions(+)Quoted 13 lines
arch/arm64/include/asm/tlbflush.h static inline void arch_tlbbatch_add_pending(struct arch_tlbflush_unmap_batch *b · line 528
Quoted 12 lines
arch/arm64/include/asm/ renamed from arch/arm64/include/asm/tlbflush.h +0
-0
tlbflush.h
index18a5dc0c9a54..4a566d589100100644arch/arm64/include/asm/tlbflush.h +0
-0@@ -524,6 +524,35 @@ static inline void arch_tlbbatch_add_pending(struct
arch_tlbflush_unmap_batch*b
{
__flush_tlb_range_nosync(mm, start, end, PAGE_SIZE, true, 3); }arch/arm64/include/asm/tlbflush.h +2
-0+ +static inline bool __pte_flags_need_flush (pteval_t oldval,pteval_t newval)
ptdesc_tis the preferred any-level type.I keep forgetting this :)
Quoted 9 lines
arch/arm64/include/asm/tlbflush.h static inline void arch_tlbbatch_add_pending(struct arch_tlbflush_unmap_batch *b · line 533
Quoted 6 lines
arch/arm64/include/asm/tlbflush.h +5
-0+{ + pteval_t diff = oldval ^ newval; + + /* invalid to valid transition requires no flush */ + if (!(oldval & PTE_VALID) || (oldval & PTE_PRESENT_INVALID))Is the
PTE_PRESENT_INVALIDreally required? If the oldval was invalid, there can't be a TLB entry for it, so no flush is required; that's it, I think?Quoted 3 lines
In fact,
PTE_PRESENT_INVALIDis overlaid withPTE_NG; it only meansPTE_PRESENT_INVALIDwhen PTE_INVALID=0, so I think this is broken as is. Valid user-space PTEs always havePTE_NGset, so you will never flush.
Quoted 7 lines
Not sure I get you. The condition as I wrote means
1. If
PTE_VALIDis not set, then do not flush.
2. IfPTE_VALIDis set, *and*PTE_PRESENT_INVALIDis set, then do not flush.So when you say "it only means
PTE_PRESENT_INVALIDwhen PTE_INVALID=0", the second condition meets that.
Sorry I meant PTE_VALID=0. Your second condition is wrong; PTE_PRESENT_INVALID is only defined when PTE_VALID=0.
Think about it; the PTE is valid from the HW's perspective if and only if PTE_VALID=1. So that's the only condition that needs to be checked.
See this comment in the code for more info:
/*
* PTE_PRESENT_INVALID=1 & PTE_VALID=0 indicates that the pte's fields should be
* interpreted according to the HW layout by SW but any attempted HW access to
* the address will result in a fault. pte_present() returns true. */
#define PTE_PRESENT_INVALID (PTE_NG) /* only when !PTE_VALID */
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
+ return false;
+
+ /* Transition in the SW bits and access flag requires no flush */ + diff &= ~(PTE_SWBITS_MASK |PTE_AF);
Quoted 7 lines
Quoted 4 lines
Could you explain your thinking on why
PTE_AFchanges don't need a flush? I would have thought if we want to clear the access flag, that would definitely require a flush? Otherwise how would the MMU know to set the acccess bit on next access if it already has a TLB entry?You are correct, but AFAIK losing access bit information is not fatal, it will only mess with page aging. So potentially reclaim will lose some accuracy.
Sure, but it means that your change has a cost; reduced page aging accuracy. That part of the change should at least be separated into its own commit and probably backed up with performance numbers. Otherwise, I think we should retain the original behaviour.
Quoted 31 lines
Quoted 7 lines
Quoted 6 lines
+
+ if (!diff)
+ return false;
+ return true;Perhaps just "return !!diff;" here?
Sure.
Quoted 21 lines
Thanks,
RyanQuoted 16 lines
arch/arm64/include/asm/tlbflush.h +14
-0+} + +static inline bool pte_needs_flush (pte_t oldpte,pte_t newpte) +{ + return __pte_flags_need_flush(pte_val(oldpte), pte_val(newpte)); +} +#define pte_needs_flush pte_needs_flush + +static inline bool huge_pmd_needs_flush (pmd_t oldpmd,pmd_t newpmd) +{ + return __pte_flags_need_flush(pmd_val(oldpmd), pmd_val(newpmd)); +} +#define huge_pmd_needs_flush huge_pmd_needs_flush +#endif
#endif
Quoted 63 lines · Ryan Roberts
Quoted 48 lines · Dev Jain
Quoted 21 lines · Ryan Roberts
Quoted 20 lines · Dev Jain
Currently arm64 does an unconditional TLB flush in
mprotect(). This is not required for some cases, for example, when changing fromPROT_NONEtoPROT_READ|PROT_WRITE(a real usecase - glibc malloc does this to emulate growing into the non-main heaps), and unsetting uffd-wp in a range.Therefore, implement
pte_needs_flush()for arm64, which is already implemented by some other arches as well.Running a userspace program changing permissions back and forth between
PROT_NONEandPROT_READ|PROT_WRITE, and measuring the average time taken for thenone->rwtransition, I get a reduction from 3.2 microseconds to 2.95 microseconds, giving an 8.5% improvement.Signed-off-by Dev Jain <[email protected]>---
mm-selftests pass. Based on 6.17-rc6.arch/arm64/include/asm/tlbflush.h | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+)Quoted 13 lines
arch/arm64/include/asm/tlbflush.h static inline void arch_tlbbatch_add_pending(struct arch_tlbflush_unmap_batch *b · line 528
Quoted 12 lines
arch/arm64/include/asm/ renamed from arch/arm64/include/asm/tlbflush.h +0
-0
tlbflush.h
index18a5dc0c9a54..4a566d589100100644arch/arm64/include/asm/tlbflush.h +0
-0@@ -524,6 +524,35 @@ static inline void arch_tlbbatch_add_pending(struct
arch_tlbflush_unmap_batch*b
{
__flush_tlb_range_nosync(mm, start, end, PAGE_SIZE, true, 3); }arch/arm64/include/asm/tlbflush.h +2
-0+ +static inline bool __pte_flags_need_flush (pteval_t oldval,pteval_t newval)
ptdesc_tis the preferred any-level type.I keep forgetting this :)
Quoted 11 lines
arch/arm64/include/asm/tlbflush.h static inline void arch_tlbbatch_add_pending(struct arch_tlbflush_unmap_batch *b · line 533
Quoted 5 lines
arch/arm64/include/asm/tlbflush.h +5
-0+{ + pteval_t diff = oldval ^ newval; + + /* invalid to valid transition requires no flush */ + if (!(oldval & PTE_VALID) || (oldval & PTE_PRESENT_INVALID))Is the
PTE_PRESENT_INVALIDreally required? If the oldval was invalid, there can't be a TLB entry for it, so no flush is required; that's it, I think?In fact,
PTE_PRESENT_INVALIDis overlaid withPTE_NG; it only meansPTE_PRESENT_INVALIDwhen PTE_INVALID=0, so I think this is broken as is. Valid user-space PTEs always havePTE_NGset, so you will never flush.Quoted 7 lines
Not sure I get you. The condition as I wrote means
1. If
PTE_VALIDis not set, then do not flush.
2. IfPTE_VALIDis set, *and*PTE_PRESENT_INVALIDis set, then do not flush.So when you say "it only means
PTE_PRESENT_INVALIDwhen PTE_INVALID=0", the second condition meets that.Sorry I meant PTE_VALID=0. Your second condition is wrong;
PTE_PRESENT_INVALIDis only defined when PTE_VALID=0.Think about it; the PTE is valid from the HW's perspective if and only if PTE_VALID=1. So that's the only condition that needs to be checked.
Quoted 8 lines
See this comment in the code for more info:
/*
* PTE_PRESENT_INVALID=1 & PTE_VALID=0 indicates that the pte's fields should be
* interpreted according to the HW layout by SW but any attempted HW access to
* the address will result in a fault.pte_present()returns true. */
#definePTE_PRESENT_INVALID(PTE_NG) /* only when !PTE_VALID */
Got it, thanks.
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 5 lines
+ return false;
+
+ /* Transition in the SW bits and access flag requires no flush */ + diff &= ~(PTE_SWBITS_MASK |PTE_AF);Could you explain your thinking on why
PTE_AFchanges don't need a flush? I would have thought if we want to clear the access flag, that would definitely require a flush? Otherwise how would the MMU know to set the acccess bit on next access if it already has a TLB entry?
Quoted 6 lines
Quoted 2 lines
You are correct, but AFAIK losing access bit information is not fatal, it will only mess with page aging. So potentially reclaim will lose some accuracy.
Sure, but it means that your change has a cost; reduced page aging accuracy. That part of the change should at least be separated into its own commit and probably backed up with performance numbers. Otherwise, I think we should retain the original behaviour.
You are right, I checked other callsites clearing the access bit and they do some sort of TLB maintenance. I'll drop this.
Quoted 29 lines
Quoted 29 lines
Quoted 7 lines
Quoted 6 lines
+
+ if (!diff)
+ return false;
+ return true;Perhaps just "return !!diff;" here?
Sure.
Quoted 20 lines
Thanks,
RyanQuoted 16 lines
arch/arm64/include/asm/tlbflush.h +14
-0+} + +static inline bool pte_needs_flush (pte_t oldpte,pte_t newpte) +{ + return __pte_flags_need_flush(pte_val(oldpte), pte_val(newpte)); +} +#define pte_needs_flush pte_needs_flush + +static inline bool huge_pmd_needs_flush (pmd_t oldpmd,pmd_t newpmd) +{ + return __pte_flags_need_flush(pmd_val(oldpmd), pmd_val(newpmd)); +} +#define huge_pmd_needs_flush huge_pmd_needs_flush + #endif #endif