Re: [PATCH 08/33] readahead: common macros
From: Nick Piggin
Date: Thu May 25 2006 - 23:32:22 EST
Wu Fengguang wrote:
On Thu, May 25, 2006 at 03:56:24PM +1000, Nick Piggin wrote:
+#define PAGES_BYTE(size) (((size) + PAGE_CACHE_SIZE - 1) >>
PAGE_CACHE_SHIFT)
+#define PAGES_KB(size) PAGES_BYTE((size)*1024)
Don't really like the names. Don't think they do anything for clarity, but
if you can come up with something better for PAGES_BYTE I might change my
mind ;) (just forget about PAGES_KB - people know what *1024 means)
No, they are mainly for concision. Don't you think it's cleaner to write
PAGES_KB(VM_MAX_READAHEAD)
than
(VM_MAX_READAHEAD * 1024) / PAGE_CACHE_SIZE
No. Apart from semantics being different (which I'll address below), anybody
with any business looking at this code will immediately know and understand
what the latter line means. Not so for the former.
Admittedly the names are somewhat awkward though :)
Also: the replacements are wrong: if you've defined VM_MAX_READAHEAD to be
4095 bytes, you don't want the _actual_ readahead to be 4096 bytes, do you?
It is saying nothing about minimum, so presumably 0 is the correct choice.
The macros were first introduced exact for this reason ;)
It is rumored that there will be 64K page support, and this macro
helps round up the 16K sized VM_MIN_READAHEAD. The eof_index also
needs rounding up.
But VM_MIN_READAHEAD of course should be rounded up, for the same
reasons I said VM_MAX_READAHEAD should be rounded down.
So OK as a bug fix, but it needs to be in its own patch, not in a "common
macros" one, and sufficiently commented (and preferably outside your core
adaptive readahead code so it can be quickly merged up)
--
Send instant messages to your online friends http://au.messenger.yahoo.com
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/