Re: [PATCH 4/4] MIPS: Sync icache & dcache in set_pte_at

[Paul Burton <paul.burton@imgtec.com>]

On Fri, Mar 04, 2016 at 09:43:54AM -0800, David Daney wrote:
> On 03/01/2016 09:19 AM, Paul Burton wrote:
> >On Tue, Mar 01, 2016 at 09:13:23AM -0800, David Daney wrote:
> >>On 02/29/2016 06:37 PM, Paul Burton wrote:
> >>[...]
> >>>@@ -234,6 +237,22 @@ static inline void pte_clear(struct mm_struct *mm, unsigned long addr, pte_t *pt
> >>>  }
> >>>  #endif
> >>>
> >>>+static inline void set_pte_at(struct mm_struct *mm, unsigned long addr,
> >>>+			      pte_t *ptep, pte_t pteval)
> >>>+{
> >>>+	extern void __update_cache(unsigned long address, pte_t pte);
> >>>+
> >>>+	if (!pte_present(pteval))
> >>>+		goto cache_sync_done;
> >>>+
> >>>+	if (pte_present(*ptep) && (pte_pfn(*ptep) == pte_pfn(pteval)))
> >>>+		goto cache_sync_done;
> >>>+
> >>>+	__update_cache(addr, pteval);
> >>>+cache_sync_done:
> >>>+	set_pte(ptep, pteval);
> >>>+}
> >>>+
> >>
> >>This seems crazy.
> >
> >Perhaps, but also correct...
> >
> >>I don't think any other architecture does this type of work in set_pte_at().
> >
> >Yes they do. As mentioned in the commit message see arm, ia64 or powerpc
> >for architectures that all do the same sort of thing in set_pte_at.
> >
> >>Can you look into finding a better way?
> >
> >Not that I can see.
> >
> >>What if you ...
> >>
> >>
> >>>  /*
> >>>   * (pmds are folded into puds so this doesn't get actually called,
> >>>   * but the define is needed for a generic inline function.)
> >>>@@ -430,15 +449,12 @@ static inline pte_t pte_modify(pte_t pte, pgprot_t newprot)
> >>>
> >>>  extern void __update_tlb(struct vm_area_struct *vma, unsigned long address,
> >>>  	pte_t pte);
> >>>-extern void __update_cache(struct vm_area_struct *vma, unsigned long address,
> >>>-	pte_t pte);
> >>>
> >>>  static inline void update_mmu_cache(struct vm_area_struct *vma,
> >>>  	unsigned long address, pte_t *ptep)
> >>>  {
> >>>  	pte_t pte = *ptep;
> >>>  	__update_tlb(vma, address, pte);
> >>>-	__update_cache(vma, address, pte);
> >>
> >>... Reversed the order of these two operations?
> >
> >It would make no difference. The window for the race exists between
> >flush_dcache_page & set_pte_at. update_mmu_cache isn't called until
> >later than set_pte_at, so cannot possibly avoid the race. The commit
> >message walks through where the race exists - I don't think you've read
> >it.
> 
> 
> I think the code that calls set_pte_at() should be fixed.
> 
> If cache maintenance is needed before modifying the page tables, that is
> explicitly done in the calling code.
> 
> In migrate.c (remove_migration_pte, similar in do_swap_page) we have:
>    .
>    .
>    .
> 	flush_dcache_page(new);
> 	set_pte_at(mm, addr, ptep, pte);
>    .
>    .
>    .
> 
> Similar in huge_memory.c (unfreeze_page_vma, freeze_page_vma, etc.)
> 
> The point being, the callers have the knowledge about what is changing and
> should make sure they do the right thing to keep the caches consistent.  The
> job of set_pte_at() is to manipulate the page tables, nothing else.

...but if we do the flush in flush_dcache_page then we abandon the lazy
flushing.

Why do you want MIPS to be different to every other widely used
architecture that has this problem? set_pte_at clearly is not used only
to manipulate page tables, no matter what you might like.

Thanks,
    Paul