Re: [PATCH] double flush_page_to_ram

Re: [PATCH] double flush_page_to_ram

[David S. Miller]

   From: Hugh Dickins <hugh@veritas.com>
   Date: Thu, 9 May 2002 13:03:52 +0100 (BST)

   filemap_nopage and shmem_nopage do flush_page_to_ram before returning
   page, but do_no_page also does flush_page_to_ram on any page it slots
   into the user address space.  It's memory.c's business, remove it from
   shmem and filemap (and cut outdated comment from when filemap copied).
   
Wrong, consider the case where we do early COW in do_no_page, you miss
a flush on the new-new page.

Please don't apply this patch.

[PATCH] double flush_page_to_ram

[Hugh Dickins]

filemap_nopage and shmem_nopage do flush_page_to_ram before returning
page, but do_no_page also does flush_page_to_ram on any page it slots
into the user address space.  It's memory.c's business, remove it from
shmem and filemap (and cut outdated comment from when filemap copied).

Hugh

--- 2.5.14/mm/filemap.c	Wed May  8 20:42:40 2002
+++ linux/mm/filemap.c	Thu May  9 12:49:10 2002
@@ -1532,12 +1532,7 @@
 		goto page_not_uptodate;
 
 success:
-	/*
-	 * Found the page and have a reference on it, need to check sharing
-	 * and possibly copy it over to another page..
-	 */
 	mark_page_accessed(page);
-	flush_page_to_ram(page);
 	return page;
 
 no_cached_page:
--- 2.5.14/mm/shmem.c	Wed May  1 12:22:32 2002
+++ linux/mm/shmem.c	Thu May  9 12:49:25 2002
@@ -638,7 +638,6 @@
 	if (shmem_getpage(inode, idx, &page))
 		return page;
 
-	flush_page_to_ram(page);
 	return(page);
 }
 

Re: [PATCH] double flush_page_to_ram

[David S. Miller]

   From: Andrea Arcangeli <andrea@suse.de>
   Date: Thu, 9 May 2002 15:01:46 +0200

   On Thu, May 09, 2002 at 04:56:43AM -0700, David S. Miller wrote:
   > Wrong, consider the case where we do early COW in do_no_page, you miss
   > a flush on the new-new page.
   
   so you mean we need a flush_page_to_ram also before the
   copy_user_highpage to be sure we copy uptodate contents of the
   pagecache? (possibly mapped writeable elsewhere in the user address
   space?)
   
That is correct.

The fact that this ends up with multiple flushes is one of
several reasons why Documentation/cachetlb.txt encourages
ports to move to the newer way to handle this (flush_dcache_page()
plus bits in {copy,clear}_user_page() to handle the cache issues
and not using flush_page_to_ram() at all).

Franks a lot,
David S. Miller
davem@redhat.com

Re: [PATCH] double flush_page_to_ram

[Andrea Arcangeli]

On Thu, May 09, 2002 at 04:56:43AM -0700, David S. Miller wrote:
>    From: Hugh Dickins <hugh@veritas.com>
>    Date: Thu, 9 May 2002 13:03:52 +0100 (BST)
> 
>    filemap_nopage and shmem_nopage do flush_page_to_ram before returning
>    page, but do_no_page also does flush_page_to_ram on any page it slots
>    into the user address space.  It's memory.c's business, remove it from
>    shmem and filemap (and cut outdated comment from when filemap copied).
>    
> Wrong, consider the case where we do early COW in do_no_page, you miss
> a flush on the new-new page.

so you mean we need a flush_page_to_ram also before the
copy_user_highpage to be sure we copy uptodate contents of the
pagecache? (possibly mapped writeable elsewhere in the user address
space?)

If not then I don't see how non-flushed pagecache can be mapped into
user address space.

Andrea

Re: [PATCH] double flush_page_to_ram

[David S. Miller]

   From: Hugh Dickins <hugh@veritas.com>
   Date: Thu, 9 May 2002 14:18:49 +0100 (BST)

   On Thu, 9 May 2002, David S. Miller wrote:
   > Wrong, consider the case where we do early COW in do_no_page, you miss
   > a flush on the new-new page.
   
   Of course we do, and then we don't map it into user address space;
   if it ever gets mapped into user address space later, do_no_page
   does the flush_page_to_ram then.

You miss the fact that if we do an early COW and another process
recently WROTE into that page via a shared MMAP, we will potentially
copy old data into the COW page we use for the current process.

Your changes are wrong and will cause corruption.

Re: [PATCH] double flush_page_to_ram

[David S. Miller]

   From: Andrea Arcangeli <andrea@suse.de>
   Date: Thu, 9 May 2002 15:21:16 +0200

   The reason I did this patch is because it was functional equivalent to
   old 2.4 kernels.
   
   in turn old 2.4 kernels were all wrong:
   
   	if (no_share) {
   		struct page *new_page = alloc_page(GFP_HIGHUSER);
   
   		if (new_page) {
   			copy_user_highpage(new_page, old_page, address);
   			flush_page_to_ram(new_page);
   		} else
   			new_page = NOPAGE_OOM;
   		page_cache_release(page);
   		return new_page;
   	}
   
   	flush_page_to_ram(old_page);
   	return old_page;
   
   they don't flush old_page before the the memcpy, they only flush the
   _anon_ page _after_ the memcpy like current kernel with vm updates or Hugh's
   patch is doing (never the pagecache if it's an early cow).

filemap.c:filemap_nopage() does the flush in 2.4.x, what are you
talking about?

   It seems like
   moving the early cow into memory.c fixed the flush_page_to_ram bug by
   luck, because Hugh's patch is otherwise equivalent to old 2.4 kernels.
   
   Confirm?
   
No it isn't.  In 2.4.x kernels filemap_nopage does the flush just
like it does now in 2.5.x  What ancient 2.4.x sources are you looking
at where filemap_nopage does not do the flush_page_to_ram right after
the success: label?

   If yes, I prefer to move the flush_page_to_ram on the _pagecache_
   _before_ the memcpy into memory.c, it's cleaner if the pagecache layer
   doesn't need to care about cache aliasing between kernel direct mapping
   and userspace address space (but that it only cares about struct pages
   and filesystem, so only kernel side), and that such user-related part is
   covered only in memory.c.
   
NO!  The correct answer is to kill off flush_page_to_ram altogether.
It is broken by design, and trying to make it work somehow "better"
in 2.5.x is a losing game.  We should make the ports fix up their
stuff for a mechanism that we know _does_ work and that is
flush_dcache_page plus {copy,clear}_user_page().

Re: [PATCH] double flush_page_to_ram

[Hugh Dickins]

On Thu, 9 May 2002, David S. Miller wrote:
>    From: Hugh Dickins <hugh@veritas.com>
>    Date: Thu, 9 May 2002 13:03:52 +0100 (BST)
> 
>    filemap_nopage and shmem_nopage do flush_page_to_ram before returning
>    page, but do_no_page also does flush_page_to_ram on any page it slots
>    into the user address space.  It's memory.c's business, remove it from
>    shmem and filemap (and cut outdated comment from when filemap copied).
>    
> Wrong, consider the case where we do early COW in do_no_page, you miss
> a flush on the new-new page.

Of course we do, and then we don't map it into user address space;
if it ever gets mapped into user address space later, do_no_page
does the flush_page_to_ram then.

Hugh

Re: [PATCH] double flush_page_to_ram

[Andrea Arcangeli]

On Thu, May 09, 2002 at 05:52:00AM -0700, David S. Miller wrote:
> That is correct.

so I will need to backout this one liner from my vm updates too.

--- 2.4.19-pre4/mm/filemap.c~aa-150-read_write_tweaks	Tue Mar 26
23:11:33 2002
+++ 2.4.19-pre4-akpm/mm/filemap.c	Tue Mar 26 23:11:33 2002
@@ -1968,7 +1968,6 @@ success:
 	 * and possibly copy it over to another page..
 	 */
 	mark_page_accessed(page);
-	flush_page_to_ram(page);
 	return page;
 
 no_cached_page:

The reason I did this patch is because it was functional equivalent to
old 2.4 kernels.

in turn old 2.4 kernels were all wrong:

	if (no_share) {
		struct page *new_page = alloc_page(GFP_HIGHUSER);

		if (new_page) {
			copy_user_highpage(new_page, old_page, address);
			flush_page_to_ram(new_page);
		} else
			new_page = NOPAGE_OOM;
		page_cache_release(page);
		return new_page;
	}

	flush_page_to_ram(old_page);
	return old_page;

they don't flush old_page before the the memcpy, they only flush the
_anon_ page _after_ the memcpy like current kernel with vm updates or Hugh's
patch is doing (never the pagecache if it's an early cow). It seems like
moving the early cow into memory.c fixed the flush_page_to_ram bug by
luck, because Hugh's patch is otherwise equivalent to old 2.4 kernels.

Confirm?

If yes, I prefer to move the flush_page_to_ram on the _pagecache_
_before_ the memcpy into memory.c, it's cleaner if the pagecache layer
doesn't need to care about cache aliasing between kernel direct mapping
and userspace address space (but that it only cares about struct pages
and filesystem, so only kernel side), and that such user-related part is
covered only in memory.c.

Andrea

Re: [PATCH] double flush_page_to_ram

[David S. Miller]

   From: Andrea Arcangeli <andrea@suse.de>
   Date: Thu, 9 May 2002 15:44:44 +0200
   
   Fine with me if Marcelo drops it, but I guess sparc/m68k/mips users
   won't like not be able to use 2.4 kernels anymore unless they first
   rewrite the entere cache flushing in the middle of a stable series.
   If you don't drop it in late 2.4 then my suggestion looks still a nice
   common code cleanup and it's functional equivalent to the current
   2.4.19pre8 code, so it cannot go wrong.

I only suggest doing it for 2.5, repeat, only 2.5

And yes adding the flush_page_to_ram call in filemap_nopage was a bug
fix in 2.4.10.  I believe it was from one of the SH port maintainers.

Re: [PATCH] double flush_page_to_ram

[Andrea Arcangeli]

On Thu, May 09, 2002 at 06:13:11AM -0700, David S. Miller wrote:
>    From: Andrea Arcangeli <andrea@suse.de>
>    Date: Thu, 9 May 2002 15:21:16 +0200
> 
>    The reason I did this patch is because it was functional equivalent to
>    old 2.4 kernels.
>    
>    in turn old 2.4 kernels were all wrong:
>    
>    	if (no_share) {
>    		struct page *new_page = alloc_page(GFP_HIGHUSER);
>    
>    		if (new_page) {
>    			copy_user_highpage(new_page, old_page, address);
>    			flush_page_to_ram(new_page);
>    		} else
>    			new_page = NOPAGE_OOM;
>    		page_cache_release(page);
>    		return new_page;
>    	}
>    
>    	flush_page_to_ram(old_page);
>    	return old_page;
>    
>    they don't flush old_page before the the memcpy, they only flush the
>    _anon_ page _after_ the memcpy like current kernel with vm updates or Hugh's
>    patch is doing (never the pagecache if it's an early cow).
> 
> filemap.c:filemap_nopage() does the flush in 2.4.x, what are you
> talking about?

I said "in turn old 2.4 kernels were all wrong", that means the _old_ 2.4
kernels not recent 2.4.x, look at 2.4.7 for istance.

> 
>    It seems like
>    moving the early cow into memory.c fixed the flush_page_to_ram bug by
>    luck, because Hugh's patch is otherwise equivalent to old 2.4 kernels.
>    
>    Confirm?
>    
> No it isn't.  In 2.4.x kernels filemap_nopage does the flush just

The change between 2.4.7 and 2.4.19pre8 doesn't look intentional, Hugh
just gone back to the old way of doing the flush in 2.4 that apparently
was buggy and that's why his patch his wrong (I assume this was the case
for him because I falled in the same trap too).

> like it does now in 2.5.x  What ancient 2.4.x sources are you looking
> at where filemap_nopage does not do the flush_page_to_ram right after
> the success: label?

anything older than 2.4.10.

> 
>    If yes, I prefer to move the flush_page_to_ram on the _pagecache_
>    _before_ the memcpy into memory.c, it's cleaner if the pagecache layer
>    doesn't need to care about cache aliasing between kernel direct mapping
>    and userspace address space (but that it only cares about struct pages
>    and filesystem, so only kernel side), and that such user-related part is
>    covered only in memory.c.
>    
> NO!  The correct answer is to kill off flush_page_to_ram altogether.
> It is broken by design, and trying to make it work somehow "better"
> in 2.5.x is a losing game.  We should make the ports fix up their
> stuff for a mechanism that we know _does_ work and that is
> flush_dcache_page plus {copy,clear}_user_page().

Fine with me if Marcelo drops it, but I guess sparc/m68k/mips users
won't like not be able to use 2.4 kernels anymore unless they first
rewrite the entere cache flushing in the middle of a stable series.
If you don't drop it in late 2.4 then my suggestion looks still a nice
common code cleanup and it's functional equivalent to the current
2.4.19pre8 code, so it cannot go wrong.

Andrea

Re: [PATCH] double flush_page_to_ram

[Hugh Dickins]

On Thu, 9 May 2002, Andrea Arcangeli wrote:
> 
> If yes, I prefer to move the flush_page_to_ram on the _pagecache_
> _before_ the memcpy into memory.c, it's cleaner if the pagecache layer
> doesn't need to care about cache aliasing between kernel direct mapping
> and userspace address space (but that it only cares about struct pages
> and filesystem, so only kernel side), and that such user-related part is
> covered only in memory.c.

Agreed.  It's better to keep flush_page_to_ram out of filemap_nopage
(and how many other _nopages that ought at present to have it according
to David, but don't).  As things stand, it's being done unnecessarily
twice on the shared maps: your proposal fixes that.

Hugh

Re: [PATCH] double flush_page_to_ram

[David S. Miller]

   From: Hugh Dickins <hugh@veritas.com>
   Date: Thu, 9 May 2002 14:50:11 +0100 (BST)
   
   Agreed.  It's better to keep flush_page_to_ram out of filemap_nopage
   (and how many other _nopages that ought at present to have it according
   to David, but don't).  As things stand, it's being done unnecessarily
   twice on the shared maps: your proposal fixes that.

There are many other cases where flush_page_to_ram overflushes,
its deficiencies are known.

This is why I am suggesting "don't touch this in 2.4 and let's
kill it off in 2.5"