lkml.org 
[lkml]   [2010]   [Jan]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: cache alias in mmap + write
From
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/

\
 
 \ /
  Last update: 2010-01-21 06:03    [W:0.142 / U:0.236 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site