* [PATCH 0/2] powerpc/64s/radix: avoid unnecessary TLB flushes on fault @ 2018-05-09 6:51 Nicholas Piggin 2018-05-09 6:51 ` [PATCH 1/2] powerpc/64s/radix: do not flush TLB when relaxing access Nicholas Piggin 2018-05-09 6:51 ` [PATCH 2/2] powerpc/64s/radix: do not flush TLB on spurious fault Nicholas Piggin 0 siblings, 2 replies; 7+ messages in thread From: Nicholas Piggin @ 2018-05-09 6:51 UTC (permalink / raw) To: linuxppc-dev; +Cc: Nicholas Piggin, Alistair Popple These two patches make radix match hash and not flush the TLB after fixing up faults unnecessarily. There was some concern that accelerators need to have this flush, but nothing is documented or commented, so it should just be removed. We have a coprocessor count in the mm context now, and that can easily be special cased if neccesary. This and a few other changes reduce our broadcast tlbie rates by 10x on a kernel compile benchmark, so it would be good to get it in. Thanks, Nick Nicholas Piggin (2): powerpc/64s/radix: do not flush TLB when relaxing access powerpc/64s/radix: do not flush TLB on spurious fault arch/powerpc/include/asm/book3s/64/tlbflush.h | 7 +++++++ arch/powerpc/mm/pgtable-book3s64.c | 1 - arch/powerpc/mm/pgtable.c | 3 ++- 3 files changed, 9 insertions(+), 2 deletions(-) -- 2.17.0 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 1/2] powerpc/64s/radix: do not flush TLB when relaxing access 2018-05-09 6:51 [PATCH 0/2] powerpc/64s/radix: avoid unnecessary TLB flushes on fault Nicholas Piggin @ 2018-05-09 6:51 ` Nicholas Piggin 2018-05-09 7:07 ` Balbir Singh 2018-05-09 6:51 ` [PATCH 2/2] powerpc/64s/radix: do not flush TLB on spurious fault Nicholas Piggin 1 sibling, 1 reply; 7+ messages in thread From: Nicholas Piggin @ 2018-05-09 6:51 UTC (permalink / raw) To: linuxppc-dev; +Cc: Nicholas Piggin, Alistair Popple Radix flushes the TLB when updating ptes to increase permissiveness of protection (increase access authority). Book3S does not require TLB flushing in this case, and it is not done on hash. This patch avoids the flush for radix. >From Power ISA v3.0B, p.1090: Setting a Reference or Change Bit or Upgrading Access Authority (PTE Subject to Atomic Hardware Updates) If the only change being made to a valid PTE that is subject to atomic hardware updates is to set the Reference or Change bit to 1 or to add access authorities, a simpler sequence suffices because the translation hardware will refetch the PTE if an access is attempted for which the only problems were reference and/or change bits needing to be set or insufficient access authority. Signed-off-by: Nicholas Piggin <npiggin@gmail.com> --- arch/powerpc/mm/pgtable-book3s64.c | 1 - arch/powerpc/mm/pgtable.c | 3 ++- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/arch/powerpc/mm/pgtable-book3s64.c b/arch/powerpc/mm/pgtable-book3s64.c index 518518fb7c45..6e991eaccab4 100644 --- a/arch/powerpc/mm/pgtable-book3s64.c +++ b/arch/powerpc/mm/pgtable-book3s64.c @@ -40,7 +40,6 @@ int pmdp_set_access_flags(struct vm_area_struct *vma, unsigned long address, if (changed) { __ptep_set_access_flags(vma->vm_mm, pmdp_ptep(pmdp), pmd_pte(entry), address); - flush_pmd_tlb_range(vma, address, address + HPAGE_PMD_SIZE); } return changed; } diff --git a/arch/powerpc/mm/pgtable.c b/arch/powerpc/mm/pgtable.c index 9f361ae571e9..5b07a626df5b 100644 --- a/arch/powerpc/mm/pgtable.c +++ b/arch/powerpc/mm/pgtable.c @@ -224,7 +224,8 @@ int ptep_set_access_flags(struct vm_area_struct *vma, unsigned long address, if (!is_vm_hugetlb_page(vma)) assert_pte_locked(vma->vm_mm, address); __ptep_set_access_flags(vma->vm_mm, ptep, entry, address); - flush_tlb_page(vma, address); + if (!IS_ENABLED(CONFIG_PPC_BOOK3S_64)) + flush_tlb_page(vma, address); } return changed; } -- 2.17.0 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] powerpc/64s/radix: do not flush TLB when relaxing access 2018-05-09 6:51 ` [PATCH 1/2] powerpc/64s/radix: do not flush TLB when relaxing access Nicholas Piggin @ 2018-05-09 7:07 ` Balbir Singh 2018-05-09 7:43 ` Nicholas Piggin 0 siblings, 1 reply; 7+ messages in thread From: Balbir Singh @ 2018-05-09 7:07 UTC (permalink / raw) To: Nicholas Piggin Cc: open list:LINUX FOR POWERPC (32-BIT AND 64-BIT), Alistair Popple On Wed, May 9, 2018 at 4:51 PM, Nicholas Piggin <npiggin@gmail.com> wrote: > Radix flushes the TLB when updating ptes to increase permissiveness > of protection (increase access authority). Book3S does not require > TLB flushing in this case, and it is not done on hash. This patch > avoids the flush for radix. > > From Power ISA v3.0B, p.1090: > > Setting a Reference or Change Bit or Upgrading Access Authority > (PTE Subject to Atomic Hardware Updates) > > If the only change being made to a valid PTE that is subject to > atomic hardware updates is to set the Reference or Change bit to 1 > or to add access authorities, a simpler sequence suffices because > the translation hardware will refetch the PTE if an access is > attempted for which the only problems were reference and/or change > bits needing to be set or insufficient access authority. > > Signed-off-by: Nicholas Piggin <npiggin@gmail.com> > --- > arch/powerpc/mm/pgtable-book3s64.c | 1 - > arch/powerpc/mm/pgtable.c | 3 ++- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/arch/powerpc/mm/pgtable-book3s64.c b/arch/powerpc/mm/pgtable-book3s64.c > index 518518fb7c45..6e991eaccab4 100644 > --- a/arch/powerpc/mm/pgtable-book3s64.c > +++ b/arch/powerpc/mm/pgtable-book3s64.c > @@ -40,7 +40,6 @@ int pmdp_set_access_flags(struct vm_area_struct *vma, unsigned long address, > if (changed) { > __ptep_set_access_flags(vma->vm_mm, pmdp_ptep(pmdp), > pmd_pte(entry), address); > - flush_pmd_tlb_range(vma, address, address + HPAGE_PMD_SIZE); The comment states that this can be used for missing execution permissions as well. I am not convinced we can skip a flush in those cases > } > return changed; > } > diff --git a/arch/powerpc/mm/pgtable.c b/arch/powerpc/mm/pgtable.c > index 9f361ae571e9..5b07a626df5b 100644 > --- a/arch/powerpc/mm/pgtable.c > +++ b/arch/powerpc/mm/pgtable.c > @@ -224,7 +224,8 @@ int ptep_set_access_flags(struct vm_area_struct *vma, unsigned long address, > if (!is_vm_hugetlb_page(vma)) > assert_pte_locked(vma->vm_mm, address); > __ptep_set_access_flags(vma->vm_mm, ptep, entry, address); > - flush_tlb_page(vma, address); > + if (!IS_ENABLED(CONFIG_PPC_BOOK3S_64)) > + flush_tlb_page(vma, address); Same as above Balbir Singh. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] powerpc/64s/radix: do not flush TLB when relaxing access 2018-05-09 7:07 ` Balbir Singh @ 2018-05-09 7:43 ` Nicholas Piggin 2018-05-09 8:27 ` Balbir Singh 0 siblings, 1 reply; 7+ messages in thread From: Nicholas Piggin @ 2018-05-09 7:43 UTC (permalink / raw) To: Balbir Singh Cc: open list:LINUX FOR POWERPC (32-BIT AND 64-BIT), Alistair Popple On Wed, 9 May 2018 17:07:47 +1000 Balbir Singh <bsingharora@gmail.com> wrote: > On Wed, May 9, 2018 at 4:51 PM, Nicholas Piggin <npiggin@gmail.com> wrote: > > Radix flushes the TLB when updating ptes to increase permissiveness > > of protection (increase access authority). Book3S does not require > > TLB flushing in this case, and it is not done on hash. This patch > > avoids the flush for radix. > > > > From Power ISA v3.0B, p.1090: > > > > Setting a Reference or Change Bit or Upgrading Access Authority > > (PTE Subject to Atomic Hardware Updates) > > > > If the only change being made to a valid PTE that is subject to > > atomic hardware updates is to set the Reference or Change bit to 1 > > or to add access authorities, a simpler sequence suffices because > > the translation hardware will refetch the PTE if an access is > > attempted for which the only problems were reference and/or change > > bits needing to be set or insufficient access authority. > > > > Signed-off-by: Nicholas Piggin <npiggin@gmail.com> > > --- > > arch/powerpc/mm/pgtable-book3s64.c | 1 - > > arch/powerpc/mm/pgtable.c | 3 ++- > > 2 files changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/arch/powerpc/mm/pgtable-book3s64.c b/arch/powerpc/mm/pgtable-book3s64.c > > index 518518fb7c45..6e991eaccab4 100644 > > --- a/arch/powerpc/mm/pgtable-book3s64.c > > +++ b/arch/powerpc/mm/pgtable-book3s64.c > > @@ -40,7 +40,6 @@ int pmdp_set_access_flags(struct vm_area_struct *vma, unsigned long address, > > if (changed) { > > __ptep_set_access_flags(vma->vm_mm, pmdp_ptep(pmdp), > > pmd_pte(entry), address); > > - flush_pmd_tlb_range(vma, address, address + HPAGE_PMD_SIZE); > > The comment states that this can be used for missing execution > permissions as well. I am not convinced we can skip a flush in those > cases Why not? Execute is part of the access authority. And they're already no ops on hash. What am I missing? Thanks, Nick ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] powerpc/64s/radix: do not flush TLB when relaxing access 2018-05-09 7:43 ` Nicholas Piggin @ 2018-05-09 8:27 ` Balbir Singh 2018-05-09 11:39 ` Nicholas Piggin 0 siblings, 1 reply; 7+ messages in thread From: Balbir Singh @ 2018-05-09 8:27 UTC (permalink / raw) To: Nicholas Piggin Cc: open list:LINUX FOR POWERPC (32-BIT AND 64-BIT), Alistair Popple On Wed, May 9, 2018 at 5:43 PM, Nicholas Piggin <npiggin@gmail.com> wrote: > On Wed, 9 May 2018 17:07:47 +1000 > Balbir Singh <bsingharora@gmail.com> wrote: > >> On Wed, May 9, 2018 at 4:51 PM, Nicholas Piggin <npiggin@gmail.com> wrote: >> > Radix flushes the TLB when updating ptes to increase permissiveness >> > of protection (increase access authority). Book3S does not require >> > TLB flushing in this case, and it is not done on hash. This patch >> > avoids the flush for radix. >> > >> > From Power ISA v3.0B, p.1090: >> > >> > Setting a Reference or Change Bit or Upgrading Access Authority >> > (PTE Subject to Atomic Hardware Updates) >> > >> > If the only change being made to a valid PTE that is subject to >> > atomic hardware updates is to set the Reference or Change bit to 1 >> > or to add access authorities, a simpler sequence suffices because >> > the translation hardware will refetch the PTE if an access is >> > attempted for which the only problems were reference and/or change >> > bits needing to be set or insufficient access authority. >> > >> > Signed-off-by: Nicholas Piggin <npiggin@gmail.com> >> > --- >> > arch/powerpc/mm/pgtable-book3s64.c | 1 - >> > arch/powerpc/mm/pgtable.c | 3 ++- >> > 2 files changed, 2 insertions(+), 2 deletions(-) >> > >> > diff --git a/arch/powerpc/mm/pgtable-book3s64.c b/arch/powerpc/mm/pgtable-book3s64.c >> > index 518518fb7c45..6e991eaccab4 100644 >> > --- a/arch/powerpc/mm/pgtable-book3s64.c >> > +++ b/arch/powerpc/mm/pgtable-book3s64.c >> > @@ -40,7 +40,6 @@ int pmdp_set_access_flags(struct vm_area_struct *vma, unsigned long address, >> > if (changed) { >> > __ptep_set_access_flags(vma->vm_mm, pmdp_ptep(pmdp), >> > pmd_pte(entry), address); >> > - flush_pmd_tlb_range(vma, address, address + HPAGE_PMD_SIZE); >> >> The comment states that this can be used for missing execution >> permissions as well. I am not convinced we can skip a flush in those >> cases > > Why not? Execute is part of the access authority. And they're already no > ops on hash. What am I missing? I have not reviewed the hash code, but if relaxing access means allowing the code to provide execute permission, won't this result in spurious faults? A simple test might be to run a JIT workload and see if the number of faults go up with and without the patch? Balbir ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] powerpc/64s/radix: do not flush TLB when relaxing access 2018-05-09 8:27 ` Balbir Singh @ 2018-05-09 11:39 ` Nicholas Piggin 0 siblings, 0 replies; 7+ messages in thread From: Nicholas Piggin @ 2018-05-09 11:39 UTC (permalink / raw) To: Balbir Singh Cc: open list:LINUX FOR POWERPC (32-BIT AND 64-BIT), Alistair Popple On Wed, 9 May 2018 18:27:07 +1000 Balbir Singh <bsingharora@gmail.com> wrote: > On Wed, May 9, 2018 at 5:43 PM, Nicholas Piggin <npiggin@gmail.com> wrote: > > On Wed, 9 May 2018 17:07:47 +1000 > > Balbir Singh <bsingharora@gmail.com> wrote: > > > >> On Wed, May 9, 2018 at 4:51 PM, Nicholas Piggin <npiggin@gmail.com> wrote: > >> > Radix flushes the TLB when updating ptes to increase permissiveness > >> > of protection (increase access authority). Book3S does not require > >> > TLB flushing in this case, and it is not done on hash. This patch > >> > avoids the flush for radix. > >> > > >> > From Power ISA v3.0B, p.1090: > >> > > >> > Setting a Reference or Change Bit or Upgrading Access Authority > >> > (PTE Subject to Atomic Hardware Updates) > >> > > >> > If the only change being made to a valid PTE that is subject to > >> > atomic hardware updates is to set the Reference or Change bit to 1 > >> > or to add access authorities, a simpler sequence suffices because > >> > the translation hardware will refetch the PTE if an access is > >> > attempted for which the only problems were reference and/or change > >> > bits needing to be set or insufficient access authority. > >> > > >> > Signed-off-by: Nicholas Piggin <npiggin@gmail.com> > >> > --- > >> > arch/powerpc/mm/pgtable-book3s64.c | 1 - > >> > arch/powerpc/mm/pgtable.c | 3 ++- > >> > 2 files changed, 2 insertions(+), 2 deletions(-) > >> > > >> > diff --git a/arch/powerpc/mm/pgtable-book3s64.c b/arch/powerpc/mm/pgtable-book3s64.c > >> > index 518518fb7c45..6e991eaccab4 100644 > >> > --- a/arch/powerpc/mm/pgtable-book3s64.c > >> > +++ b/arch/powerpc/mm/pgtable-book3s64.c > >> > @@ -40,7 +40,6 @@ int pmdp_set_access_flags(struct vm_area_struct *vma, unsigned long address, > >> > if (changed) { > >> > __ptep_set_access_flags(vma->vm_mm, pmdp_ptep(pmdp), > >> > pmd_pte(entry), address); > >> > - flush_pmd_tlb_range(vma, address, address + HPAGE_PMD_SIZE); > >> > >> The comment states that this can be used for missing execution > >> permissions as well. I am not convinced we can skip a flush in those > >> cases > > > > Why not? Execute is part of the access authority. And they're already no > > ops on hash. What am I missing? > > I have not reviewed the hash code, but if relaxing access means > allowing the code to provide execute permission, Yes, adding RWX or RC bits would qualify at least. > won't this result in > spurious faults? Well it gets called as part of page faults, which means the MMU will already reload the pte when the access is retried, as per the ISA. A few paths actually don't call it from page fault paths, but the cost of a superfluous fault occasionally versus always doing a tlbie makes this the wrong thing to do. x86 and hash are the same, they don't flush here. Hash mode should behave basically the same way in terms of what it stores in the TLB and whether it would take more faults. Radix is the newcomer so it should match hash without some reason not to. > A simple test might be to run a JIT workload and see > if the number of faults go up with and without the patch? What pattern of accesses are you worried about? Why is execute different from write, for example? Thanks, Nick ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 2/2] powerpc/64s/radix: do not flush TLB on spurious fault 2018-05-09 6:51 [PATCH 0/2] powerpc/64s/radix: avoid unnecessary TLB flushes on fault Nicholas Piggin 2018-05-09 6:51 ` [PATCH 1/2] powerpc/64s/radix: do not flush TLB when relaxing access Nicholas Piggin @ 2018-05-09 6:51 ` Nicholas Piggin 1 sibling, 0 replies; 7+ messages in thread From: Nicholas Piggin @ 2018-05-09 6:51 UTC (permalink / raw) To: linuxppc-dev; +Cc: Nicholas Piggin, Alistair Popple In the case of a spurious fault (which can happen due to a race with another thread that changes the page table), the default Linux mm code calls flush_tlb_page for that address. This is not required because the pte will be re-fetched. Hash does not wire this up to a hardware TLB flush for this reason. This patch avoids the flush for radix. >From Power ISA v3.0B, p.1090: Setting a Reference or Change Bit or Upgrading Access Authority (PTE Subject to Atomic Hardware Updates) If the only change being made to a valid PTE that is subject to atomic hardware updates is to set the Refer- ence or Change bit to 1 or to add access authorities, a simpler sequence suffices because the translation hardware will refetch the PTE if an access is attempted for which the only problems were reference and/or change bits needing to be set or insufficient access authority. Signed-off-by: Nicholas Piggin <npiggin@gmail.com> --- arch/powerpc/include/asm/book3s/64/tlbflush.h | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/arch/powerpc/include/asm/book3s/64/tlbflush.h b/arch/powerpc/include/asm/book3s/64/tlbflush.h index 0cac17253513..9c43431e01c5 100644 --- a/arch/powerpc/include/asm/book3s/64/tlbflush.h +++ b/arch/powerpc/include/asm/book3s/64/tlbflush.h @@ -137,6 +137,13 @@ static inline void flush_all_mm(struct mm_struct *mm) #define flush_tlb_page(vma, addr) local_flush_tlb_page(vma, addr) #define flush_all_mm(mm) local_flush_all_mm(mm) #endif /* CONFIG_SMP */ + +#define flush_tlb_fix_spurious_fault flush_tlb_fix_spurious_fault +static inline void flush_tlb_fix_spurious_fault(struct vm_area_struct *vma, + unsigned long address) +{ +} + /* * flush the page walk cache for the address */ -- 2.17.0 ^ permalink raw reply related [flat|nested] 7+ messages in thread
end of thread, other threads:[~2018-05-09 11:39 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-05-09 6:51 [PATCH 0/2] powerpc/64s/radix: avoid unnecessary TLB flushes on fault Nicholas Piggin 2018-05-09 6:51 ` [PATCH 1/2] powerpc/64s/radix: do not flush TLB when relaxing access Nicholas Piggin 2018-05-09 7:07 ` Balbir Singh 2018-05-09 7:43 ` Nicholas Piggin 2018-05-09 8:27 ` Balbir Singh 2018-05-09 11:39 ` Nicholas Piggin 2018-05-09 6:51 ` [PATCH 2/2] powerpc/64s/radix: do not flush TLB on spurious fault Nicholas Piggin
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.