| Message ID | 20250325093625.55184-4-miko.lenczewski@arm.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Initial BBML2 support for contpte_convert() | expand |
On 25.03.25 10:36, Mikołaj Lenczewski wrote: > When converting a region via contpte_convert() to use mTHP, we have two > different goals. We have to mark each entry as contiguous, and we would > like to smear the dirty and young (access) bits across all entries in > the contiguous block. Currently, we do this by first accumulating the > dirty and young bits in the block, using an atomic > __ptep_get_and_clear() and the relevant pte_{dirty,young}() calls, > performing a tlbi, and finally smearing the correct bits across the > block using __set_ptes(). > > This approach works fine for BBM level 0, but with support for BBM level > 2 we are allowed to reorder the tlbi to after setting the pagetable > entries. We expect the time cost of a tlbi to be much greater than the > cost of clearing and resetting the PTEs. As such, this reordering of the > tlbi outside the window where our PTEs are invalid greatly reduces the > duration the PTE are visibly invalid for other threads. This reduces the > likelyhood of a concurrent page walk finding an invalid PTE, reducing > the likelyhood of a fault in other threads, and improving performance > (more so when there are more threads). > > Because we support via allowlist only bbml2 implementations that never > raise conflict aborts and instead invalidate the tlb entries > automatically in hardware, we can avoid the final flush altogether. > Avoiding flushes is a win. > > Signed-off-by: Mikołaj Lenczewski <miko.lenczewski@arm.com> > Reviewed-by: Ryan Roberts <ryan.roberts@arm.com> > --- > arch/arm64/mm/contpte.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/arch/arm64/mm/contpte.c b/arch/arm64/mm/contpte.c > index 55107d27d3f8..77ed03b30b72 100644 > --- a/arch/arm64/mm/contpte.c > +++ b/arch/arm64/mm/contpte.c > @@ -68,7 +68,8 @@ static void contpte_convert(struct mm_struct *mm, unsigned long addr, > pte = pte_mkyoung(pte); > } > > - __flush_tlb_range(&vma, start_addr, addr, PAGE_SIZE, true, 3); > + if (!system_supports_bbml2_noabort()) > + __flush_tlb_range(&vma, start_addr, addr, PAGE_SIZE, true, 3); > > __set_ptes(mm, start_addr, start_ptep, pte, CONT_PTES); > } Reviewed-by: David Hildenbrand <david@redhat.com>
On 25/03/2025 05:36, Mikołaj Lenczewski wrote: > When converting a region via contpte_convert() to use mTHP, we have two > different goals. We have to mark each entry as contiguous, and we would > like to smear the dirty and young (access) bits across all entries in > the contiguous block. Currently, we do this by first accumulating the > dirty and young bits in the block, using an atomic > __ptep_get_and_clear() and the relevant pte_{dirty,young}() calls, > performing a tlbi, and finally smearing the correct bits across the > block using __set_ptes(). > > This approach works fine for BBM level 0, but with support for BBM level > 2 we are allowed to reorder the tlbi to after setting the pagetable > entries. We expect the time cost of a tlbi to be much greater than the > cost of clearing and resetting the PTEs. As such, this reordering of the > tlbi outside the window where our PTEs are invalid greatly reduces the > duration the PTE are visibly invalid for other threads. This reduces the > likelyhood of a concurrent page walk finding an invalid PTE, reducing > the likelyhood of a fault in other threads, and improving performance > (more so when there are more threads). > > Because we support via allowlist only bbml2 implementations that never > raise conflict aborts and instead invalidate the tlb entries > automatically in hardware, we can avoid the final flush altogether. > Avoiding flushes is a win. > > Signed-off-by: Mikołaj Lenczewski <miko.lenczewski@arm.com> > Reviewed-by: Ryan Roberts <ryan.roberts@arm.com> > --- > arch/arm64/mm/contpte.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/arch/arm64/mm/contpte.c b/arch/arm64/mm/contpte.c > index 55107d27d3f8..77ed03b30b72 100644 > --- a/arch/arm64/mm/contpte.c > +++ b/arch/arm64/mm/contpte.c > @@ -68,7 +68,8 @@ static void contpte_convert(struct mm_struct *mm, unsigned long addr, > pte = pte_mkyoung(pte); > } > > - __flush_tlb_range(&vma, start_addr, addr, PAGE_SIZE, true, 3); > + if (!system_supports_bbml2_noabort()) > + __flush_tlb_range(&vma, start_addr, addr, PAGE_SIZE, true, 3); > > __set_ptes(mm, start_addr, start_ptep, pte, CONT_PTES); Despite all the conversation we had about completely eliding the TLBI for the BBML2 case, I've continued to be a bit uneasy about it. I had another chat with Alex C and we concluded that it is safe, but there could be conceivable implementations where it is not performant. Alex suggested doing a TLBI without the DSB and I think that's a good idea. So after the __set_ptes(), I suggest adding: if (system_supports_bbml2_noabort()) __flush_tlb_range_nosync(mm, start_addr, addr, PAGE_SIZE, true, 3); That will issue the TLBI but won't wait for it to complete. So it should be very fast. We are guranteed correctness immediately. We are guranteed performance after the next DSB (worst-case; next context switch). Thanks, Ryan > }
On Thu, Apr 03, 2025 at 09:14:43AM +0100, Ryan Roberts wrote: > On 25/03/2025 05:36, Mikołaj Lenczewski wrote: > > diff --git a/arch/arm64/mm/contpte.c b/arch/arm64/mm/contpte.c > > index 55107d27d3f8..77ed03b30b72 100644 > > --- a/arch/arm64/mm/contpte.c > > +++ b/arch/arm64/mm/contpte.c > > @@ -68,7 +68,8 @@ static void contpte_convert(struct mm_struct *mm, unsigned long addr, > > pte = pte_mkyoung(pte); > > } > > > > - __flush_tlb_range(&vma, start_addr, addr, PAGE_SIZE, true, 3); > > + if (!system_supports_bbml2_noabort()) > > + __flush_tlb_range(&vma, start_addr, addr, PAGE_SIZE, true, 3); > > > > __set_ptes(mm, start_addr, start_ptep, pte, CONT_PTES); > > Despite all the conversation we had about completely eliding the TLBI for the > BBML2 case, I've continued to be a bit uneasy about it. I had another chat with > Alex C and we concluded that it is safe, but there could be conceivable > implementations where it is not performant. Alex suggested doing a TLBI without > the DSB and I think that's a good idea. So after the __set_ptes(), I suggest adding: > > if (system_supports_bbml2_noabort()) > __flush_tlb_range_nosync(mm, start_addr, addr, PAGE_SIZE, > true, 3); > > That will issue the TLBI but won't wait for it to complete. So it should be very > fast. We are guranteed correctness immediately. We are guranteed performance > after the next DSB (worst-case; next context switch). > > Thanks, > Ryan Hi Ryan, Sure, perfectly happy to add that on. Will respin and add a note about this behaviour to the source code and to the patch / cover letter.
diff --git a/arch/arm64/mm/contpte.c b/arch/arm64/mm/contpte.c index 55107d27d3f8..77ed03b30b72 100644 --- a/arch/arm64/mm/contpte.c +++ b/arch/arm64/mm/contpte.c @@ -68,7 +68,8 @@ static void contpte_convert(struct mm_struct *mm, unsigned long addr, pte = pte_mkyoung(pte); } - __flush_tlb_range(&vma, start_addr, addr, PAGE_SIZE, true, 3); + if (!system_supports_bbml2_noabort()) + __flush_tlb_range(&vma, start_addr, addr, PAGE_SIZE, true, 3); __set_ptes(mm, start_addr, start_ptep, pte, CONT_PTES); }
When converting a region via contpte_convert() to use mTHP, we have two different goals. We have to mark each entry as contiguous, and we would like to smear the dirty and young (access) bits across all entries in the contiguous block. Currently, we do this by first accumulating the dirty and young bits in the block, using an atomic __ptep_get_and_clear() and the relevant pte_{dirty,young}() calls, performing a tlbi, and finally smearing the correct bits across the block using __set_ptes(). This approach works fine for BBM level 0, but with support for BBM level 2 we are allowed to reorder the tlbi to after setting the pagetable entries. We expect the time cost of a tlbi to be much greater than the cost of clearing and resetting the PTEs. As such, this reordering of the tlbi outside the window where our PTEs are invalid greatly reduces the duration the PTE are visibly invalid for other threads. This reduces the likelyhood of a concurrent page walk finding an invalid PTE, reducing the likelyhood of a fault in other threads, and improving performance (more so when there are more threads). Because we support via allowlist only bbml2 implementations that never raise conflict aborts and instead invalidate the tlb entries automatically in hardware, we can avoid the final flush altogether. Avoiding flushes is a win. Signed-off-by: Mikołaj Lenczewski <miko.lenczewski@arm.com> Reviewed-by: Ryan Roberts <ryan.roberts@arm.com> --- arch/arm64/mm/contpte.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)