[PATCH] Fixs to work on ARM and PARIC platforms.

--- fuse-2.6.0-pre2/kernel/dev.c.orig	2006-04-01 00:40:28.000000000 -0800
+++ fuse-2.6.0-pre2/kernel/dev.c	2006-04-01 00:40:43.000000000 -0800
@@ -522,6 +522,7 @@
 {
 	unsigned ncpy = min(*size, cs->len);
 	if (val) {
+		flush_cache_all();
 		if (cs->write)
 			memcpy(cs->buf, *val, ncpy);
 		else

--- fuse-2.6.0-pre2/lib/fuse.c.orig	2006-04-01 00:54:32.000000000 -0800
+++ fuse-2.6.0-pre2/lib/fuse.c	2006-04-01 00:47:48.000000000 -0800
@@ -2068,6 +2068,7 @@
     }

     mutex_init(&f->lock);
+    pthread_rwlock_init(&f->tree_lock, PTHREAD_RWLOCK_PREFER_READER_NP);
     memcpy(&f->op, op, op_size);
     f->compat = compat;

Re: [parisc-linux] [PATCH] Fixs to work on ARM and PARIC platforms.

On Sat, 2006-04-01 at 01:22 -0800, Ryan Bradetich wrote:
> 2. I had to add an additional cache flush in the fuse_copy_do
> function.  On both ARM and PARISC, the data was corrupted unless cache
> is flushed first.  Unfortunately I hit it with a very big hammer
> (flush_cache_all) ... I am hoping someone more familiar with the
> caches will suggest a better flush function to use.  I tried to use
> the flush_kernel_dcache_page and flush_dcache_page functions without
> any luck :( 

You're right that a flush_cache_all() isn't going to be acceptable.
However, the patch is to a non-existent file, so I don't really see how
we can help unless you reduce it to a test case like the previous
copytest failures.

James

-------------------------------------------------------
This SF.Net email is sponsored by xPML, a groundbreaking scripting language
that extends applications into web and mobile media. Attend the live webcast
and join the prime developer group breaking into this new coding territory!
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=110944&bid=241720&dat=121642

Re: [parisc-linux] [PATCH] Fixs to work on ARM and PARIC platforms.

On Sat, 2006-04-01 at 01:22 -0800, Ryan Bradetich wrote:
> --- fuse-2.6.0-pre2/kernel/dev.c.orig   2006-04-01 00:40:28.000000000
> -0800
> +++ fuse-2.6.0-pre2/kernel/dev.c        2006-04-01 00:40:43.000000000
> -0800

Is this path just wrong and you meant fs/fuse/dev.c?

In which case, the placement of the flush and the fact that
flush_dcache_page() doesn't bring coherency indicates one of the input
pages is incoherent at the user level.  So it looks like you have an
anonymous page you haven't called get_user_pages() on .... and by the
way, you have checked that you're actually using the kernel with the
parisc implementations of flush_kernel_dcache_page() and
flush_anon_page()?

James

-------------------------------------------------------
This SF.Net email is sponsored by xPML, a groundbreaking scripting language
that extends applications into web and mobile media. Attend the live webcast
and join the prime developer group breaking into this new coding territory!
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=110944&bid=241720&dat=121642

Re: [PATCH] Fixs to work on ARM and PARIC platforms.

> 1.  The rwlock tree_lock  in struct fuse was not properly initialized using
> pthread_rwlock_init().    This normally works on other archs (besides
> PARISC) since the lock can be initialized to 0 values, this is not true on
> the PARISC platform ... these locks must be initialized properly to work on
> PARISC.

Good find, thanks.

The initialization should actually look like this:

  pthread_rwlock_init(&f->tree_lock, NULL);

I can't imagine how your version managed to work...

> 2. I had to add an additional cache flush in the fuse_copy_do function.  On
> both ARM and PARISC, the data was corrupted unless cache is flushed first.
> Unfortunately I hit it with a very big hammer (flush_cache_all) ... I am
> hoping someone more familiar with the caches will suggest a better flush
> function to use.  I tried to use the flush_kernel_dcache_page and
> flush_dcache_page functions without any luck :(

You should try using the parisc patchset from:

   http://cvs.parisc-linux.org/download/linux-2.6/

and replace flush_dcache_page() with flush_kernel_dcache_page() in
FUSE source.

Thanks,
Miklos

-------------------------------------------------------
This SF.Net email is sponsored by xPML, a groundbreaking scripting language
that extends applications into web and mobile media. Attend the live webcast
and join the prime developer group breaking into this new coding territory!
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=110944&bid=241720&dat=121642

Re: Re: [parisc-linux] [PATCH] Fixs to work on ARM and PARIC platforms.

> On Sat, 2006-04-01 at 01:22 -0800, Ryan Bradetich wrote:
> > --- fuse-2.6.0-pre2/kernel/dev.c.orig   2006-04-01 00:40:28.000000000
> > -0800
> > +++ fuse-2.6.0-pre2/kernel/dev.c        2006-04-01 00:40:43.000000000
> > -0800
> 
> Is this path just wrong and you meant fs/fuse/dev.c?

Yeah.  It's the out-of-tree version, which closely resembles the
in-tree one.

> In which case, the placement of the flush and the fact that
> flush_dcache_page() doesn't bring coherency indicates one of the input
> pages is incoherent at the user level.  So it looks like you have an
> anonymous page you haven't called get_user_pages() on .... and by the
> way, you have checked that you're actually using the kernel with the
> parisc implementations of flush_kernel_dcache_page() and
> flush_anon_page()?

BTW, these seem to be needed for ARM too.  Do you have any idea who I
should bug about this?  The MAINTAINERS file is far from clear on this
point.

Thanks,
Miklos

-------------------------------------------------------
This SF.Net email is sponsored by xPML, a groundbreaking scripting language
that extends applications into web and mobile media. Attend the live webcast
and join the prime developer group breaking into this new coding territory!
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=110944&bid=241720&dat=121642

Re: Re: [parisc-linux] [PATCH] Fixs to work on ARM and PARIC platforms.

On Sat, 2006-04-01 at 18:51 +0200, Miklos Szeredi wrote:
> BTW, these seem to be needed for ARM too.  Do you have any idea who I
> should bug about this?  The MAINTAINERS file is far from clear on this
> point.

Erm, not just arm.  The anon page problem looks like it might afflict
other incoherent architectures (like sparc and ppc); it just depends
whether their flush_dcache_page() accidentally flushes anonymous pages
or not.

For arm, it does look like the arm list won't accept postings, so I'd
send patches to Russell King <rmk@...>

James

-------------------------------------------------------
This SF.Net email is sponsored by xPML, a groundbreaking scripting language
that extends applications into web and mobile media. Attend the live webcast
and join the prime developer group breaking into this new coding territory!
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=110944&bid=241720&dat=121642

Re: Re: [parisc-linux] [PATCH] Fixs to work on ARM and PARIC platforms.

> On Sat, 2006-04-01 at 18:51 +0200, Miklos Szeredi wrote:
> > BTW, these seem to be needed for ARM too.  Do you have any idea who I
> > should bug about this?  The MAINTAINERS file is far from clear on this
> > point.
> 
> Erm, not just arm.  The anon page problem looks like it might afflict
> other incoherent architectures (like sparc and ppc); it just depends
> whether their flush_dcache_page() accidentally flushes anonymous pages
> or not.

Hmm, seems like they do.

I'm confused.  What was the exact reason for having a separate
function for anon pages?  Wouldn't unifying the functionality of
flush_dcache_page() and flush_anon_page() make more sense?

Otherwise sparc and ppc would have to check PageAnon() from both
flush_dcache_page() and flush_anon_page() and call a common function,
which seems to just complicate things without any gain.

Miklos

-------------------------------------------------------
This SF.Net email is sponsored by xPML, a groundbreaking scripting language
that extends applications into web and mobile media. Attend the live webcast
and join the prime developer group breaking into this new coding territory!
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=110944&bid=241720&dat=121642

Re: Re: [parisc-linux] [PATCH] Fixs to work on ARM and PARIC platforms.

On Sun, 2006-04-02 at 10:45 +0200, Miklos Szeredi wrote:
> I'm confused.  What was the exact reason for having a separate
> function for anon pages?  Wouldn't unifying the functionality of
> flush_dcache_page() and flush_anon_page() make more sense?

No because flush_dcache_page() is designed for filesystems, which should
never see anonymous pages.

> Otherwise sparc and ppc would have to check PageAnon() from both
> flush_dcache_page() and flush_anon_page() and call a common function,
> which seems to just complicate things without any gain.

I don't think what you propose is desirable or possible:
flush_dcache_pages() is designed not to have to flush them (it can't
actually because it can't find them properly) but it may flush them by
accident if they get swept up into the implementation.  Sparc and PPC
are CAM flushing implementations, so I suspect the cache just needs to
be asked to flush a single physical address and it will find all the
virtual aliases and flush them (including the anonymous pages).

James

-------------------------------------------------------
This SF.Net email is sponsored by xPML, a groundbreaking scripting language
that extends applications into web and mobile media. Attend the live webcast
and join the prime developer group breaking into this new coding territory!
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=110944&bid=241720&dat=121642

Re: Re: [parisc-linux] [PATCH] Fixs to work on ARM and PARIC platforms.

> > Otherwise sparc and ppc would have to check PageAnon() from both
> > flush_dcache_page() and flush_anon_page() and call a common function,
> > which seems to just complicate things without any gain.
> 
> I don't think what you propose is desirable or possible:
> flush_dcache_pages() is designed not to have to flush them (it can't
> actually because it can't find them properly)

I see now, that flush_anon_page() also gets the virtual address.

Pardon my ignorance, but I'm still not getting the whole picture.

Current (with your API updates) get_user_pages() does the following on
PARISC:

  flush_anon_page():

    - If page is anonymous, the the user address is flushed which was
      passed to get_user_pages().

  flush_dcache_page():
     - The kernel address is flushed regardless whether the page is
       anonymous or not

     - If the page is file backed, then all user addresses refering to
       the page are flushed

Why this discrepancy between anonymous and file backed pages?
Wounldn't it be enough for file backed pages too to flush only one
user address?

Added to the mix are copy_to/from_user_page() which already seem to do
the above, and are used in combination with get_user_pages() which
results in multiple redundant cache flushes.  Not too clean, is it?

Thanks,
Miklos

-------------------------------------------------------
This SF.Net email is sponsored by xPML, a groundbreaking scripting language
that extends applications into web and mobile media. Attend the live webcast
and join the prime developer group breaking into this new coding territory!
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=110944&bid=241720&dat=121642

Re: Re: [parisc-linux] [PATCH] Fixs to work on ARM and PARIC platforms.

On Mon, 2006-04-03 at 15:21 +0200, Miklos Szeredi wrote:
>   flush_dcache_page():
>      - The kernel address is flushed regardless whether the page is
>        anonymous or not

Not quite ... if the page is file backed but has no user mappings, on
the page dirty bit will be set (the kernel view won't be flushed).

>      - If the page is file backed, then all user addresses refering to
>        the page are flushed

Yes, that's what it does.

> Why this discrepancy between anonymous and file backed pages?
> Wounldn't it be enough for file backed pages too to flush only one
> user address?

Not necessarily ... you're getting deep into how VIPT and VIVT caches
work.  VIVT and non-CAM based VIPT caches need every alias flushed
(well, that's not congruent, anyway ... we try to keep congruence in
parisc, but it's not always possible).  Usually a page is either file
backed or anonymous, so for an anonymous page, we wouldn't know the user
address to flush even if it were congruent.

> Added to the mix are copy_to/from_user_page() which already seem to do
> the above, and are used in combination with get_user_pages() which
> results in multiple redundant cache flushes.  Not too clean, is it?

I don't see that they do.  If flush_dcache_page() also does anon pages,
then the arch implementation of flush_anon_page() will be empty.  If it
doesn't, then the flush_anon_page() and flush_dcache_page() are mutually
exclusive, anyway (because they both check the anon flag).

James

-------------------------------------------------------
This SF.Net email is sponsored by xPML, a groundbreaking scripting language
that extends applications into web and mobile media. Attend the live webcast
and join the prime developer group breaking into this new coding territory!
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=110944&bid=241720&dat=121642