Re: problems in commit 2d4dc890b5c8 (block: add helpers to run flush_dcache_page() against a bio and a request's pages)

[Russell King <rmk@arm.linux.org.uk>]

On Thu, Dec 10, 2009 at 11:59:16AM -0600, James Bottomley wrote:
> On Thu, 2009-12-10 at 17:48 +0000, Russell King wrote:
> > Well, it would mean every kunmap_atomic() gains an expensive cache flush
> > no matter what it's doing.  That would be very bad for things like
> > copy_user_highpage(), where we kmap the source and destination pages,
> > and then kunmap both.
> > 
> > However, there are cases where we do want to flush on kunmap_atomic() -
> > again, taking the copy_user_highpage() example, we want to ensure that
> > data written to the page is going to be visible in some way.  IOW, we
> > already have this:
> > 
> >         kfrom = kmap_atomic(from, KM_USER0);
> >         kto = kmap_atomic(to, KM_USER1);
> >         copy_page(kto, kfrom);
> > #ifdef CONFIG_HIGHMEM
> >         /*
> >          * kmap_atomic() doesn't set the page virtual address, and
> >          * kunmap_atomic() takes care of cache flushing already.
> >          */
> >         if (page_address(to) != NULL)
> > #endif
> >                 __cpuc_flush_dcache_page(kto);
> >         kunmap_atomic(kto, KM_USER1);
> >         kunmap_atomic(kfrom, KM_USER0);
> > 
> > would become something like:
> > 
> > 	...
> > 	kunmap_atomic_flush(kto, KM_USER1);
> > 	kunmap_atomic(kfrom, KM_USER0);
> > 
> > So I think what we want to add is kunmap_atomic_flush() for the cases
> > where we need the additional coherency, or maybe a kunmap_atomic_noflush()
> > for those which we don't.
> 
> So if you think about it on a VI architecture, assuming we modified some
> data in the kmap page at the returned address, why would we ever want to
> unmap without flushing?  The only case I can think of is when we *know*
> the kmap address and the other addresses are all congruent (so we have
> no aliases).

The above example code comes from non-aliasing VIPT - where for the
vast majority of cases, unmapping without flush is fine provided we
haven't written data.  However, unmapping with flush is required to
ensure coherency with the instruction cache.

> So I really think in kunmap(_atomic) we need to check to see if the page
> was modified (using the pte flags),

That's fine _if_ you have such flags.  Not everything has - in which
case, going down the route you're proposing means that every single
kunmap_atomic() ends up having to flush the whole page whether it's
actually needed on an architecture "just because" - with no technical
reason to actually do so.

We need the two cases separated for hardware which is not PARISC.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of: