Re: [PATCH v3 09/31] arm64: Cache maintenance routines

[Simon Baatz <gmbnomis@gmail.com>]

Hi Catalin,

On Wed, Sep 12, 2012 at 10:29:54AM +0100, Catalin Marinas wrote:
> 
> > > +void __flush_dcache_page(struct address_space *mapping, struct page *page)
> > > +{
> > > +     __flush_dcache_area(page_address(page), PAGE_SIZE);
> > > +}
> > > +
> > > +void __sync_icache_dcache(pte_t pte)
> > > +{
> > > +     unsigned long pfn;
> > > +     struct page *page;
> > > +
> > > +     pfn = pte_pfn(pte);
> > > +     if (!pfn_valid(pfn))
> > > +             return;
> > > +
> > > +     page = pfn_to_page(pfn);
> > > +     if (!test_and_set_bit(PG_dcache_clean, &page->flags))
> > > +             __flush_dcache_page(NULL, page);
> > > +     __flush_icache_all();
> > > +}
> > > +
> > > +/*
> > > + * Ensure cache coherency between kernel mapping and userspace mapping of this
> > > + * page.
> > > + */
> > > +void flush_dcache_page(struct page *page)
> > > +{
> > > +     struct address_space *mapping;
> > > +
> > > +     /*
> > > +      * The zero page is never written to, so never has any dirty cache
> > > +      * lines, and therefore never needs to be flushed.
> > > +      */
> > > +     if (page == ZERO_PAGE(0))
> > > +             return;
> > > +
> > > +     mapping = page_mapping(page);
> > > +
> > > +     if (mapping && !mapping_mapped(mapping))
> > > +             clear_bit(PG_dcache_clean, &page->flags);
> > > +     else {
> > > +             __flush_dcache_page(mapping, page);
> > > +             if (mapping)
> > > +                     __flush_icache_all();
> > 
> > 
> > Is this necessary to ensure I/D coherency? Then, I would have
> > expected
> > 
> >                 if (mapping) {
> >                         __flush_dcache_page(mapping, page);
> >                         __flush_icache_all();
> >                 }
> > 
> > similar to __sync_icache_dcache() above.
> 
> We don't want to do additional flushing if !mapping_mapped() as the page
> isn't mapped in user space. In this case we defer the flushing until
> __sync_icache_dcache().
> 
> The other case is for anonymous pages where mapping == NULL. Here we
> don't defer the D-cache flush and do it directly. The I-cache, if
> needed, is handled later in __sync_icache_dcache(). This was based on
> the idea that this case is mainly for the args/env page which is mapped
> shortly after anyway, so not worth deferring. On AArch64, I don't think
> it makes any difference. Maybe a slight improvement (at least in
> clarity) in flush_dcache_page():
> 
> 	if (mapping && mapping_mapped(mapping)) {
> 		__flush_dcache_page(page);
> 		__flush_icache_all();
> 		set_bit(PG_dcache_clean, &page->flags);
> 	} else {
> 		clear_bit(PG_dcache_clean, &page->flags);
> 	}
> 
> In this case the anonymous page flushing is deferred to
> __sync_icache_dcache().

Yes, I think this is much clearer. It makes clear that the D-cache
flush is done to avoid I/D incoherencies.  Previously, the obvious
question was "Why do we flush only the D-cache in some situations
given that it essentially behaves like a PIPT cache?" (the motivation
for this became only clear to me after your explanation.)

However, the reason why this was an obvious question for me is
interesting: I think the main use case you had in mind does not
happen since about five years (since commit b6a2fe, "mm: variable
length argument support").  And I had a completely different main use
case in mind.

I reckon that the use case you refer to is the use in fs/exec.c? 
Copying arg/env was changed in the commit mentioned above.  This also
changed the use of flush_dcache_page() (which is not supposed to
handle anon pages) into flush_kernel_dcache_page() (which is supposed
to handle kernel modified user pages, i.e.  also anon pages).

Nevertheless, the __flush_dcache_page(mapping, page) in the
mapping==NULL case is absoluty necessary for aliasing D-caches on arm
for the use case I had in mind (which does not apply to arm64 and
thus my question).

In case of direct I/O (and probably also in other cases like SG_IO)
the block layer will see pages from get_user_pages() directly, i.e.
also anonymous pages. Many drivers (especially emulated storage
drivers like dm-crypt) use flush_dcache_page() after modifying a
page. Although flush_dcache_page() is not even supposed to handle
anonymous pages, it flushes the kernel mapping of the page because of
this code line and everything is well on aliasing D-caches.

Ironically, flush_kernel_dcache_page(), which is specifically
designed to handle this case, does not on arm.  Thus, those few parts
of the kernel which use flush_kernel_dcache_page() may fail horribly
(for example the scatterlist memory iterator API, see [1]).

Back to arm64 (and possibly to arm with non-aliasing D-caches?), this
also means that the saved D-cache flush in the anonymous page case is
not only a slight improvement on clarity, but may avoid a
considerable number of D-cache flushes in some I/O situations.  (If
it is still correct that there are no problems with the I-cache for
this use case.)

If now we could additionally avoid to flush the entire I-cache for
every page in direct I/O operations with user mapped page cache
pages (e.g. direct I/O read into an mmap region)...

 
> > What is the reason why the D-cache flush is done in different
> > cases than the following I-cache flush?
> 
> For __sync_icache_dcache(), we need to handle the situation where the
> page mapped into user space has been cleaned (D-cache) but there may be
> stale data in the I-cache. I think this can only happen with an
> ASID-tagged VIVT I-cache configuration (which is allowed on AArch64) if
> an existing page has been unmapped and the same virtual address remapped
> (withing the same mm context) to a different page that had been cleaned
> previously. We could optimise the __sync_icache_dcache() as below:
> 
> 	if (!test_and_set_bit(PG_dcache_clean, &page->flags)) {
> 		__flush_dcache_page(page);
> 		__flush_icache_all();
> 	} else if (icache_is_aivivt()) {
> 		__flush_icache_all();
> 	}

Sorry, this is out of my depth. I think I don't really understand the
cases leading to I/D incoherency.


- Simon

[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2012-July/111393.html

PS: You did not mention the following comment from my mail. It was
easy to overlook. Just to make sure you did not miss it:

> diff --git a/arch/arm64/mm/cache.S b/arch/arm64/mm/cache.S
> new file mode 100644
> index 0000000..3df0aa7
> --- /dev/null
> +++ b/arch/arm64/mm/cache.S

...
> +/*
> + *	__flush_kern_dcache_page(kaddr)


Should be:  __flush_dcache_area(kaddr,size)

> + *
> + *	Ensure that the data held in the page kaddr is written back
> to the
> + *	page in question.

s/page/area

> + *
> + *	- kaddr   - kernel address
> + *	- size    - size in question
> + */
> +ENTRY(__flush_dcache_area)
> +	dcache_line_size x2, x3
> +	add	x1, x0, x1
> +