Re: cache alias in mmap + write

[anfei zhou <anfei.zhou@gmail.com>]

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, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>