flush_kernel_dcache_page() not needed ?

flush_kernel_dcache_page() not needed ?

[Franck Bui-Huu]

Hi,

I noticed that there's currently (v2.6.23-rc4) no implementation
of this function even for mips CPUs that have dcache aliasing.

Could anybody explain why ?

thanks,
		Franck

Re: flush_kernel_dcache_page() not needed ?

[Markus Gothe]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

Ralf added this a while ago:

"arch/mips/mm/cache.c:void __flush_dcache_page(struct page *page)"

//Markus

Franck Bui-Huu wrote:
> Hi,
>
> I noticed that there's currently (v2.6.23-rc4) no implementation of
> this function even for mips CPUs that have dcache aliasing.
>
> Could anybody explain why ?
>
> thanks, Franck
>


- --
_______________________________________

Mr Markus Gothe
Software Engineer

Phone: +46 (0)13 21 81 20 (ext. 1046)
Fax: +46 (0)13 21 21 15
Mobile: +46 (0)73 718 72 80
Diskettgatan 11, SE-583 35 Linköping, Sweden
www.27m.com
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.6 (GNU/Linux)

iD8DBQFG2Cqe6I0XmJx2NrwRCNouAKDTex/fiBt4qJgX15tW4TzwQct7DQCgi6sE
GNFJZe/vjsW3z/FQw01yxx0=
=RFVE
-----END PGP SIGNATURE-----

Re: flush_kernel_dcache_page() not needed ?

[Markus Gothe]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

N/M, I was thinking of flush_Anon_Page
Franck Bui-Huu wrote:
> Hi,
>
> I noticed that there's currently (v2.6.23-rc4) no implementation of
> this function even for mips CPUs that have dcache aliasing.
>
> Could anybody explain why ?
>
> thanks, Franck
>


- --
_______________________________________

Mr Markus Gothe
Software Engineer

Phone: +46 (0)13 21 81 20 (ext. 1046)
Fax: +46 (0)13 21 21 15
Mobile: +46 (0)73 718 72 80
Diskettgatan 11, SE-583 35 Linköping, Sweden
www.27m.com
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.6 (GNU/Linux)

iD8DBQFG2CuW6I0XmJx2NrwRCOXHAJ4gs+pw2Ra3X27MFgpjx9V3B7WbSACgviej
7K8mydrGQPHKZWikCsGopVM=
=nOcy
-----END PGP SIGNATURE-----

Re: flush_kernel_dcache_page() not needed ?

[Franck Bui-Huu]

Hi,

Markus Gothe wrote:
> Ralf added this a while ago:
> 
> "arch/mips/mm/cache.c:void __flush_dcache_page(struct page *page)"

I was asking for flush_kernel_dcache_page()...

Please note the word "kernel" in the name of the function.

		Franck

Re: flush_kernel_dcache_page() not needed ?

[Ralf Baechle]

On Fri, Aug 31, 2007 at 04:50:07PM +0200, Markus Gothe wrote:

> Ralf added this a while ago:
> 
> "arch/mips/mm/cache.c:void __flush_dcache_page(struct page *page)"

__flush_dcache_page is just the part of flush_dcache_page which does
the real work.and has nothing to do with flush_kernel_dcache_page.

  Ralf

Re: flush_kernel_dcache_page() not needed ?

[Markus Gothe]

[-- Attachment #1: Type: text/plain, Size: 688 bytes --]

I thought I mailed out 'N/M' shortly after the first mail ;)

On 01 Sep, 2007, at 01:42 , Ralf Baechle wrote:

> On Fri, Aug 31, 2007 at 04:50:07PM +0200, Markus Gothe wrote:
>
>> Ralf added this a while ago:
>>
>> "arch/mips/mm/cache.c:void __flush_dcache_page(struct page *page)"
>
> __flush_dcache_page is just the part of flush_dcache_page which does
> the real work.and has nothing to do with flush_kernel_dcache_page.
>
>   Ralf

_______________________________________

Mr Markus Gothe
Software Engineer

Phone: +46 (0)13 21 81 20 (ext. 1046)
Fax: +46 (0)13 21 21 15
Mobile: +46 (0)73 718 72 80
Diskettgatan 11, SE-583 35 Linköping, Sweden
www.27m.com



[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 186 bytes --]

Re: flush_kernel_dcache_page() not needed ?

[Atsushi Nemoto]

On Fri, 31 Aug 2007 14:25:03 +0200, Franck Bui-Huu <vagabon.xyz@gmail.com> wrote:
> I noticed that there's currently (v2.6.23-rc4) no implementation
> of this function even for mips CPUs that have dcache aliasing.
> 
> Could anybody explain why ?

Maybe because the API was not called by anybody until 2.6.23-rc1 :)

Now copy_strings() calls this and I'm wondering we should implement or
not.  It seems the kernel works fine for me without the API...

Do you have any problem due to luck of the API?

---
Atsushi Nemoto

Re: flush_kernel_dcache_page() not needed ?

[Franck Bui-Huu]

Atsushi Nemoto wrote:
> On Fri, 31 Aug 2007 14:25:03 +0200, Franck Bui-Huu <vagabon.xyz@gmail.com> wrote:
>> I noticed that there's currently (v2.6.23-rc4) no implementation
>> of this function even for mips CPUs that have dcache aliasing.
>>
>> Could anybody explain why ?
> 
> Maybe because the API was not called by anybody until 2.6.23-rc1 :)
> 

But this function has been introduced since commit
5a3a5a98b6422d05c39eaa32c8b3f83840c7b768 ([PATCH] Add
flush_kernel_dcache_page() API) which had been merged during 2.6.16
merge window. So it's more than one year now...

Basically it gives a default implementation for all architectures. The
problem here is that this implementation may be boggus for
architectures that have dcache aliasing issue.

The sad thing is that the kernel will silently compile this default
implementation. At least, it could have showed a big fat warning
during the building process.

> Now copy_strings() calls this and I'm wondering we should implement or
> not.  It seems the kernel works fine for me without the API...
 
Do you use a cpu with dcache aliasing issue ?

> Do you have any problem due to luck of the API?

No, but looking at copy_strings(), I think we can have some trouble.

BTW, do you recall flush_anon_page() and fuse bug ? It seems the same
here...

		Franck

Re: flush_kernel_dcache_page() not needed ?

[Atsushi Nemoto]

On Mon, 03 Sep 2007 17:36:16 +0200, Franck Bui-Huu <vagabon.xyz@gmail.com> wrote:
> > Now copy_strings() calls this and I'm wondering we should implement or
> > not.  It seems the kernel works fine for me without the API...
>  
> Do you use a cpu with dcache aliasing issue ?

Yes.  4 way 32KB dcache.

> > Do you have any problem due to luck of the API?
> 
> No, but looking at copy_strings(), I think we can have some trouble.
> 
> BTW, do you recall flush_anon_page() and fuse bug ? It seems the same
> here...

Indeed.  But copy_strings() is not rare case (called on each execve),
so there might be some constraints which make us free from the
aliasing problem.  I'll look at it further, but any testcase are
welcome.

---
Atsushi Nemoto

Re: flush_kernel_dcache_page() not needed ?

[Franck Bui-Huu]

Atsushi Nemoto wrote:
> Indeed.  But copy_strings() is not rare case (called on each execve),
> so there might be some constraints which make us free from the
> aliasing problem.

The point is that this _generic_ function has been created so we need
to understand if MIPS architecture needs to implement it or not,
whatever its current usages. This was actually what I was trying to
understand with this thread.

Whatever the constraints, they don't seem to be intended at all since
flush_kernel_dcache_page() is called... So even if the current code is
working fine, it seems very fragile _if_ MIPS needs to implement this
cache helper.

> I'll look at it further, but any testcase are welcome.

One thing you might want to try is:

	$ echo 0 > /proc/sys/kernel/randomize_va_space

and see if your system still works fine. This command should avoid a
data cache flush when moving the stack around. See shift_arg_pages().

With this, maybe you could give this testcase a try:

	$ /bin/echo "`seq 10000`" > seq.txt

and see if seq.txt is correct. This command should pass to echo (not
the bash builtin one) a long argument that should fill your
dcache.

That said the execve syscall code is quite 'hairy' and it may not be
suprising that after this syscall the dcache has been completly
flushed and thus make the problem disappear.

		Franck

Re: flush_kernel_dcache_page() not needed ?

[Atsushi Nemoto]

On Tue, 04 Sep 2007 14:46:54 +0200, Franck Bui-Huu <vagabon.xyz@gmail.com> wrote:
> > Indeed.  But copy_strings() is not rare case (called on each execve),
> > so there might be some constraints which make us free from the
> > aliasing problem.
> 
> The point is that this _generic_ function has been created so we need
> to understand if MIPS architecture needs to implement it or not,
> whatever its current usages. This was actually what I was trying to
> understand with this thread.
> 
> Whatever the constraints, they don't seem to be intended at all since
> flush_kernel_dcache_page() is called... So even if the current code is
> working fine, it seems very fragile _if_ MIPS needs to implement this
> cache helper.

Yes, agreed.

> > I'll look at it further, but any testcase are welcome.

I found the reason.

do_execve()
  copy_strings()
    flush_kernel_dcache_page()
  search_binary_handler()
    load_elf_binary()
      setup_arg_pages()
        shift_arg_pages()
          move_page_tables()
            flush_cache_range()

And MIPS flush_cache_range() blasts whole dcache!  That's why empty
flush_kernel_dcache_page() was enough for now.  

Anyway, here is a patch.


Subject: [MIPS] Implement flush_kernel_dcache_page()

Signed-off-by: Atsushi Nemoto <anemo@mba.ocn.ne.jp>
---
diff --git a/include/asm-mips/cacheflush.h b/include/asm-mips/cacheflush.h
index 4933b49..82e734d 100644
--- a/include/asm-mips/cacheflush.h
+++ b/include/asm-mips/cacheflush.h
@@ -79,6 +79,13 @@ extern void (*flush_icache_all)(void);
 extern void (*local_flush_data_cache_page)(void * addr);
 extern void (*flush_data_cache_page)(unsigned long addr);
 
+#define ARCH_HAS_FLUSH_KERNEL_DCACHE_PAGE
+static inline void flush_kernel_dcache_page(struct page *page)
+{
+	if (cpu_has_dc_aliases)
+		flush_data_cache_page((unsigned long)page_address(page));
+}
+
 /*
  * This flag is used to indicate that the page pointed to by a pte
  * is dirty and requires cleaning before returning it to the user.

Re: flush_kernel_dcache_page() not needed ?

[Franck Bui-Huu]

Atsushi Nemoto wrote:
> On Tue, 04 Sep 2007 14:46:54 +0200, Franck Bui-Huu <vagabon.xyz@gmail.com> wrote:
>>> I'll look at it further, but any testcase are welcome.
> 
> I found the reason.
> 
> do_execve()
>   copy_strings()
>     flush_kernel_dcache_page()
>   search_binary_handler()
>     load_elf_binary()
>       setup_arg_pages()
>         shift_arg_pages()
>           move_page_tables()
>             flush_cache_range()
> 
> And MIPS flush_cache_range() blasts whole dcache!  That's why empty
> flush_kernel_dcache_page() was enough for now.  

Yeah, that what I think too and that's also why I was suggesting to
test a plain 2.6.23-rc5 with the randomize_va_space=0. Please see my
previous reply...

Could you give it a try ?

		Franck

Re: flush_kernel_dcache_page() not needed ?

[Atsushi Nemoto]

On Tue, 04 Sep 2007 16:05:10 +0200, Franck Bui-Huu <vagabon.xyz@gmail.com> wrote:
> Yeah, that what I think too and that's also why I was suggesting to
> test a plain 2.6.23-rc5 with the randomize_va_space=0. Please see my
> previous reply...
> 
> Could you give it a try ?

OK, I'll try it in a few days...
---
Atsushi Nemoto

Re: flush_kernel_dcache_page() not needed ?

[Franck Bui-Huu]

Atsushi Nemoto wrote:
> On Tue, 04 Sep 2007 16:05:10 +0200, Franck Bui-Huu <vagabon.xyz@gmail.com> wrote:
>> Yeah, that what I think too and that's also why I was suggesting to
>> test a plain 2.6.23-rc5 with the randomize_va_space=0. Please see my
>> previous reply...
>>
>> Could you give it a try ?
> 
> OK, I'll try it in a few days...

thank you, Atsushi.

		Franck

Re: flush_kernel_dcache_page() not needed ?

[Atsushi Nemoto]

On Tue, 04 Sep 2007 14:46:54 +0200, Franck Bui-Huu <vagabon.xyz@gmail.com> wrote:
> One thing you might want to try is:
> 
> 	$ echo 0 > /proc/sys/kernel/randomize_va_space
> 
> and see if your system still works fine. This command should avoid a
> data cache flush when moving the stack around. See shift_arg_pages().
> 
> With this, maybe you could give this testcase a try:
> 
> 	$ /bin/echo "`seq 10000`" > seq.txt
> 
> and see if seq.txt is correct. This command should pass to echo (not
> the bash builtin one) a long argument that should fill your
> dcache.

I tried this and everything worked fine with/without the
flush_kernel_dcache_page() ;)

> That said the execve syscall code is quite 'hairy' and it may not be
> suprising that after this syscall the dcache has been completly
> flushed and thus make the problem disappear.

Yes, there is an yet another path to "flush all dcache".

do_execve()
  copy_strings()
    flush_kernel_dcache_page()
  search_binary_handler()
    load_elf_binary()
      flush_old_exec()
        exec_mmap()
          mmput()
            exit_mmap()
              flush_cache_mm()
                r4k_blast_dcache()

Anyway, the implementation of flush_kernel_dcache_page() is very
simple so that we can believe it works correctly without any testcase.
Too optimistic? :)

---
Atsushi Nemoto

Re: flush_kernel_dcache_page() not needed ?

[Franck Bui-Huu]

Atsushi Nemoto wrote:
> On Tue, 04 Sep 2007 14:46:54 +0200, Franck Bui-Huu <vagabon.xyz@gmail.com> wrote:
>> One thing you might want to try is:
>>
>> 	$ echo 0 > /proc/sys/kernel/randomize_va_space
>>
>> and see if your system still works fine. This command should avoid a
>> data cache flush when moving the stack around. See shift_arg_pages().
>>
>> With this, maybe you could give this testcase a try:
>>
>> 	$ /bin/echo "`seq 10000`" > seq.txt
>>
>> and see if seq.txt is correct. This command should pass to echo (not
>> the bash builtin one) a long argument that should fill your
>> dcache.
> 
> I tried this and everything worked fine with/without the
> flush_kernel_dcache_page() ;)
> 
>> That said the execve syscall code is quite 'hairy' and it may not be
>> suprising that after this syscall the dcache has been completly
>> flushed and thus make the problem disappear.
> 
> Yes, there is an yet another path to "flush all dcache".
> 
> do_execve()
>   copy_strings()
>     flush_kernel_dcache_page()
>   search_binary_handler()
>     load_elf_binary()
>       flush_old_exec()
>         exec_mmap()
>           mmput()
>             exit_mmap()
>               flush_cache_mm()
>                 r4k_blast_dcache()
> 
> Anyway, the implementation of flush_kernel_dcache_page() is very
> simple so that we can believe it works correctly without any testcase.
> Too optimistic? :)

god only knows ;)

		Franck

Re: flush_kernel_dcache_page() not needed ?

[Franck Bui-Huu]

Atsushi Nemoto wrote:
> Yes, there is an yet another path to "flush all dcache".
> 
> do_execve()
>   copy_strings()
>     flush_kernel_dcache_page()
>   search_binary_handler()
>     load_elf_binary()
>       flush_old_exec()
>         exec_mmap()
>           mmput()
>             exit_mmap()
>               flush_cache_mm()
>                 r4k_blast_dcache()
> 

BTW, flush_cache_mm() flushes (write back + invalidate ) the whole
data cache unconditionnaly, but I'm wondering if it's really necessary
for cpus which don't have any cache aliasing issues. After all they're
equivalent to physical caches, aren't they ?

thanks,
		Franck

Re: flush_kernel_dcache_page() not needed ?

[Atsushi Nemoto]

On Fri, 14 Sep 2007 10:32:48 +0200, Franck Bui-Huu <vagabon.xyz@gmail.com> wrote:
> BTW, flush_cache_mm() flushes (write back + invalidate ) the whole
> data cache unconditionnaly, but I'm wondering if it's really necessary
> for cpus which don't have any cache aliasing issues. After all they're
> equivalent to physical caches, aren't they ?

r4k_flush_cache_mm() returns immediately if !cpu_has_dc_aliases.

---
Atsushi Nemoto

Re: flush_kernel_dcache_page() not needed ?

[Franck Bui-Huu]

Atsushi Nemoto wrote:
> On Fri, 14 Sep 2007 10:32:48 +0200, Franck Bui-Huu <vagabon.xyz@gmail.com> wrote:
>> BTW, flush_cache_mm() flushes (write back + invalidate ) the whole
>> data cache unconditionnaly, but I'm wondering if it's really necessary
>> for cpus which don't have any cache aliasing issues. After all they're
>> equivalent to physical caches, aren't they ?
> 
> r4k_flush_cache_mm() returns immediately if !cpu_has_dc_aliases.
> 

oops you're right, I was actually looking at local_r4k_flush_cache_mm().

thanks,
		Franck