Date:   Sat, 20 Jun 2020 11:47:35 +0800
From:   maobibo <maobibo@...ngson.cn>
To:     Thomas Bogendoerfer <tsbogend@...ha.franken.de>
Cc:     linux-mips@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] MIPS: Do not flush tlb when setting pmd entry



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

Powered by blists - more mailing lists