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:

quoted

On Wed, Jun 10, 2009 at 03:37:02AM +0800, Johannes Weiner wrote:

quoted

On Tue, Jun 09, 2009 at 09:01:28PM +0200, Johannes Weiner wrote:

quoted

[resend with lists cc'd, sorry]

[and fixed Hugh's email.  crap]

quoted

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

Thanks,
Fengguang

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>