Re: [patch v3] swap: virtual swap readahead

KAMEZAWA Hiroyuki
2009-06-10T08:34:40Z

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
> ?
>
> > 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;

+ ptep = pte_offset_map_lock(mm, pmd, min, &ptl);
+ for (i = nr = 0; i < limit; i++)
+ if (is_swap_pte(ptep[i]))
+ entries[nr++] = pte_to_swp_entry(ptep[i]);
+ pte_unmap_unlock(ptep, ptl);

Cheer!,
-Kame