[PATCH V3 2/2] ARM: Handle user space mapped pages in flush_kernel_dcache_page

[Simon Baatz]

Hi Russel,

On Wed, May 08, 2013 at 07:43:54PM +0100, Russell King - ARM Linux wrote:
> On Wed, May 08, 2013 at 04:08:00PM +0100, Catalin Marinas wrote:
> > On Sat, May 04, 2013 at 09:21:27AM +0100, Ming Lei wrote:
> > > On Fri, May 3, 2013 at 6:02 PM, Catalin Marinas <catalin.marinas at arm.com> wrote:
> > > >>
> > > >> I assume that you inhibited the call to flush_dcache_page() in
> > > >> __get_user_pages() for anon pages.  Otherwise, you will be flooded
> > > >> with warnings.
> > > >
> > > > I haven't done any stress testing so I don't think I hit this code path,
> > > > so no warning. But yes, it should have triggered. Anyway, in this case
> > > > flush_dcache_page() should have just ignored (clearing PG_arch_1 is
> > > > harmless anyway if we also ignore this bit in __sync_icache_dcache for
> > > > non-aliasing caches).
> > > 
> > > Yes, maybe we can do a little optimization for O_DIRECT since no
> > > dcache alias and I/D coherency problem in this case on ARMv7, how
> > > about below change?
> > > 
> > > diff --git a/arch/arm/mm/flush.c b/arch/arm/mm/flush.c
> > > index 1c8f7f5..962a657 100644
> > > --- a/arch/arm/mm/flush.c
> > > +++ b/arch/arm/mm/flush.c
> > > @@ -287,6 +287,8 @@ void flush_dcache_page(struct page *page)
> > >  	    mapping && !mapping_mapped(mapping))
> > >  		clear_bit(PG_dcache_clean, &page->flags);
> > >  	else {
> > > +		if (!mapping && cache_is_vipt_nonaliasing())
> > > +			return;
> > >  		__flush_dcache_page(mapping, page);
> > >  		if (mapping && cache_is_vivt())
> > >  			__flush_dcache_aliases(mapping, page);
> > 
> > I wonder whether we could move the:
> > 
> > 	if (!mapping)
> > 		return
> > 
> > at the top of this function, IOW don't touch any anonymous pages. Would
> > anything be broken (apart from wrong API use)?
> 
> I suspect you may be missing something - and I suggest reading the Sparc
> version, which is where this PG_arch_1 stuff was first dreamed up (by
> DaveM).
> 
> Are you positive that this path can be eliminated?  Totally sure?  Have
> you checked the arg/env pages that fs/exec.c and the binfmt's insert into
> the new process?
> 
> The comments against the Sparc version suggest that the "else" clause
> is necessary ensure that case is adequately covered.

At least the fs/exec.c and the binfmt stuff use
flush_kernel_dcache_page() for 5 years or so now (see commit b6a2fe). 
Which makes sense, since flush_dcache_page() is not supposed to
handle anon pages anyway.

The problem are the tons of flush_dcache_page() uses that wrongly
assume that no anon pages need to be handled (e.g. 
drivers/block/loop.c and crypto/scatterwalk.c just to name two).

- Simon