Re: [PATCH 08/33] readahead: common macros

[Nick Piggin <nickpiggin@yahoo.com.au>]

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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/