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

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

Hi Leonid,

On Wed, Mar 02, 2016 at 07:03:35PM -0800, Leonid Yegoshin wrote:
> Paul Burton wrote:
> >It is, however, used in such a way by others & seems to me like the only
> >correct way to implement the lazy cache flushing. The alternative would
> >be to adjust all generic code to ensure flush_icache_page gets called
> >before set_pte_at
> 
> ... which is an exact case right now.

No, it isn't. I included call traces for 2 cases where this is not the
case right in the commit message.

> Both calls of flush_icache_page() are:
> 
> 1) do_swap_page()
> 
>         flush_icache_page(vma, page);
>         if (pte_swp_soft_dirty(orig_pte))
>                 pte = pte_mksoft_dirty(pte);
>         set_pte_at(mm, address, page_table, pte);
> ...
> 
> 2) do_set_pte()
> 
>         flush_icache_page(vma, page);
>         entry = mk_pte(page, vma->vm_page_prot);
>         if (write)
>                 entry = maybe_mkwrite(pte_mkdirty(entry), vma);
>         if (anon) {
>                 inc_mm_counter_fast(vma->vm_mm, MM_ANONPAGES);
>                 page_add_new_anon_rmap(page, vma, address);
>         } else {
>                 inc_mm_counter_fast(vma->vm_mm, MM_FILEPAGES);
>                 page_add_file_rmap(page);
>         }
>         set_pte_at(vma->vm_mm, address, pte, entry);
> 
>         /* no need to invalidate: a not-present page won't be cached */
>         update_mmu_cache(vma, address, pte);
> ....
> 
> You should only be sure that flush_icache_page() completes a lazy D-cache
> flush.
> 
> - Leonid.

Well of course that's fine in those 2 cases. Of course both callers of
flush_icache_page call flush_icache_page - that's a meaningless
tautology. You're grepping for the wrong thing.

The problem is that there are other code paths which dirty pages (ie.
call flush_dcache_page), then get as far as set_pte_at *without* calling
flush_icache_page. Again - I included call traces from 2 paths that hit
this right in the commit message.

So:

  - flush_icache_page isn't always called before we *need* to writeback
    the page from the dcache. This is demonstrably the case (again, see
    the commit message), and causes bugs when using UBIFS on boards
    using the pistachio SoC at least.

  - flush_icache_page is indicated as something that should go away in
    Documentation/cachetlb.txt. Why do you feel we should make use of
    it?

  - Other architectures (at least arm, ia64 & powerpc which may not be
    an exhaustive list) handle this in set_pte_at too. Doing it in the
    same way can only be good for consistency.

So flush_icache_page is insufficient, apparently disliked & not the way
any other architectures solve the exact same problem (again, because it
does *not* cover all cases).

Thanks,
    Paul