ARM highmem stuff - 5687/1

ARM highmem stuff - 5687/1

[Russell King - ARM Linux]

Do we need a similar fix for flush_anon_page()?  In both cases, can we
just avoid calling __cpuc_flush_dcache_page() if PageHighMem(page) is
true rather than checking page_address(page) is non-zero?

ARM highmem stuff - 5687/1

[Nicolas Pitre]

On Wed, 2 Sep 2009, Russell King - ARM Linux wrote:

> Do we need a similar fix for flush_anon_page()?

Hmmmm... Maybe.  Especially since the only usage of flush_anon_page() is 
in __get_user_pages() where the page passed to flush_anon_page() may or 
may not be kmapped (it is not explicitly kmapped in that function, but 
non atomic kmaps are lazily unmapped and would still require to be 
flushed if their mapping is still there).  However, in 
__get_user_pages(), there is a flush_dcache_page() right after the call 
to flush_anon_page(), so the __cpuc_flush_dcache_page() in 
__flush_anon_page() appears redundant to me and could simply be removed 
entirely.

> In both cases, can we just avoid calling __cpuc_flush_dcache_page() if 
> PageHighMem(page) is true rather than checking page_address(page) is 
> non-zero?

No.  Like I say above, unlike with kunmap_atomic(), the cache is 
untouched by kunmap().  This is because kmap() and kunmap() can be used 
repeatedly on the same set of pages and their actual mappings don't need 
to be flushed and removed between those calls.  However if a 
flush_dcache_page() is required on a highmem page that still has a valid 
kmap mapping then the flush is relevant.


Nicolas

ARM highmem stuff - 5687/1

[Nicolas Pitre]

On Wed, 2 Sep 2009, Nicolas Pitre wrote:

> On Wed, 2 Sep 2009, Russell King - ARM Linux wrote:
> 
> > Do we need a similar fix for flush_anon_page()?
> 
> Hmmmm... Maybe.  Especially since the only usage of flush_anon_page() is 
> in __get_user_pages() where the page passed to flush_anon_page() may or 
> may not be kmapped (it is not explicitly kmapped in that function, but 
> non atomic kmaps are lazily unmapped and would still require to be 
> flushed if their mapping is still there).  However, in 
> __get_user_pages(), there is a flush_dcache_page() right after the call 
> to flush_anon_page(), so the __cpuc_flush_dcache_page() in 
> __flush_anon_page() appears redundant to me and could simply be removed 
> entirely.

This made me think about a race that exists with the highmem kmap 
business though...

Let's suppose a highmem page is kmap'd with kmap().  A pkmap entry is 
used, the page mapped to it, and the virtual cache is dirtied.  Then 
kunmap() is used which does virtually nothing except for decrementing a 
usage count.

Then, let's suppose the _same_ page gets mapped using kmap_atomic().  
It is therefore mapped onto a fixmap entry instead, which has a 
different virtual address unaware of the dirty data for that page 
sitting in the pkmap mapping.

Fortunately the fix is simple:

diff --git a/arch/arm/mm/highmem.c b/arch/arm/mm/highmem.c
index a34954d..73cae57 100644
--- a/arch/arm/mm/highmem.c
+++ b/arch/arm/mm/highmem.c
@@ -40,11 +40,16 @@ void *kmap_atomic(struct page *page, enum km_type type)
 {
 	unsigned int idx;
 	unsigned long vaddr;
+	void *kmap;
 
 	pagefault_disable();
 	if (!PageHighMem(page))
 		return page_address(page);
 
+	kmap = kmap_high_get(page);
+	if (kmap)
+		return kmap;
+
 	idx = type + KM_TYPE_NR * smp_processor_id();
 	vaddr = __fix_to_virt(FIX_KMAP_BEGIN + idx);
 #ifdef CONFIG_DEBUG_HIGHMEM
@@ -80,6 +85,9 @@ void kunmap_atomic(void *kvaddr, enum km_type type)
 #else
 		(void) idx;  /* to kill a warning */
 #endif
+	} else if (vaddr >= PKMAP_ADDR(0) && vaddr < PKMAP_ADDR(LAST_PKMAP)) {
+		/* this address was obtained through kmap_high_get() */
+		kunmap_high(pte_page(pkmap_page_table[PKMAP_NR(vaddr)]));
 	}
 	pagefault_enable();
 }


Nicolas

ARM highmem stuff - 5687/1

[Nicolas Pitre]

On Wed, 2 Sep 2009, Nicolas Pitre wrote:

> On Wed, 2 Sep 2009, Russell King - ARM Linux wrote:
> 
> > Do we need a similar fix for flush_anon_page()?
> 
> Hmmmm... Maybe.  Especially since the only usage of flush_anon_page() is 
> in __get_user_pages() where the page passed to flush_anon_page() may or 
> may not be kmapped (it is not explicitly kmapped in that function, but 
> non atomic kmaps are lazily unmapped and would still require to be 
> flushed if their mapping is still there).  However, in 
> __get_user_pages(), there is a flush_dcache_page() right after the call 
> to flush_anon_page(), so the __cpuc_flush_dcache_page() in 
> __flush_anon_page() appears redundant to me and could simply be removed 
> entirely.

Something like this:

    [ARM] remove redundant cache flush from __flush_anon_page()
    
    The only usage of flush_anon_page() is in __get_user_pages() where
    a call to flush_dcache_page() is performed immediately after the call
    to flush_anon_page().  This makes the __cpuc_flush_dcache_page() in
    __flush_anon_page() redundant.
    
    Signed-off-by: Nicolas Pitre <nico@marvell.com>

diff --git a/arch/arm/mm/flush.c b/arch/arm/mm/flush.c
index 575f3ad..c0f4039 100644
--- a/arch/arm/mm/flush.c
+++ b/arch/arm/mm/flush.c
@@ -262,11 +262,4 @@ void __flush_anon_page(struct vm_area_struct *vma, struct page *page, unsigned l
 		 */
 		flush_pfn_alias(pfn, vmaddr);
 	}
-
-	/*
-	 * Invalidate kernel mapping.  No data should be contained
-	 * in this mapping of the page.  FIXME: this is overkill
-	 * since we actually ask for a write-back and invalidate.
-	 */
-	__cpuc_flush_dcache_page(page_address(page));
 }

ARM highmem stuff - 5687/1

[Russell King - ARM Linux]

On Wed, Sep 02, 2009 at 11:52:33AM -0400, Nicolas Pitre wrote:
> On Wed, 2 Sep 2009, Russell King - ARM Linux wrote:
> 
> > Do we need a similar fix for flush_anon_page()?
> 
> Hmmmm... Maybe.  Especially since the only usage of flush_anon_page() is 
> in __get_user_pages() where the page passed to flush_anon_page() may or 
> may not be kmapped (it is not explicitly kmapped in that function, but 
> non atomic kmaps are lazily unmapped and would still require to be 
> flushed if their mapping is still there).  However, in 
> __get_user_pages(), there is a flush_dcache_page() right after the call 
> to flush_anon_page(), so the __cpuc_flush_dcache_page() in 
> __flush_anon_page() appears redundant to me and could simply be removed 
> entirely.

I'm not sure this is right.  It looks to me as if it is possible for
a page have both a mapping _and_ be an anonymous page - which can happen
if the page is an anonymous page which has been placed into the swapcache.

In this case, flush_dcache_page() may just set the PG_dcache_dirty bit
without flushing anything.

I do know that flush_anon_page() was required to stabilise the list server.
With the list server now effectively redundant, I've no way to test this
stuff, so I'm going to leave the duplicate flushing in place and won't
apply your patch.

ARM highmem stuff - 5687/1

[Nicolas Pitre]

On Fri, 4 Sep 2009, Russell King - ARM Linux wrote:

> On Wed, Sep 02, 2009 at 11:52:33AM -0400, Nicolas Pitre wrote:
> > On Wed, 2 Sep 2009, Russell King - ARM Linux wrote:
> > 
> > > Do we need a similar fix for flush_anon_page()?
> > 
> > Hmmmm... Maybe.  Especially since the only usage of flush_anon_page() is 
> > in __get_user_pages() where the page passed to flush_anon_page() may or 
> > may not be kmapped (it is not explicitly kmapped in that function, but 
> > non atomic kmaps are lazily unmapped and would still require to be 
> > flushed if their mapping is still there).  However, in 
> > __get_user_pages(), there is a flush_dcache_page() right after the call 
> > to flush_anon_page(), so the __cpuc_flush_dcache_page() in 
> > __flush_anon_page() appears redundant to me and could simply be removed 
> > entirely.
> 
> I'm not sure this is right.  It looks to me as if it is possible for
> a page have both a mapping _and_ be an anonymous page - which can happen
> if the page is an anonymous page which has been placed into the swapcache.

Hmmm...  Right.  However...

> In this case, flush_dcache_page() may just set the PG_dcache_dirty bit
> without flushing anything.

But only if there is indeed a mapping and it is unmapped.  In which case 
isn't the flush going to happen before the page gets mapped into user 
space anyway?

> I do know that flush_anon_page() was required to stabilise the list server.

Do you mean __cpuc_flush_dcache_page() inside flush_anon_page()?

Might this be because the page can still be dirty by not being mapped 
yet and therefore PG_dcache_dirty is still set at the moment 
flush_anon_page() is called, and then some IO is performed on that page 
using DMA to swap it out for example?  Is that possible? If so that 
would look more logical to me if the call to __cpuc_flush_dcache_page() 
was conditional on the PG_dcache_dirty bit.

> With the list server now effectively redundant, I've no way to test this
> stuff, so I'm going to leave the duplicate flushing in place and won't
> apply your patch.

If my assertion above is true, then I agree that the patch is wrong.


Nicolas

ARM highmem stuff - 5687/1

[Russell King - ARM Linux]

On Fri, Sep 04, 2009 at 03:33:53PM -0400, Nicolas Pitre wrote:
> On Fri, 4 Sep 2009, Russell King - ARM Linux wrote:
> > In this case, flush_dcache_page() may just set the PG_dcache_dirty bit
> > without flushing anything.
> 
> But only if there is indeed a mapping and it is unmapped.  In which case 
> isn't the flush going to happen before the page gets mapped into user 
> space anyway?

Only if it still has a mapping at that point.

> > I do know that flush_anon_page() was required to stabilise the list server.
> 
> Do you mean __cpuc_flush_dcache_page() inside flush_anon_page()?

I really don't know.  flush_anon_page() wasn't written incrementally, it
was written to allow the accesses as described in cachetlb.txt to work as
expected.  The rationale for this function is further described in commit
03beb07664d768db97bf454ae5c9581cd4737bb4.

I really can't comment any further on this, and I'm really concerned
about changing the current behaviour.  We know what we currently have
works, and given that we've lost a way to test changes in this area
anymore, I think it's definitely a case if leaving well alone.

ARM highmem stuff - 5687/1

[Nicolas Pitre]

On Fri, 4 Sep 2009, Russell King - ARM Linux wrote:

> On Fri, Sep 04, 2009 at 03:33:53PM -0400, Nicolas Pitre wrote:
> > On Fri, 4 Sep 2009, Russell King - ARM Linux wrote:
> > > In this case, flush_dcache_page() may just set the PG_dcache_dirty bit
> > > without flushing anything.
> > 
> > But only if there is indeed a mapping and it is unmapped.  In which case 
> > isn't the flush going to happen before the page gets mapped into user 
> > space anyway?
> 
> Only if it still has a mapping at that point.

Hmmmmm... Isn't that a bug?  What if PG_dcache_dirty is set even if 
there is no more mapping for the page?  Wouldn't that mean that the 
kernel view of the page might still have dirty cache lines that could 
eventually overwrite what user space might have written to that page?
In other words, wouldn't that be clearer and safer to always flush the 
cache whenever the bit indicating cache dirtiness is set?

> > > I do know that flush_anon_page() was required to stabilise the list server.
> > 
> > Do you mean __cpuc_flush_dcache_page() inside flush_anon_page()?
> 
> I really don't know.  flush_anon_page() wasn't written incrementally, it
> was written to allow the accesses as described in cachetlb.txt to work as
> expected.  The rationale for this function is further described in commit
> 03beb07664d768db97bf454ae5c9581cd4737bb4.

There it is said:

    Currently, get_user_pages() returns fully coherent pages to the kernel for
    anything other than anonymous pages.  This is a problem for things like
    fuse and the SCSI generic ioctl SG_IO which can potentially wish to do DMA
    to anonymous pages passed in by users.

So I think that matches my earlier theory about such pages marked with 
PG_dcache_dirty that user space didn't fault yet, and therefore still 
not coherent wrt its kernel view.  Doing DMA on such a page would not 
produce intended results unless __cpuc_flush_dcache_page() is used on 
it.

> I really can't comment any further on this, and I'm really concerned
> about changing the current behaviour.  We know what we currently have
> works, and given that we've lost a way to test changes in this area
> anymore, I think it's definitely a case if leaving well alone.

Good enough.  However I'd suggest replacing 
__cpuc_flush_dcache_page(page_address(page)) with 
__flush_dcache_page(NULL, page).  This way the unmapped highmem page 
fix would benefit __flush_anon_page() as well and be located in only one 
place, with otherwise the same result.


Nicolas