On Mon, Nov 24, 2014 at 11:46:46AM +0900, Changman Lee wrote:
> Hi Simon,
> Thanks for your explanation kindly.
>
> On Sun, Nov 23, 2014 at 11:08:54AM +0100, Simon Baatz wrote:
> > Hi Changman, Jaegeuk,
> >
> > On Thu, Nov 20, 2014 at 05:47:29PM +0900, Changman Lee wrote:
> > > On Wed, Nov 19, 2014 at 10:45:33PM -0800, Jaegeuk Kim wrote:
> > > > On Thu, Nov 20, 2014 at 03:04:10PM +0900, Changman Lee wrote:
> > > > > Hi Jaegeuk,
> > > > >
> > > > > We should call flush_dcache_page before kunmap because the purpose of
> > > > > the cache flush is to address aliasing problem related to virtual
> > > > > address.
> > > >
> > > > Oh, I just followed zero_user_segments below.
> > > >
> > > > static inline void zero_user_segments(struct page *page,
> > > > unsigned start1, unsigned end1,
> > > > unsigned start2, unsigned end2)
> > > > {
> > > > void *kaddr = kmap_atomic(page);
> > > >
> > > > BUG_ON(end1 > PAGE_SIZE || end2 > PAGE_SIZE);
> > > >
> > > > if (end1 > start1)
> > > > memset(kaddr + start1, 0, end1 - start1);
> > > >
> > > > if (end2 > start2)
> > > > memset(kaddr + start2, 0, end2 - start2);
> > > >
> > > > kunmap_atomic(kaddr);
> > > > flush_dcache_page(page);
> > > > }
> > > >
> > > > Is this a wrong reference? Or, a bug?
> > > >
> > >
> > > Well.. Data in cache only have to be flushed until before other users
> > > read the data.
> > > If so, it's not a bug.
> > >
> >
> > Yes, it is not a bug, since flush_dcache_page() needs to be able to
> > deal with non-kmapped pages. However, this may create overhead in
> > some situations.
> >
>
> Previously, I was vague but I thought that it should be different
> according to vaddr exists or not. So I told jaegeuk that it should
> be better to change an order between flush_dache_page and kunmap.
> But actually, it doesn't matter the order between them except
> the situation you said.
> Could you explain the situation that makes overhead by flushing after kummap.
> I can't imagine it by just seeing flush_dcache_page code.
>
> > According to documentation (see Documentation/cachetlb.txt), this is
> > a use for flush_kernel_dcache_page(), since the page has been
> > modified by the kernel only. In contrast to flush_dcache_page(),
> > this function must be called before kunmap().
> >
> > flush_kernel_dcache_page() does not need to flush the user space
> > aliases. Additionally, at least on ARM, it does not flush at all
> > when called within kmap_atomic()/kunmap_atomic(), when
> > kunmap_atomic() is going to flush the page anyway. (I know that
> > almost no one uses flush_kernel_dcache_page() (probably because
> > almost no one knows when to use which of the two functions), but it
> > may save a few cache flushes on architectures which are affected by
> > aliasing)
> >
> >
> > > > Anyway I modified as below.
> > > >
> > > > Thanks,
> > > >
> > > > >From 7cb7b27c8cd2efc8a31d79239bef5b41c6e79216 Mon Sep 17 00:00:00 2001
> > > > From: Jaegeuk Kim <jaeg...@kernel.org>
> > > > Date: Tue, 18 Nov 2014 10:50:21 -0800
> > > > Subject: [PATCH] f2fs: call flush_dcache_page when the page was updated
> > > >
> > > > Whenever f2fs updates mapped pages, it needs to call flush_dcache_page.
> > > >
> > > > Signed-off-by: Jaegeuk Kim <jaeg...@kernel.org>
> > > > ---
> > > > fs/f2fs/dir.c | 7 ++++++-
> > > > fs/f2fs/inline.c | 2 ++
> > > > 2 files changed, 8 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
> > > > index 5a49995..fabf4ee 100644
> > > > --- a/fs/f2fs/dir.c
> > > > +++ b/fs/f2fs/dir.c
> > > > @@ -287,8 +287,10 @@ void f2fs_set_link(struct inode *dir, struct
> > > > f2fs_dir_entry *de,
> > > > f2fs_wait_on_page_writeback(page, type);
> > > > de->ino = cpu_to_le32(inode->i_ino);
> > > > set_de_type(de, inode);
> > > > - if (!f2fs_has_inline_dentry(dir))
> > > > + if (!f2fs_has_inline_dentry(dir)) {
> > > > + flush_dcache_page(page);
> > > > kunmap(page);
> > > > + }
> >
> > Is this a page that may be mapped into user space? (I may be
> > completely wrong here, since I have no idea how this code works. But
> > it looks like as if the answer is "no" ;-) ).
> >
> > It is not necessary to flush pages that cannot be seen by user space
> > (see also the NOTE in the documentation of flush_dcache_page() in
> > cachetlb.txt). Thus, if you know that a page will not be mapped into
> > user space, please don't create the overhead of flushing it.
> >
>
> In the case of dentry unlike inline data, this is not mapped to user space,
> so dcache flush
> makes overhead. Do you mean that?
>
> Best regard,
> Changman
Hi Changman,
This would be ok though.
>From 5c1c46d31452a59a37c96f6dcae600776a8002e3 Mon Sep 17 00:00:00 2001
From: Jaegeuk Kim <jaeg...@kernel.org>
Date: Tue, 18 Nov 2014 10:50:21 -0800
Subject: [PATCH v2] f2fs: call flush_dcache_page when the page was updated
Change log from v1:
o remove flush_dcache_page calls for dentry pages.
Whenever f2fs updates mapped pages, it needs to call flush_dcache_page.
Signed-off-by: Jaegeuk Kim <jaeg...@kernel.org>
---
fs/f2fs/inline.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/fs/f2fs/inline.c b/fs/f2fs/inline.c
index f26fb87..914b6d3 100644
--- a/fs/f2fs/inline.c
+++ b/fs/f2fs/inline.c
@@ -106,6 +106,7 @@ int f2fs_convert_inline_page(struct dnode_of_data *dn,
struct page *page)
src_addr = inline_data_addr(dn->inode_page);
dst_addr = kmap_atomic(page);
memcpy(dst_addr, src_addr, MAX_INLINE_DATA);
+ flush_dcache_page(page);
kunmap_atomic(dst_addr);
SetPageUptodate(page);
no_update:
--
2.1.1
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
