| Message ID | 1591177333-17833-1-git-send-email-maobibo@loongson.cn (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | MIPS: Do not flush tlb when setting pmd entry | expand |
On Wed, Jun 03, 2020 at 05:42:13PM +0800, Bibo Mao wrote: > Function set_pmd_at is to set pmd entry, if tlb entry need to > be flushed, there exists pmdp_huge_clear_flush alike function > before set_pmd_at is called. So it is not necessary to call > flush_tlb_all in this function. have you checked all set_pmd_at() calls ? I found a few case where it's not clear to me, if tlb flushing is done... If you think this is still the right thing to do, please change arch/mips/mm/pgtable-32.c as well. Thomas.
On 06/15/2020 06:14 PM, Thomas Bogendoerfer wrote: > On Wed, Jun 03, 2020 at 05:42:13PM +0800, Bibo Mao wrote: >> Function set_pmd_at is to set pmd entry, if tlb entry need to >> be flushed, there exists pmdp_huge_clear_flush alike function >> before set_pmd_at is called. So it is not necessary to call >> flush_tlb_all in this function. > > have you checked all set_pmd_at() calls ? I found a few case where > it's not clear to me, if tlb flushing is done... If you think this > is still the right thing to do, please change arch/mips/mm/pgtable-32.c > as well. well, I will double check this and do more testing about thp and hugepage. regards bibo,mao > > Thomas. >
On Tue, Jun 16, 2020 at 06:34:21PM +0800, maobibo wrote: > > > On 06/15/2020 06:14 PM, Thomas Bogendoerfer wrote: > > On Wed, Jun 03, 2020 at 05:42:13PM +0800, Bibo Mao wrote: > >> Function set_pmd_at is to set pmd entry, if tlb entry need to > >> be flushed, there exists pmdp_huge_clear_flush alike function > >> before set_pmd_at is called. So it is not necessary to call > >> flush_tlb_all in this function. > > > > have you checked all set_pmd_at() calls ? I found a few case where > > it's not clear to me, if tlb flushing is done... If you think this > > is still the right thing to do, please change arch/mips/mm/pgtable-32.c > > as well. > well, I will double check this and do more testing about thp and hugepage. I was more concerned about fs/dax.c fs/proc/task_mmu.c mm/rmap.c Thomas.
On 06/17/2020 07:14 PM, Thomas Bogendoerfer wrote: > On Tue, Jun 16, 2020 at 06:34:21PM +0800, maobibo wrote: >> >> >> On 06/15/2020 06:14 PM, Thomas Bogendoerfer wrote: >>> On Wed, Jun 03, 2020 at 05:42:13PM +0800, Bibo Mao wrote: >>>> Function set_pmd_at is to set pmd entry, if tlb entry need to >>>> be flushed, there exists pmdp_huge_clear_flush alike function >>>> before set_pmd_at is called. So it is not necessary to call >>>> flush_tlb_all in this function. >>> >>> have you checked all set_pmd_at() calls ? I found a few case where >>> it's not clear to me, if tlb flushing is done... If you think this >>> is still the right thing to do, please change arch/mips/mm/pgtable-32.c >>> as well. >> well, I will double check this and do more testing about thp and hugepage. > > I was more concerned about > > fs/dax.c > fs/proc/task_mmu.c > mm/rmap.c I think that flush_tlb_all should not be called in function set_pmd_at on mips platform. However update_mmu_cache_pmd() should be called __after__ set_pmd_at() function to update tlb entry at some places, it is another issue. Here is my analysis in the three files where set_pmd_at is called. in file fs/dax.c ------------------------------------------------------ if (pmdp) { #ifdef CONFIG_FS_DAX_PMD pmd_t pmd; if (pfn != pmd_pfn(*pmdp)) goto unlock_pmd; if (!pmd_dirty(*pmdp) && !pmd_write(*pmdp)) goto unlock_pmd; flush_cache_page(vma, address, pfn); pmd = pmdp_invalidate(vma, address, pmdp); pmd = pmd_wrprotect(pmd); pmd = pmd_mkclean(pmd); set_pmd_at(vma->vm_mm, address, pmdp, pmd); unlock_pmd: #endif ------------------------------------------------------ pmdp_invalidate is called here to flush pmd range already, it is not necessary to flush pmd range in function set_pmd_at ------------------------------------------------------ if (!pmd_none(*(vmf->pmd))) { spin_unlock(ptl); goto fallback; } if (pgtable) { pgtable_trans_huge_deposit(vma->vm_mm, vmf->pmd, pgtable); mm_inc_nr_ptes(vma->vm_mm); } pmd_entry = mk_pmd(zero_page, vmf->vma->vm_page_prot); pmd_entry = pmd_mkhuge(pmd_entry); set_pmd_at(vmf->vma->vm_mm, pmd_addr, vmf->pmd, pmd_entry); spin_unlock(ptl); trace_dax_pmd_load_hole(inode, vmf, zero_page, *entry); return VM_FAULT_NOPAGE; ------------------------------------------------------ pmd entry is none, does not need to flush pmd range in file fs/proc/task_mmu.c ------------------------------------------------------ static inline void clear_soft_dirty_pmd(struct vm_area_struct *vma, unsigned long addr, pmd_t *pmdp) { pmd_t old, pmd = *pmdp; if (pmd_present(pmd)) { /* See comment in change_huge_pmd() */ old = pmdp_invalidate(vma, addr, pmdp); if (pmd_dirty(old)) pmd = pmd_mkdirty(pmd); if (pmd_young(old)) pmd = pmd_mkyoung(pmd); pmd = pmd_wrprotect(pmd); pmd = pmd_clear_soft_dirty(pmd); set_pmd_at(vma->vm_mm, addr, pmdp, pmd); } else if (is_migration_entry(pmd_to_swp_entry(pmd))) { pmd = pmd_swp_clear_soft_dirty(pmd); set_pmd_at(vma->vm_mm, addr, pmdp, pmd); } } ------------------------------------------------------ At the first place where set_pmd_at is called, pmdp_invalidate is called to flush pmd range. At the second place, on mips system pmd_swp_clear_soft_dirty(pmd) is equal to pmd, pmd entry has no change, does not need to flush pmd range in file linux/mm/rmap.c: ------------------------------------------------------ pmd_t *pmd = pvmw.pmd; pmd_t entry; if (!pmd_dirty(*pmd) && !pmd_write(*pmd)) continue; flush_cache_page(vma, address, page_to_pfn(page)); entry = pmdp_invalidate(vma, address, pmd); entry = pmd_wrprotect(entry); entry = pmd_mkclean(entry); set_pmd_at(vma->vm_mm, address, pmd, entry); ret = 1; ------------------------------------------------------ pmdp_invalidate is called to flush pmd range, does not need to flush pmd range again in function set_pmd_at. > > Thomas. >
On Sat, Jun 20, 2020 at 11:47:35AM +0800, maobibo wrote: > > > On 06/17/2020 07:14 PM, Thomas Bogendoerfer wrote: > > On Tue, Jun 16, 2020 at 06:34:21PM +0800, maobibo wrote: > >> > >> > >> On 06/15/2020 06:14 PM, Thomas Bogendoerfer wrote: > >>> On Wed, Jun 03, 2020 at 05:42:13PM +0800, Bibo Mao wrote: > >>>> Function set_pmd_at is to set pmd entry, if tlb entry need to > >>>> be flushed, there exists pmdp_huge_clear_flush alike function > >>>> before set_pmd_at is called. So it is not necessary to call > >>>> flush_tlb_all in this function. > >>> > >>> have you checked all set_pmd_at() calls ? I found a few case where > >>> it's not clear to me, if tlb flushing is done... If you think this > >>> is still the right thing to do, please change arch/mips/mm/pgtable-32.c > >>> as well. > >> well, I will double check this and do more testing about thp and hugepage. > > > > I was more concerned about > > > > fs/dax.c > > fs/proc/task_mmu.c > > mm/rmap.c > > I think that flush_tlb_all should not be called in function set_pmd_at > on mips platform. However update_mmu_cache_pmd() should be called __after__ > set_pmd_at() function to update tlb entry at some places, it is another issue. > Here is my analysis in the three files where set_pmd_at is called. > [..] thank you for confirming that we are good with removing flush_tlb_all(). Thomas.
On 06/22/2020 11:48 PM, Thomas Bogendoerfer wrote: > On Sat, Jun 20, 2020 at 11:47:35AM +0800, maobibo wrote: >> >> >> On 06/17/2020 07:14 PM, Thomas Bogendoerfer wrote: >>> On Tue, Jun 16, 2020 at 06:34:21PM +0800, maobibo wrote: >>>> >>>> >>>> On 06/15/2020 06:14 PM, Thomas Bogendoerfer wrote: >>>>> On Wed, Jun 03, 2020 at 05:42:13PM +0800, Bibo Mao wrote: >>>>>> Function set_pmd_at is to set pmd entry, if tlb entry need to >>>>>> be flushed, there exists pmdp_huge_clear_flush alike function >>>>>> before set_pmd_at is called. So it is not necessary to call >>>>>> flush_tlb_all in this function. >>>>> >>>>> have you checked all set_pmd_at() calls ? I found a few case where >>>>> it's not clear to me, if tlb flushing is done... If you think this >>>>> is still the right thing to do, please change arch/mips/mm/pgtable-32.c >>>>> as well. >>>> well, I will double check this and do more testing about thp and hugepage. >>> >>> I was more concerned about >>> >>> fs/dax.c >>> fs/proc/task_mmu.c >>> mm/rmap.c >> >> I think that flush_tlb_all should not be called in function set_pmd_at >> on mips platform. However update_mmu_cache_pmd() should be called __after__ >> set_pmd_at() function to update tlb entry at some places, it is another issue. >> Here is my analysis in the three files where set_pmd_at is called. >> [..] > > thank you for confirming that we are good with removing flush_tlb_all(). Sorry, there is something wrong if remove flush_tlb_all(). If pmd_none is true, pmd points to invalid_pte_table, maybe there exists pte entry with normal page size for fault address. And we need invalidate this pte entry like it is done in function build_huge_handler_tail in file arch/mips/mm/tlbex.c I will send another patch. > > Thomas. >
diff --git a/arch/mips/mm/pgtable-64.c b/arch/mips/mm/pgtable-64.c index 6fd6e96..a236752 100644 --- a/arch/mips/mm/pgtable-64.c +++ b/arch/mips/mm/pgtable-64.c @@ -101,7 +101,6 @@ void set_pmd_at(struct mm_struct *mm, unsigned long addr, pmd_t *pmdp, pmd_t pmd) { *pmdp = pmd; - flush_tlb_all(); } void __init pagetable_init(void)
Function set_pmd_at is to set pmd entry, if tlb entry need to be flushed, there exists pmdp_huge_clear_flush alike function before set_pmd_at is called. So it is not necessary to call flush_tlb_all in this function. Signed-off-by: Bibo Mao <maobibo@loongson.cn> --- arch/mips/mm/pgtable-64.c | 1 - 1 file changed, 1 deletion(-)