[RFC,V3] test bit before clear files_struct bits

Commit Message

Wang, Yalin Feb. 15, 2015, 8:27 a.m. UTC

Test bit before clear close_on_exec and open_fds,
by trace __clear_bit(), these 2 place are false in most times,
we test it so that we don't need clear_bit, and we can win
in most time.
Add *_if_need bitop non-atomic version.

Signed-off-by: Yalin Wang <[e-mail]>
---
 fs/file.c                               |  9 +++++++--
 include/asm-generic/bitops/non-atomic.h | 35 +++++++++++++++++++++++++++++++++
 2 files changed, 42 insertions(+), 2 deletions(-)

Comments

Andrew Morton Feb. 18, 2015, 9:27 p.m. UTC | #1

On Sun, 15 Feb 2015 16:27:27 +0800 "Wang, Yalin" <[e-mail]> wrote:

> +/*
> + * __*_if_need version is used in cases that you don't want write a bit which
> + * have been set / clear, to avoid dirty cacheline.
> + */
> +static inline void __set_bit_if_need(int nr, volatile unsigned long *addr)

"if_need" is gramatically incorrect - it should be "if_needed".

And __set_bit_if_needed is too long.  But I can't think of anything
shorter, unless we do something lame like __set_bit2(). 
__set_bit_if_clear() isn't too bad?

Also, your changelog still doesn't include the results of the
quantitative testing which you performed to demonstrate that these code
sites will benefit from this conversion.  Please copy all that info
into the changelog!
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/fs/file.c b/fs/file.c
index ee738ea..2e08e6f 100644
--- a/fs/file.c
+++ b/fs/file.c
@@ -209,7 +209,7 @@  static inline void __set_close_on_exec(int fd, struct fdtable *fdt)
 
 static inline void __clear_close_on_exec(int fd, struct fdtable *fdt)
 {
-	__clear_bit(fd, fdt->close_on_exec);
+	__clear_bit_if_need(fd, fdt->close_on_exec);
 }
 
 static inline void __set_open_fd(int fd, struct fdtable *fdt)
@@ -222,6 +222,11 @@  static inline void __clear_open_fd(int fd, struct fdtable *fdt)
 	__clear_bit(fd, fdt->open_fds);
 }
 
+static inline void __clear_open_fd_if_need(int fd, struct fdtable *fdt)
+{
+	__clear_bit_if_need(fd, fdt->open_fds);
+}
+
 static int count_open_files(struct fdtable *fdt)
 {
 	int size = fdt->max_fds;
@@ -316,7 +321,7 @@  struct files_struct *dup_fd(struct files_struct *oldf, int *errorp)
 			 * is partway through open().  So make sure that this
 			 * fd is available to the new process.
 			 */
-			__clear_open_fd(open_files - i, new_fdt);
+			__clear_open_fd_if_need(open_files - i, new_fdt);
 		}
 		rcu_assign_pointer(*new_fds++, f);
 	}
diff --git a/include/asm-generic/bitops/non-atomic.h b/include/asm-generic/bitops/non-atomic.h
index 697cc2b..0713e3b 100644
--- a/include/asm-generic/bitops/non-atomic.h
+++ b/include/asm-generic/bitops/non-atomic.h
@@ -105,4 +105,39 @@  static inline int test_bit(int nr, const volatile unsigned long *addr)
 	return 1UL & (addr[BIT_WORD(nr)] >> (nr & (BITS_PER_LONG-1)));
 }
 
+/*
+ * __*_if_need version is used in cases that you don't want write a bit which
+ * have been set / clear, to avoid dirty cacheline.
+ */
+static inline void __set_bit_if_need(int nr, volatile unsigned long *addr)
+{
+	if (!test_bit(nr, addr))
+		__set_bit(nr, addr);
+}
+
+static inline void __clear_bit_if_need(int nr, volatile unsigned long *addr)
+{
+	if (test_bit(nr, addr))
+		__clear_bit(nr, addr);
+}
+
+static inline int __test_and_set_bit_if_need(int nr, volatile unsigned long *addr)
+{
+	if (!test_bit(nr, addr)) {
+		__set_bit(nr, addr);
+		return false;
+	} else {
+		return true;
+	}
+}
+
+static inline int __test_and_clear_bit_if_need(int nr, volatile unsigned long *addr)
+{
+	if (test_bit(nr, addr)) {
+		__clear_bit(nr, addr);
+		return true;
+	} else {
+		return false;
+	}
+}
 #endif /* _ASM_GENERIC_BITOPS_NON_ATOMIC_H_ */