Message-Id: <20090223155313.abd41881.akpm@linux-foundation.org>
Date:	Mon, 23 Feb 2009 15:53:13 -0800
From:	Andrew Morton <akpm@...ux-foundation.org>
To:	Mel Gorman <mel@....ul.ie>
Cc:	linux-mm@...ck.org, penberg@...helsinki.fi, riel@...hat.com,
	kosaki.motohiro@...fujitsu.com, cl@...ux-foundation.org,
	hannes@...xchg.org, npiggin@...e.de, linux-kernel@...r.kernel.org,
	ming.m.lin@...el.com, yanmin_zhang@...ux.intel.com
Subject: Re: [PATCH 20/20] Get rid of the concept of hot/cold page freeing

On Mon, 23 Feb 2009 23:30:30 +0000
Mel Gorman <mel@....ul.ie> wrote:

> On Mon, Feb 23, 2009 at 01:37:23AM -0800, Andrew Morton wrote:
> > On Sun, 22 Feb 2009 23:17:29 +0000 Mel Gorman <mel@....ul.ie> wrote:
> > 
> > > Currently an effort is made to determine if a page is hot or cold when
> > > it is being freed so that cache hot pages can be allocated to callers if
> > > possible. However, the reasoning used whether to mark something hot or
> > > cold is a bit spurious. A profile run of kernbench showed that "cold"
> > > pages were never freed so it either doesn't happen generally or is so
> > > rare, it's barely measurable.
> > > 
> > > It's dubious as to whether pages are being correctly marked hot and cold
> > > anyway. Things like page cache and pages being truncated are are considered
> > > "hot" but there is no guarantee that these pages have been recently used
> > > and are cache hot. Pages being reclaimed from the LRU are considered
> > > cold which is logical because they cannot have been referenced recently
> > > but if the system is reclaiming pages, then we have entered allocator
> > > slowpaths and are not going to notice any potential performance boost
> > > because a "hot" page was freed.
> > > 
> > > This patch just deletes the concept of freeing hot or cold pages and
> > > just frees them all as hot.
> > > 
> > 
> > Well yes.  We waffled for months over whether to merge that code originally.
> > 
> > What tipped the balance was a dopey microbenchmark which I wrote which
> > sat in a loop extending (via write()) and then truncating the same file
> > by 32 kbytes (or thereabouts).  Its performance was increased by a lot
> > (2x or more, iirc) and no actual regressions were demonstrable, so we
> > merged it.
> > 
> > Could you check that please?  I'd suggest trying various values of 32k,
> > too.
> > 
> 
> I dug around the archives but hadn't much luck finding the original
> discussion. I saw some results from around the 2.5.40-mm timeframe that talked
> about ~60% difference with this benchmark (http://lkml.org/lkml/2002/10/6/174)
> but didn't find the source. The more solid benchmark reports was
> https://lwn.net/Articles/14761/ where you talked about 1-2% kernel compile
> improvements, good SpecWEB and a big hike on performance with SDET.
> 
> It's not clearcut. I tried reproducing your original benchmark rather than
> whinging about not finding yours :) . The source is below so maybe you can
> tell me if it's equivalent? I only ran it on one CPU which also may be a
> factor. The results were
> 
>     size      with   without difference
>       64  0.216033  0.558803 -158.67%
>      128  0.158551  0.150673   4.97%
>      256  0.153240  0.153488  -0.16%
>      512  0.156502  0.158769  -1.45%
>     1024  0.162146  0.163302  -0.71%
>     2048  0.167001  0.169573  -1.54%
>     4096  0.175376  0.178882  -2.00%
>     8192  0.237618  0.243385  -2.43%
>    16384  0.735053  0.351040  52.24%
>    32768  0.524731  0.583863 -11.27%
>    65536  1.149310  1.227855  -6.83%
>   131072  2.160248  2.084981   3.48%
>   262144  3.858264  4.046389  -4.88%
>   524288  8.228358  8.259957  -0.38%
>  1048576 16.228190 16.288308  -0.37%
> 
> with    == Using hot/cold information to place pages at the front or end of
>         the freelist
> without == Consider all pages being freed as hot

My head is spinning.  Smaller is better, right?  So for 16384-byte
writes, current mainline is slower?

That's odd.

> The results are a bit all over the place but mostly negative but nowhere near
> 60% of a difference so the benchmark might be wrong. Oddly, 64 shows massive
> regressions but 16384 shows massive improvements. With profiling enabled, it's
> 
>       64  0.214873  0.196666   8.47%
>      128  0.166807  0.162612   2.51%
>      256  0.170776  0.161861   5.22%
>      512  0.175772  0.164903   6.18%
>     1024  0.178835  0.168695   5.67%
>     2048  0.183769  0.174317   5.14%
>     4096  0.191877  0.183343   4.45%
>     8192  0.262511  0.254148   3.19%
>    16384  0.388201  0.371461   4.31%
>    32768  0.655402  0.611528   6.69%
>    65536  1.325445  1.193961   9.92%
>   131072  2.218135  2.209091   0.41%
>   262144  4.117233  4.116681   0.01%
>   524288  8.514915  8.590700  -0.89%
>  1048576 16.657330 16.708367  -0.31%
> 
> Almost the opposite with steady improvements almost all the way through.
> 
> With the patch applied, we are still using hot/cold information on the
> allocation side so I'm somewhat surprised the patch even makes much of a
> difference. I'd have expected the pages being freed to be mostly hot.

Oh yeah.  Back in the ancient days, hot-cold-pages was using separate
magazines for hot and cold pages.  Then Christoph went and mucked with
it, using a single queue.  That might have affected things.

It would be interesting to go back to a suitably-early kernel to see if
we broke it sometime after the early quantitative testing.  But I could
understand you not being so terribly interested ;)

> Kernbench was no help figuring this out either.
> 
> with:    Elapsed: 74.1625s User: 253.85s System: 27.1s CPU: 378.5%
> without: Elapsed: 74.0525s User: 252.9s System: 27.3675s CPU: 378.25%
> 
> Improvements on elapsed and user time but a regression on system time.
> 
> The issue is sufficiently cloudy that I'm just going to drop the patch
> for now. Hopefully the rest of the patchset is more clear-cut. I'll pick
> it up again at a later time.

Well...  if the benefits of the existing code are dubious then we
should default to deleting it.

> Here is the microbenchmark I used
> 
> Thanks.
> 
> /*
>  * write-truncate.c
>  * Microbenchmark that tests the speed of write/truncate of small files.
>  * 
>  * Suggested by Andrew Morton
>  * Written by Mel Gorman 2009
>  */
> #include <stdio.h>
> #include <limits.h>
> #include <unistd.h>
> #include <sys/types.h>
> #include <sys/time.h>
> #include <fcntl.h>
> #include <stdlib.h>
> #include <string.h>
> 
> #define TESTFILE "./write-truncate-testfile.dat"
> #define ITERATIONS 10000
> #define STARTSIZE 32
> #define SIZES 15
> 
> #ifndef MIN
> #define MIN(x,y) ((x)<(y)?(x):(y))
> #endif
> #ifndef MAX
> #define MAX(x,y) ((x)>(y)?(x):(y))
> #endif
> 
> double whattime()
> {
>         struct timeval tp;
>         int i;
> 
> 	if (gettimeofday(&tp,NULL) == -1) {
> 		perror("gettimeofday");
> 		exit(EXIT_FAILURE);
> 	}
> 
>         return ( (double) tp.tv_sec + (double) tp.tv_usec * 1.e-6 );
> }
> 
> int main(void)
> {
> 	int fd;
> 	int bufsize, sizes, iteration;
> 	char *buf;
> 	double t;
> 
> 	/* Create test file */
> 	fd = open(TESTFILE, O_RDWR|O_CREAT|O_EXCL);
> 	if (fd == -1) {
> 		perror("open");
> 		exit(EXIT_FAILURE);
> 	}
> 
> 	/* Unlink now for cleanup */
> 	if (unlink(TESTFILE) == -1) {
> 		perror("unlinke");
> 		exit(EXIT_FAILURE);
> 	}
> 
> 	/* Go through a series of sizes */
> 	bufsize = STARTSIZE;
> 	for (sizes = 1; sizes <= SIZES; sizes++) {
> 		bufsize *= 2;
> 		buf = malloc(bufsize);
> 		if (buf == NULL) {
> 			printf("ERROR: Malloc failed\n");
> 			exit(EXIT_FAILURE);
> 		}
> 		memset(buf, 0xE0, bufsize);
> 
> 		t = whattime();
> 		for (iteration = 0; iteration < ITERATIONS; iteration++) {
> 			size_t written = 0, thiswrite;
> 			
> 			while (written != bufsize) {
> 				thiswrite = write(fd, buf, bufsize);

(it should write bufsize-written ;))

> 				if (thiswrite == -1) {
> 					perror("write");
> 					exit(EXIT_FAILURE);
> 				}
> 				written += thiswrite;
> 			}
> 
> 			if (ftruncate(fd, 0) == -1) {
> 				perror("ftruncate");
> 				exit(EXIT_FAILURE);
> 			}
> 
> 			if (lseek(fd, 0, SEEK_SET) != 0) {
> 				perror("lseek");
> 				exit(EXIT_FAILURE);
> 			}
> 		}

yup, I think that captures the same idea.

> 		t = whattime() - t;
> 		free(buf);
> 
> 		printf("%d %f\n", bufsize, t);
> 	}
> 
> 	if (close(fd) == -1) {
> 		perror("close");
> 		exit(EXIT_FAILURE);
> 	}
> 
> 	exit(EXIT_SUCCESS);
> }
> -- 
> Mel Gorman
> Part-time Phd Student                          Linux Technology Center
> University of Limerick                         IBM Dublin Software Lab
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists