Date:   Thu, 4 Nov 2021 18:04:45 +0000
From:   Catalin Marinas <catalin.marinas@....com>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     Matthew Wilcox <willy@...radead.org>,
        Christoph Hellwig <hch@....de>,
        linux-arch <linux-arch@...r.kernel.org>,
        Ira Weiny <ira.weiny@...el.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Russell King <linux@...linux.org.uk>
Subject: Re: flush_dcache_page vs kunmap_local

+ rmk

On Thu, Nov 04, 2021 at 10:08:40AM -0700, Linus Torvalds wrote:
> On Thu, Nov 4, 2021 at 9:54 AM Catalin Marinas <catalin.marinas@....com> wrote:
> > We do. flush_dcache_page() is not just about virtual caches. On arm32/64
> > (and powerpc), even with PIPT-like caches, we use it to flag a page's
> > D-cache as no longer clean. Subsequently in set_pte_at(), if the mapping
> > is executable, we do the cache maintenance to ensure the I and D caches
> > are coherent with each other.
> 
> Ugh,. ok, so we have two very different use-cases for that function.
> 
> Perhaps more importantly, they have hugely different semantics. For
> you, it's about pages that can be mapped executable, so it's only
> relevant for mappable pages.
> 
> For the traditional broken pure virtual cache case, it's not about
> user mappings at all, it's about any data structure that we might have
> in highmem.
> 
> Of course, I think we got rid of most of the other uses of highmem,
> and we no longer put any "normal" kernel data in highmem pages. There
> used to be patches that did inodes and things like that in highmem,
> and they actually depended on the "cache the virtual address so that
> it's always the same" behavior.

We can still have ptes in highmem.

> > I wouldn't add this call to kmap/kunmap_local(), it would be a slight
> > unnecessary overhead (we had a customer complaining about kmap_atomic()
> > breaking write-streaming, I think the new kmap_local() solved this
> > problem, if in the right context).
> 
> kmap_local() ends up being (I think) fundamentally broken for virtual
> cache coherency anyway, because two different CPU's can see two
> different virtual addresses at the same time for the same page (in
> ways that the old kmap interfaces could not).

Luckily I don't think we have a (working) SMP system with VIVT caches.
On UP, looking at arm, for VIVT caches it flushes the D-cache before
kunmap_local() (arch_kmap_local_pre_unmap()). So any new kmap_local()
would see the correct data even if it's in a different location.

> So maybe the answer is "let's forget about the old virtual cache
> coherence issue, and make it purely about the I$ mapping case".

We still have VIVT processors supported in the kernel and a few where
the VIPT cache is aliasing (some ARMv6 CPUs). On these,
flush_dcache_page() is still used to ensure the user aliases are
coherent with the kernel one, so it's not just about the I/D-cache
coherency.

> At that point, kmap is irrelevant from a virtual address standpoint
> and so it doesn't make much sense to fliush on kunmap - but anybody
> who writes to a page still needs that flush_dcache_page() thing.

The cachetlb.rst doc states the two cases where flush_dcache_page()
should be called:

1. After writing to a page cache page (that's what we need on arm64 for
   the I-cache).

2. Before reading from a page cache page and user mappings potentially
   exist. I think arm32 ensures the D-cache user aliases are coherent
   with the kernel one (added rmk to confirm).

Now, whether the kernel code does call flush_dcache_page() in the above
scenarios is another matter. But if we are to remove the 2nd case, for
VIVT/aliasing-VIPT hardware we'd need kmap() to perform some cache
maintenance even if the page is not in highmem.

-- 
Catalin

Powered by blists - more mailing lists