lkml.org 
[lkml]   [2009]   [Jun]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [patch v3] swap: virtual swap readahead
    On Wed, Jun 10, 2009 at 05:32:49PM +0900, KAMEZAWA Hiroyuki wrote:
    > On Wed, 10 Jun 2009 16:11:32 +0800
    > Wu Fengguang <fengguang.wu@intel.com> wrote:
    >
    > > On Wed, Jun 10, 2009 at 03:45:08PM +0800, Johannes Weiner wrote:
    > > > Hi Fengguang,
    > > >
    > > > On Wed, Jun 10, 2009 at 01:03:42PM +0800, Wu Fengguang wrote:
    > > > > On Wed, Jun 10, 2009 at 03:37:02AM +0800, Johannes Weiner wrote:
    > > > > > On Tue, Jun 09, 2009 at 09:01:28PM +0200, Johannes Weiner wrote:
    > > > > > > [resend with lists cc'd, sorry]
    > > > > >
    > > > > > [and fixed Hugh's email. crap]
    > > > > >
    > > > > > > Hi,
    > > > > > >
    > > > > > > here is a new iteration of the virtual swap readahead. Per Hugh's
    > > > > > > suggestion, I moved the pte collecting to the callsite and thus out
    > > > > > > ouf swap code. Unfortunately, I had to bound page_cluster due to an
    > > > > > > array of that many swap entries on the stack, but I think it is better
    > > > > > > to limit the cluster size to a sane maximum than using dynamic
    > > > > > > allocation for this purpose.
    > > > >
    > > > > Hi Johannes,
    > > > >
    > > > > When stress testing your patch, I found it triggered many OOM kills.
    > > > > Around the time of last OOMs, the memory usage is:
    > > > >
    > > > > total used free shared buffers cached
    > > > > Mem: 474 468 5 0 0 239
    > > > > -/+ buffers/cache: 229 244
    > > > > Swap: 1023 221 802
    > > >
    > > > Wow, that really confused me for a second as we shouldn't read more
    > > > pages ahead than without the patch, probably even less under stress.
    > >
    > > Yup - swap readahead is much more challenging than sequential readahead,
    > > in that it must be accurate enough given some really obscure patterns.
    > >
    > > > So the problem has to be a runaway reading. And indeed, severe
    > > > stupidity here:
    > > >
    > > > + window = cluster << PAGE_SHIFT;
    > > > + min = addr & ~(window - 1);
    > > > + max = min + cluster;
    > > > + /*
    > > > + * To keep the locking/highpte mapping simple, stay
    > > > + * within the PTE range of one PMD entry.
    > > > + */
    > > > + limit = addr & PMD_MASK;
    > > > + if (limit > min)
    > > > + min = limit;
    > > > + limit = pmd_addr_end(addr, max);
    > > > + if (limit < max)
    > > > + max = limit;
    > > > + limit = max - min;
    > > >
    > > > The mistake is at the initial calculation of max. It should be
    > > >
    > > > max = min + window;
    > > >
    > > > The resulting problem is that min could get bigger than max when
    > > > cluster is bigger than PMD_SHIFT. Did you use page_cluster == 5?
    > >
    > > No I use the default 3.
    > >
    > > btw, the mistake reflects bad named variables. How about rename
    > > cluster => pages
    > > window => bytes
    > > ?

    Proven twice, fixed in v4.

    > > > The initial min is aligned to a value below the PMD boundary and max
    > > > based on it with a too small offset, staying below the PMD boundary as
    > > > well. When min is rounded up, this becomes a bit large:
    > > >
    > > > limit = max - min;
    > > >
    > > > So if my brain is already functioning, fixing the initial max should
    > > > be enough because either
    > > >
    > > > o window is smaller than PMD_SIZE, than we won't round down
    > > > below a PMD boundary in the first place or
    > > >
    > > > o window is bigger than PMD_SIZE, than we can round down below
    > > > a PMD boundary but adding window to that is garuanteed to
    > > > cross the boundary again
    > > >
    > > > and thus max is always bigger than min.
    > > >
    > > > Fengguang, does this make sense? If so, the patch below should fix
    > > > it.
    > >
    > > Too bad, a quick test of the below patch freezes the box..
    > >
    >
    > + window = cluster << PAGE_SHIFT;
    > + min = addr & ~(window - 1);
    > + max = min + cluster;
    >
    > max = min + window; # this is fixed. then,
    >
    > + /*
    > + * To keep the locking/highpte mapping simple, stay
    > + * within the PTE range of one PMD entry.
    > + */
    > + limit = addr & PMD_MASK;
    > + if (limit > min)
    > + min = limit;
    > + limit = pmd_addr_end(addr, max);
    > + if (limit < max)
    > + max = limit;
    > + limit = max - min;
    >
    > limit = (max - min) >> PAGE_SHIFT;

    Head -> desk.

    Fixed in v4, thank you.

    Hannes


    \
     
     \ /
      Last update: 2009-06-10 11:35    [W:0.027 / U:31.184 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site