Messages in this thread | | | Date | Thu, 21 Jan 2010 12:59:57 +0800 | Subject | Re: cache alias in mmap + write | From | anfei zhou <> |
| |
On Thu, Jan 21, 2010 at 9:10 AM, KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com> wrote: >> On Wed, Jan 20, 2010 at 06:10:11PM +0900, KOSAKI Motohiro wrote: >> > Hello, >> > >> > > 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(); >> > >> > I'm not sure ARM cache coherency model. but I guess correct patch is here. >> > >> > + 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(page); >> > >> > Why do we need to call flush_dcache_page() twice? >> > >> The latter flush_dcache_page is used to flush the kernel changes >> (iov_iter_copy_from_user_atomic), which makes the userspace to see the >> write, and the one I added is used to flush the userspace changes. >> And I think it's better to split this function into two: >> flush_dcache_user_page(page); >> kmap_atomic(page); >> write to page; >> kunmap_atomic(page); >> flush_dcache_kern_page(page); >> But currently there is no such API. > > Why can't we create new api? this your pseudo code looks very fine to me. > I will resend the patch, if this patch is acceptable, I will create another patch to introduce this new API. > > note: if you don't like to create new api. I can agree your current patch. > but I have three requests. > 1. Move flush_dcache_page() into iov_iter_copy_from_user_atomic(). > Your above explanation indicate it is real intention. plus, change > iov_iter_copy_from_user_atomic() fixes fuse too.
There is a check on mapping, that's not passed into iov_iter_copy_from_user_atomic, and this function is only called a few places, So I just add flush_dcache_page directly.
> 2. Add some commnet. almost developer only have x86 machine. so, arm > specific trick need additional explicit explanation. otherwise anybody > might break this code in the future. > 3. Resend the patch. original mail isn't good patch format. please consider > to reduce akpm suffer. > I will send it soon.
Thanks, Anfei. > > > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |