[PATCH] MIPS: Do not flush tlb when setting pmd entry

[PATCH] MIPS: Do not flush tlb when setting pmd entry

[Bibo Mao]

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(-)

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)
-- 
1.8.3.1

Re: [PATCH] MIPS: Do not flush tlb when setting pmd entry

[Thomas Bogendoerfer]

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.

-- 
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea.                                                [ RFC1925, 2.3 ]

Re: [PATCH] MIPS: Do not flush tlb when setting pmd entry

[maobibo]

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.
> 

Re: [PATCH] MIPS: Do not flush tlb when setting pmd entry

[Thomas Bogendoerfer]

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.

-- 
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea.                                                [ RFC1925, 2.3 ]

Re: [PATCH] MIPS: Do not flush tlb when setting pmd entry

[maobibo]

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.
> 

Re: [PATCH] MIPS: Do not flush tlb when setting pmd entry

[Thomas Bogendoerfer]

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.

-- 
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea.                                                [ RFC1925, 2.3 ]

Re: [PATCH] MIPS: Do not flush tlb when setting pmd entry

[maobibo]

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.
>