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.026 / U:1.428 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site