Hi Paul
I tested your patch set and compared with our in house tree:
Indeed the vanilla kernel cache handling + the first three patches from your set fails booting from UBIFS. Applying the fourth patch resolves the problem.
Now consider this. Our tree runs with Steven J. Hill's original patch set that fixed HIGHMEM for non-DMA block devices and we can boot ok with UBIFS. The cache flushing happens inside flush_icache_page as on the vanilla kernel.
I would suggest that you take a close look on Steven's old patch set and figure out what he did different. You will hopefully find that we can avoid flushing inside set_pte_at.
BR
Lars
> 4 mars 2016 kl. 11:37 skrev Paul Burton <paul.burton@xxxxxxxxxx>:
>
> 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
