Re: [PATCH 3/8] readahead: replace ra->mmap_miss with ra->ra_flags
From: Andrew Morton <akpm@linux-foundation.org>
Date: 2011-11-21 23:01:16
Also in:
linux-fsdevel, lkml
On Mon, 21 Nov 2011 17:18:22 +0800 Wu Fengguang [off-list ref] wrote:
Introduce a readahead flags field and embed the existing mmap_miss in it (mainly to save space).
What an ugly patch.
quoted hunk ↗ jump to hunk
It will be possible to lose the flags in race conditions, however the impact should be limited. For the race to happen, there must be two threads sharing the same file descriptor to be in page fault or readahead at the same time. Note that it has always been racy for "page faults" at the same time. And if ever the race happen, we'll lose one mmap_miss++ or mmap_miss--. Which may change some concrete readahead behavior, but won't really impact overall I/O performance. CC: Andi Kleen <redacted> CC: Steven Whitehouse <redacted> Acked-by: Rik van Riel <redacted> Signed-off-by: Wu Fengguang <redacted> --- include/linux/fs.h | 31 ++++++++++++++++++++++++++++++- mm/filemap.c | 9 ++------- 2 files changed, 32 insertions(+), 8 deletions(-)--- linux-next.orig/include/linux/fs.h 2011-11-20 11:30:55.000000000 +0800 +++ linux-next/include/linux/fs.h 2011-11-20 11:48:53.000000000 +0800@@ -945,10 +945,39 @@ struct file_ra_state { there are only # of pages ahead */ unsigned int ra_pages; /* Maximum readahead window */ - unsigned int mmap_miss; /* Cache miss stat for mmap accesses */ + unsigned int ra_flags;
And it doesn't actually save any space, unless ra_flags gets used for something else in a subsequent patch. And if it does, perhaps ra_flags should be ulong, which is compatible with the bitops.h code. Or perhaps we should use a bitfield and let the compiler do the work.
loff_t prev_pos; /* Cache last read() position */
};
+/* ra_flags bits */
+#define READAHEAD_MMAP_MISS 0x000003ff /* cache misses for mmap access */
+
+/*
+ * Don't do ra_flags++ directly to avoid possible overflow:
+ * the ra fields can be accessed concurrently in a racy way.
+ */
+static inline unsigned int ra_mmap_miss_inc(struct file_ra_state *ra)
+{
+ unsigned int miss = ra->ra_flags & READAHEAD_MMAP_MISS;
+
+ /* the upper bound avoids banging the cache line unnecessarily */
+ if (miss < READAHEAD_MMAP_MISS) {
+ miss++;
+ ra->ra_flags = miss | (ra->ra_flags & ~READAHEAD_MMAP_MISS);
+ }
+ return miss;
+}
+
+static inline void ra_mmap_miss_dec(struct file_ra_state *ra)
+{
+ unsigned int miss = ra->ra_flags & READAHEAD_MMAP_MISS;
+
+ if (miss) {
+ miss--;
+ ra->ra_flags = miss | (ra->ra_flags & ~READAHEAD_MMAP_MISS);
+ }
+}It's strange that ra_mmap_miss_inc() returns the new value whereas ra_mmap_miss_dec() returns void. -- 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/ . Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/ Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>