[RFC PATCH] vfs: wrap CONFIG_READ_ONLY_THP_FOR_FS-related code with an ifdef

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On kernels compiled without this option (which is currently the default
state) filemap_nr_thps expands to 0.

do_dentry_open has a big chunk dependent on it, most of which gets
optimized away, except for a branch and a full fence:

if (f->f_mode & FMODE_WRITE) {
[snip]
        smp_mb();
        if (filemap_nr_thps(inode->i_mapping)) {
[snip]
	}
}

While the branch is pretty minor the fence really does not need to be
there.

This is a bare-minimum patch which takes care of it until someone(tm)
cleans this up. Notably it does not conditionally compile other spots
which issue the matching fence.

I did not bother benchmarking it, not issuing a spurious full fence in
the fast path does not warrant justification from perf standpoint.

Signed-off-by: Mateusz Guzik <mjguzik@xxxxxxxxx>
---

I am not particularly familiar with any of this, the smp_mb in the open
for write path was sticking out like a sore thumb on code read so I
figured there may be One Weird Trick to whack it.

If the stock code is correct as is, then the ifdef as above is fine.

The ifdefed chunk is big enough that it should probably be its own
routine. I don't want to bikeshed so I did not go for it.

For a moment I considered adding filemap_nr_thps_mb which would expand
to 0 or issue the fence + do the read, but then I figured a routine
claiming to post a fence and only conditionally do it is misleading at
best.

As per the commit message fences in collapse_file remain compiled in.
It is unclear to me if the code following them is doing anything useful
on kernels !CONFIG_READ_ONLY_THP_FOR_FS.

All that said, if there is cosmetic touch ups you want done here, I can
do them.

However, a nice full patch would take care of all of the above and I
have neither the information needed to do it nor the interest to get it,
so should someone insinst on a full version I'm going to suggest they
write it themselves. I repeat this is merely a damage control until
someone sorts thigs out.

 fs/open.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/open.c b/fs/open.c
index 28f2fcbebb1b..654c300b3c33 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -980,6 +980,7 @@ static int do_dentry_open(struct file *f,
 	if ((f->f_flags & O_DIRECT) && !(f->f_mode & FMODE_CAN_ODIRECT))
 		return -EINVAL;
 
+#ifdef CONFIG_READ_ONLY_THP_FOR_FS
 	/*
 	 * XXX: Huge page cache doesn't support writing yet. Drop all page
 	 * cache for this file before processing writes.
@@ -1007,6 +1008,7 @@ static int do_dentry_open(struct file *f,
 			filemap_invalidate_unlock(inode->i_mapping);
 		}
 	}
+#endif
 
 	return 0;
 
-- 
2.43.0





[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Bugtraq]     [Linux OMAP]     [Linux MIPS]     [eCos]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux