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
