lkml.org 
[lkml]   [2006]   [May]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 15/33] readahead: state based method - routines
    Wu Fengguang <wfg@mail.ustc.edu.cn> wrote:
    >
    > Define some helpers on struct file_ra_state.
    >
    > +/*
    > + * The 64bit cache_hits stores three accumulated values and a counter value.
    > + * MSB LSB
    > + * 3333333333333333 : 2222222222222222 : 1111111111111111 : 0000000000000000
    > + */
    > +static int ra_cache_hit(struct file_ra_state *ra, int nr)
    > +{
    > + return (ra->cache_hits >> (nr * 16)) & 0xFFFF;
    > +}

    So... why not use four u16s?

    > +/*
    > + * Submit IO for the read-ahead request in file_ra_state.
    > + */
    > +static int ra_dispatch(struct file_ra_state *ra,
    > + struct address_space *mapping, struct file *filp)
    > +{
    > + enum ra_class ra_class = ra_class_new(ra);
    > + unsigned long ra_size = ra_readahead_size(ra);
    > + unsigned long la_size = ra_lookahead_size(ra);
    > + pgoff_t eof_index = PAGES_BYTE(i_size_read(mapping->host)) + 1;

    Sigh. I guess one gets used to that PAGES_BYTE thing after a while. If
    you're not familiar with it, it obfuscates things.

    <hunts around for its definition>

    So in fact it's converting a loff_t to a pgoff_t. Why not call it
    convert_loff_t_to_pgoff_t()? ;)

    Something better, anyway. Something lower-case and an inline-not-a-macro, too.

    > + int actual;
    > +
    > + if (unlikely(ra->ra_index >= eof_index))
    > + return 0;
    > +
    > + /* Snap to EOF. */
    > + if (ra->readahead_index + ra_size / 2 > eof_index) {

    You've had a bit of a think and you've arrived at a design decision
    surrounding the arithmetic in here. It's very very hard to look at this line
    of code and to work out why you decided to implement it in this fashion.
    The only way to make such code comprehensible (and hence maintainable) is
    to fully comment such things.


    -
    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/

    \
     
     \ /
      Last update: 2006-05-26 19:18    [W:0.033 / U:0.040 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site