Hi Simon,
On Fri, Sep 07, 2012 at 08:35:42PM +0100, Simon Baatz wrote:
> On Fri, Sep 07, 2012 at 05:26:44PM +0100, Catalin Marinas wrote:
> > +#define ARCH_HAS_FLUSH_ANON_PAGE
> > +static inline void flush_anon_page(struct vm_area_struct *vma,
> > + struct page *page, unsigned long vmaddr)
> > +{
> > + extern void __flush_anon_page(struct vm_area_struct *vma,
> > + struct page *, unsigned long);
> > + if (PageAnon(page))
> > + __flush_anon_page(vma, page, vmaddr);
>
>
> __flush_anon_page() does nothing. Shouldn't this be removed as well?
Yes, good point.
> > +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().
> 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();
}
> > + set_bit(PG_dcache_clean, &page->flags);
> > + }
> > +}
> > +EXPORT_SYMBOL(flush_dcache_page);
> > +
> > +void __flush_anon_page(struct vm_area_struct *vma, struct page *page, unsigned long vmaddr)
> > +{
> > +}
>
> Note that the __flush_dcache_page(mapping, page) in
> flush_dcache_page() above is called when page is an anonymous page
> (since mapping == NULL in this case). If the call to
> __flush_dcache_page() is right above, it should be needed
> here as well?
flush_anon_page() is called when the kernel needs to access an anonymous
page. Given that the D-cache behaves like a PIPT, there is no need for
additional flushing here. The flush_dcache_page() call was based on the
assumption that such page needs flushing anyway and it's not worth
deferring. But the code may be easier to understand as I suggested above
(and slightly more optimal for the VIPT I-cache case).
It looks like any other architecture does something different here.
--
Catalin
--
To unsubscribe from this list: send the line "unsubscribe linux-arch" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
