flush_dcache_page does too much?

flush_dcache_page does too much?

[anfei]

Hello,

I'm studying the cache alias problem especially of VIPT, I found
function flush_dcache_page() does much more operations on ARM than MIPS.
Can we not flush the userspace mappings and icache, just like MIPS?
Are the cache more consistent with these operations?

As far as I know, flush_dcache_page is usually used as this:
        kmap_atomic(page, ...);
        write the page;
        flush_dcache_page(page);
        kunmap_atomic(...);
called in the path of write()/..., but since mmap() + write() is not
ensured to work (even on ARM currently), it's the userspace to consider
msync()/munmap(), it looks okay without flush the userspace mappings
here.  Other cases seem the same if the userspace takes charge of the 
cache problem.

Thanks,
Anfei.

flush_dcache_page does too much?

[Russell King - ARM Linux]

On Mon, Jan 18, 2010 at 09:13:46PM +0800, anfei wrote:
> I'm studying the cache alias problem especially of VIPT, I found
> function flush_dcache_page() does much more operations on ARM than MIPS.
> Can we not flush the userspace mappings and icache, just like MIPS?
> Are the cache more consistent with these operations?
> 
> As far as I know, flush_dcache_page is usually used as this:
>         kmap_atomic(page, ...);
>         write the page;
>         flush_dcache_page(page);
>         kunmap_atomic(...);
> called in the path of write()/..., but since mmap() + write() is not
> ensured to work (even on ARM currently), it's the userspace to consider
> msync()/munmap(), it looks okay without flush the userspace mappings
> here.  Other cases seem the same if the userspace takes charge of the 
> cache problem.

On VIPT on ARM, flush_dcache_page() flushes:

1. the direct kernel mapping and aliases of that (which read()/write()
   will touch.)

2. the user aliases, which may not be coherent with the direct kernel
   mapping.

It is unsafe to avoid dealing with any of those - and doing so will cause
shared mappings to be incoherent with filesystem IO.

flush_dcache_page does too much?

[anfei]

On Mon, Jan 18, 2010 at 01:33:04PM +0000, Russell King - ARM Linux wrote:
> On Mon, Jan 18, 2010 at 09:13:46PM +0800, anfei wrote:
> > I'm studying the cache alias problem especially of VIPT, I found
> > function flush_dcache_page() does much more operations on ARM than MIPS.
> > Can we not flush the userspace mappings and icache, just like MIPS?
> > Are the cache more consistent with these operations?
> > 
> > As far as I know, flush_dcache_page is usually used as this:
> >         kmap_atomic(page, ...);
> >         write the page;
> >         flush_dcache_page(page);
> >         kunmap_atomic(...);
> > called in the path of write()/..., but since mmap() + write() is not
> > ensured to work (even on ARM currently), it's the userspace to consider
> > msync()/munmap(), it looks okay without flush the userspace mappings
> > here.  Other cases seem the same if the userspace takes charge of the 
> > cache problem.
> 
> On VIPT on ARM, flush_dcache_page() flushes:
> 
> 1. the direct kernel mapping and aliases of that (which read()/write()
>    will touch.)
> 
> 2. the user aliases, which may not be coherent with the direct kernel
>    mapping.
> 
> It is unsafe to avoid dealing with any of those - and doing so will cause
> shared mappings to be incoherent with filesystem IO.

Do you mean this implementation can ensure the coherence between write
and shared mmapings?  But it's easy to reproduce the alias problem by
this simple testcase (w/o error handler) on omap2430 with VIPT cache:

---
#include <stdio.h>
#include <sys/mman.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>
#include <unistd.h>

int main(void)
{
        int fd;
        int *addr;
        int tmp;
        int val = 0x11111111;

        fd = open("abc", O_RDWR);
        addr = mmap(NULL, 4096, PROT_READ|PROT_WRITE, MAP_SHARED, fd, 0);
        *(addr+0) = 0x44444444;
        tmp = *(addr+0);
        *(addr+1) = 0x77777777;
        write(fd, &val, sizeof(int));
        close(fd);

        return 0;
}

flush_dcache_page does too much?

[Russell King - ARM Linux]

On Mon, Jan 18, 2010 at 09:54:31PM +0800, anfei wrote:
> Do you mean this implementation can ensure the coherence between write
> and shared mmapings?  But it's easy to reproduce the alias problem by
> this simple testcase (w/o error handler) on omap2430 with VIPT cache:

Your program doesn't do anything to identify any problem.  You don't
even say _what_ problem you see with this program.

If you have a specific case which fails, please show the problem, please
describe exactly the behaviour that you see, and what you expect to see.

flush_dcache_page does too much?

[anfei]

On Mon, Jan 18, 2010 at 02:00:05PM +0000, Russell King - ARM Linux wrote:
> On Mon, Jan 18, 2010 at 09:54:31PM +0800, anfei wrote:
> > Do you mean this implementation can ensure the coherence between write
> > and shared mmapings?  But it's easy to reproduce the alias problem by
> > this simple testcase (w/o error handler) on omap2430 with VIPT cache:
> 
> Your program doesn't do anything to identify any problem.  You don't
> even say _what_ problem you see with this program.
> 
Sorry for that.

> If you have a specific case which fails, please show the problem, please
> describe exactly the behaviour that you see, and what you expect to see.

The steps as these:
$ dd if=/dev/zero of=abc bs=4k count=1
$ ./a.out		# this program
$ xxd abc | head -1	# check the result

I want to see it's always 0x11111111 0x77777777 at the beginning of file
"abc",  but the result is not (the log is not at hand currently), but I
remember it like this after several runs:

0x11111111 0x77777777
0x44444444 0x77777777
0x11111111 0x77777777
0x44444444 0x77777777
0x44444444 0x77777777

If I add munmap()/msync() before write(), I can see it always as
expected (0x11111111 0x77777777).

flush_dcache_page does too much?

[Russell King - ARM Linux]

On Mon, Jan 18, 2010 at 10:15:30PM +0800, anfei wrote:
> On Mon, Jan 18, 2010 at 02:00:05PM +0000, Russell King - ARM Linux wrote:
> > On Mon, Jan 18, 2010 at 09:54:31PM +0800, anfei wrote:
> > > Do you mean this implementation can ensure the coherence between write
> > > and shared mmapings?  But it's easy to reproduce the alias problem by
> > > this simple testcase (w/o error handler) on omap2430 with VIPT cache:
> > 
> > Your program doesn't do anything to identify any problem.  You don't
> > even say _what_ problem you see with this program.
> > 
> Sorry for that.
> 
> > If you have a specific case which fails, please show the problem, please
> > describe exactly the behaviour that you see, and what you expect to see.

Are you using a write allocate cache?

flush_dcache_page does too much?

[anfei]

On Mon, Jan 18, 2010 at 02:44:18PM +0000, Russell King - ARM Linux wrote:
> On Mon, Jan 18, 2010 at 10:15:30PM +0800, anfei wrote:
> > On Mon, Jan 18, 2010 at 02:00:05PM +0000, Russell King - ARM Linux wrote:
> > > On Mon, Jan 18, 2010 at 09:54:31PM +0800, anfei wrote:
> > > > Do you mean this implementation can ensure the coherence between write
> > > > and shared mmapings?  But it's easy to reproduce the alias problem by
> > > > this simple testcase (w/o error handler) on omap2430 with VIPT cache:
> > > 
> > > Your program doesn't do anything to identify any problem.  You don't
> > > even say _what_ problem you see with this program.
> > > 
> > Sorry for that.
> > 
> > > If you have a specific case which fails, please show the problem, please
> > > describe exactly the behaviour that you see, and what you expect to see.
> 
> Are you using a write allocate cache?

I'm not sure, I can get more information tomorrow.

flush_dcache_page does too much?

[anfei]

On Mon, Jan 18, 2010 at 02:44:18PM +0000, Russell King - ARM Linux wrote:
> On Mon, Jan 18, 2010 at 10:15:30PM +0800, anfei wrote:
> > On Mon, Jan 18, 2010 at 02:00:05PM +0000, Russell King - ARM Linux wrote:
> > > On Mon, Jan 18, 2010 at 09:54:31PM +0800, anfei wrote:
> > > > Do you mean this implementation can ensure the coherence between write
> > > > and shared mmapings?  But it's easy to reproduce the alias problem by
> > > > this simple testcase (w/o error handler) on omap2430 with VIPT cache:
> > > 
> > > Your program doesn't do anything to identify any problem.  You don't
> > > even say _what_ problem you see with this program.
> > > 
> > Sorry for that.
> > 
> > > If you have a specific case which fails, please show the problem, please
> > > describe exactly the behaviour that you see, and what you expect to see.
> 
> Are you using a write allocate cache?

I guess not, because this line is neccessary to reproduce the issue:
	tmp = *(addr+0);
If it's write allocate, this line may not be neccessary, since it's just
a read (and cache the data).

flush_dcache_page does too much?

[Russell King - ARM Linux]

On Mon, Jan 18, 2010 at 10:57:31PM +0800, anfei wrote:
> On Mon, Jan 18, 2010 at 02:44:18PM +0000, Russell King - ARM Linux wrote:
> > On Mon, Jan 18, 2010 at 10:15:30PM +0800, anfei wrote:
> > > On Mon, Jan 18, 2010 at 02:00:05PM +0000, Russell King - ARM Linux wrote:
> > > > On Mon, Jan 18, 2010 at 09:54:31PM +0800, anfei wrote:
> > > > > Do you mean this implementation can ensure the coherence between write
> > > > > and shared mmapings?  But it's easy to reproduce the alias problem by
> > > > > this simple testcase (w/o error handler) on omap2430 with VIPT cache:
> > > > 
> > > > Your program doesn't do anything to identify any problem.  You don't
> > > > even say _what_ problem you see with this program.
> > > > 
> > > Sorry for that.
> > > 
> > > > If you have a specific case which fails, please show the problem, please
> > > > describe exactly the behaviour that you see, and what you expect to see.
> > 
> > Are you using a write allocate cache?
> 
> I guess not, because this line is neccessary to reproduce the issue:
> 	tmp = *(addr+0);
> If it's write allocate, this line may not be neccessary, since it's just
> a read (and cache the data).

It makes no sense then - without write allocate, writes will go straight
through to the underlying page, bypassing the cache.

Sorry, no idea.

flush_dcache_page does too much?

[anfei]

On Mon, Jan 18, 2010 at 03:01:52PM +0000, Russell King - ARM Linux wrote:
> On Mon, Jan 18, 2010 at 10:57:31PM +0800, anfei wrote:
> > On Mon, Jan 18, 2010 at 02:44:18PM +0000, Russell King - ARM Linux wrote:
> > > On Mon, Jan 18, 2010 at 10:15:30PM +0800, anfei wrote:
> > > > On Mon, Jan 18, 2010 at 02:00:05PM +0000, Russell King - ARM Linux wrote:
> > > > > On Mon, Jan 18, 2010 at 09:54:31PM +0800, anfei wrote:
> > > > > > Do you mean this implementation can ensure the coherence between write
> > > > > > and shared mmapings?  But it's easy to reproduce the alias problem by
> > > > > > this simple testcase (w/o error handler) on omap2430 with VIPT cache:
> > > > > 
> > > > > Your program doesn't do anything to identify any problem.  You don't
> > > > > even say _what_ problem you see with this program.
> > > > > 
> > > > Sorry for that.
> > > > 
> > > > > If you have a specific case which fails, please show the problem, please
> > > > > describe exactly the behaviour that you see, and what you expect to see.
> > > 
> > > Are you using a write allocate cache?
> > 
> > I guess not, because this line is neccessary to reproduce the issue:
> > 	tmp = *(addr+0);
> > If it's write allocate, this line may not be neccessary, since it's just
> > a read (and cache the data).
> 
> It makes no sense then - without write allocate, writes will go straight
> through to the underlying page, bypassing the cache.
> 
Because of the read, the write is cache hitted too even on read allocate:
       *(addr+0) = 0x44444444;	<- bypass the cache
       tmp = *(addr+0);		<- read allocate
       *(addr+1) = 0x77777777;	<- same cacheline, cache hitted

So the two write values are cached, then the sequence in sys_write
cannot guarantee the coherence:
	kmap_atomic(page);
	copy to page;
	kunmap_atomic(page);
	flush_dcache_page(page);

It should call flush_dcache_page()@the beginning too in order to
flush the shared mapping.  Actually, I think it's better to split this
function into two, such as:
	flush_dcache_user_page(page);
	copy to page;
	kunmap_atomic(page);
	flush_dcache_kern_page(page);

And this patch seems to fix it, any other fs doesn't call it need to add
that too.


diff --git a/mm/filemap.c b/mm/filemap.c
index 96ac6b0..07056fb 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -2196,6 +2196,9 @@ again:
 		if (unlikely(status))
 			break;
 
+		if (mapping_writably_mapped(mapping))
+			flush_dcache_page(page);
+
 		pagefault_disable();
 		copied = iov_iter_copy_from_user_atomic(page, i, offset, bytes);
 		pagefault_enable();

flush_dcache_page does too much?

[anfei]

Hi Russell,

On Tue, Jan 19, 2010 at 08:16:36AM +0800, anfei wrote:
> On Mon, Jan 18, 2010 at 03:01:52PM +0000, Russell King - ARM Linux wrote:
> > On Mon, Jan 18, 2010 at 10:57:31PM +0800, anfei wrote:
> > > On Mon, Jan 18, 2010 at 02:44:18PM +0000, Russell King - ARM Linux wrote:
> > > > On Mon, Jan 18, 2010 at 10:15:30PM +0800, anfei wrote:
> > > > > On Mon, Jan 18, 2010 at 02:00:05PM +0000, Russell King - ARM Linux wrote:
> > > > > > On Mon, Jan 18, 2010 at 09:54:31PM +0800, anfei wrote:
> > > > > > > Do you mean this implementation can ensure the coherence between write
> > > > > > > and shared mmapings?  But it's easy to reproduce the alias problem by
> > > > > > > this simple testcase (w/o error handler) on omap2430 with VIPT cache:
> > > > > > 
> > > > > > Your program doesn't do anything to identify any problem.  You don't
> > > > > > even say _what_ problem you see with this program.
> > > > > > 
> > > > > Sorry for that.
> > > > > 
> > > > > > If you have a specific case which fails, please show the problem, please
> > > > > > describe exactly the behaviour that you see, and what you expect to see.
> > > > 
> > > > Are you using a write allocate cache?
> > > 
> > > I guess not, because this line is neccessary to reproduce the issue:
> > > 	tmp = *(addr+0);
> > > If it's write allocate, this line may not be neccessary, since it's just
> > > a read (and cache the data).
> > 
> > It makes no sense then - without write allocate, writes will go straight
> > through to the underlying page, bypassing the cache.
> > 
> Because of the read, the write is cache hitted too even on read allocate:
>        *(addr+0) = 0x44444444;	<- bypass the cache
>        tmp = *(addr+0);		<- read allocate
>        *(addr+1) = 0x77777777;	<- same cacheline, cache hitted
> 
> So the two write values are cached, then the sequence in sys_write
> cannot guarantee the coherence:
> 	kmap_atomic(page);
> 	copy to page;
> 	kunmap_atomic(page);
> 	flush_dcache_page(page);
> 
> It should call flush_dcache_page() at the beginning too in order to
> flush the shared mapping.  Actually, I think it's better to split this
> function into two, such as:
> 	flush_dcache_user_page(page);
> 	copy to page;
> 	kunmap_atomic(page);
> 	flush_dcache_kern_page(page);
> 
> And this patch seems to fix it, any other fs doesn't call it need to add
> that too.
> 
Do you think this is a bug and I can send the patch, or it's the problem
of the test case?

Thanks,
Anfei.
> 
> diff --git a/mm/filemap.c b/mm/filemap.c
> index 96ac6b0..07056fb 100644
> --- a/mm/filemap.c
> +++ b/mm/filemap.c
> @@ -2196,6 +2196,9 @@ again:
>  		if (unlikely(status))
>  			break;
>  
> +		if (mapping_writably_mapped(mapping))
> +			flush_dcache_page(page);
> +
>  		pagefault_disable();
>  		copied = iov_iter_copy_from_user_atomic(page, i, offset, bytes);
>  		pagefault_enable();

flush_dcache_page does too much?

[Russell King - ARM Linux]

On Tue, Jan 19, 2010 at 09:05:47PM +0800, anfei wrote:
> Do you think this is a bug and I can send the patch, or it's the problem
> of the test case?

Yes, it's a bug, but it needs to be reported elsewhere; neither this
list nor I can sanction patches to generic code.

It needs to be reported to the linux-kernel and linux-mm mailing lists.

flush_dcache_page does too much?

[Jamie Lokier]

Russell King - ARM Linux wrote:
> On Tue, Jan 19, 2010 at 09:05:47PM +0800, anfei wrote:
> > Do you think this is a bug and I can send the patch, or it's the problem
> > of the test case?
> 
> Yes, it's a bug, but it needs to be reported elsewhere; neither this
> list nor I can sanction patches to generic code.

It's not necessarily a bug if the appropriate msync() call fixes the
test.  Linux doesn't guarantee coherence between mappings and
read/write without msync() on some architectures, but I haven't seen
an official statement of whether it *should* or not in this type of
case.

I have looked at this in some detail a few years ago.  When I looked
at MIPS and some other architecture code, I wasn't sure it was correct
even with msync() calls everywhere.  It looked distinctly undertested.

The Linux msync(2) man page says:

       msync()  flushes  changes  made  to the in-core copy of a file that was
       mapped into memory using mmap(2) back to disk.   Without  use  of  this
       call  there  is  no guarantee that changes are written back before mun?
       map(2) is called.  To be more precise, the part of the file that corre?
       sponds  to the memory area starting at addr and having length length is
       updated.

It's not a great description.  MS_INVALIDATE talks about invalidating
"other mappings", not this one, in the Linux man page.

But on other OSes (SunOS, HP-UX for example), MS_INVALIDATE
invalidates _this_ mapping so that it sees the latest data written by
write() to the file.

In other words, Linux is a complete mess in this area.  It happens to
work on some architectures, it's broken a little bit on others, broken
a lot on some, and despite reading a fair bit of code I'm still not
sure what the kernel is trying to promise.

Probably different people have different ideas about what's expected,
which is why MIPS and ARM kernels differ.

-- Jamie