scalable kmap (was Re: vm lock contention reduction) (fwd)

Andrew and Martin,

I ran this updated patch on 2.5.25 with dbench on
the 8-way with 4 Gb of memory compared to clean 2.5.25. I saw a significant improvement in throughput about 15% (averaged over 5 runs each).
Included is the pretty picture (akpm-2525.png) the
data that picture came from (akpm-2525.data) and the raw results of the runs with min/max and timing results (2525akpmkmaphi and 2525clnhi).
I believe the drop at around 64 clients is caused by
memory swapping leading to increased disk accesses since the time increased by 200% in direct correlation with the decreased throughput.

Hanna

Thanks, Hanna.

The kernel compile test isn't a particularly heavy user of copy_to_user(), whereas with RAM-only dbench, copy_*_user() is almost the only thing it does. So that makes sense.

Tried dbench on the 2.5G 4xPIII Xeon: no improvement at all. This thing seems to have quite poor memory bandwidth - maybe 250 megabyte/sec downhill with the wind at its tail.

Yes. The test went to disk. There are two reasons why
it will do this:

1: Some dirty data was in memory for more than 30-35 seconds or

2: More than 40% of memory is dirty.

In your case, the 64-client run was taking 32 seconds. After that the disks lit up. Once that happens, dbench isn't a very good benchmark. It's an excellent benchmark when it's RAM-only though. Very repeatable and hits lots of code paths which matter.

You can run more clients before the disk I/O cuts in by increasing /proc/sys/vm/dirty_expire_centisecs and /proc/sys/vm/dirty_*_ratio.

The patch you tested only uses the atomic kmap across generic_file_read. It is reasonable to hope that another 15% or morecan be gained by holding an atomic kmap across writes as well. On your machine ;)

Here's what oprofile says about `dbench 40' with that patch:

c0140f1c 402      0.609543    __block_commit_write    
c013dfd4 413      0.626222    vfs_write               
c01402cc 431      0.653515    __find_get_block        
c013a895 472      0.715683    .text.lock.highmem      
c017fe30 494      0.749041    ext2_get_block          
c012cef0 564      0.85518     unlock_page             
c013ee80 564      0.85518     fget                    
c01079f4 571      0.865794    apic_timer_interrupt    
c01e8ecc 594      0.900669    radix_tree_lookup       
c013da90 597      0.905218    generic_file_llseek     
c01514b4 607      0.92038     __d_lookup              
c0106ff8 687      1.04168     system_call             
c013a02c 874      1.32523     kunmap_high             
c0148388 922      1.39801     link_path_walk          
c0140b00 1097     1.66336     __block_prepare_write   
c01346d0 1138     1.72552     rmqueue                 
c01127ac 1243     1.88473     smp_apic_timer_interrupt 
c0139eb8 1514     2.29564     kmap_high               
c0105368 6188     9.38272     poll_idle               
c012d8a8 9564     14.5017     file_read_actor         
c012ea70 21326    32.3361     generic_file_write      

Not taking a kmap in generic_file_write is a biggish patch - it means changing the prepare_write/commit_write API and visiting all filesystems. The API change would be: core kernel no longer holds a kmap across prepare/commit. If the filesystem wants one for its own purposes then it gets to do it for itself, possibly in its prepare_write().

I think I'd prefer to get some additional testing and understanding before undertaking that work. It arguably makes sense as a small cleanup/speedup anyway, but that's not a burning issue.

hmm. I'll do just ext2, and we can take another look then.

-

OK, so I'm full of it. It's actually quite simple and clean.

This patch is incremental to the one which you just tested. It takes an atomic kmap across generic_file_write().

There's a copy of these patches at
http://www.zip.com.au/~akpm/linux/patches/2.5/2.5.25/

The patch also teaches ext2 and ext3 about the new API. If you're using any other filesystems they will probably oops.

ext2 is still using regular kmap() for directory operations.

And this time, it actually buys 3% improvement on my lumbering hulk, so your machine will hopefully see nice improvements.

Profile looks better too. kmap_high and the IPI have vanished.

c013c164 304      0.50923     __set_page_dirty_nobuffers 
c0141008 355      0.59466     __block_commit_write    
c013e004 382      0.639887    vfs_write               
c01402fc 410      0.68679     __find_get_block        
c017fd70 471      0.788971    ext2_get_block          
c01e8eec 530      0.887802    radix_tree_lookup       
c012cef0 542      0.907903    unlock_page             
c013eeb0 562      0.941405    fget                    
c013dac0 585      0.979932    generic_file_llseek     
c01514e4 600      1.00506     __d_lookup              
c0106ff8 635      1.06369     system_call             
c01483b8 872      1.46069     link_path_walk          
c0134700 1014     1.69855     rmqueue                 
c0140b30 1191     1.99504     __block_prepare_write   
c0105368 4029     6.74897     poll_idle               
c012d8a8 9520     15.9469     file_read_actor         
c012ea70 23301    39.0315     generic_file_write      

 fs/buffer.c     |   57 +++++++++++++++++++++++++++++++-------------------------
 fs/ext2/inode.c |   16 +++++++++++++--
 fs/ext3/inode.c |   30 +++++------------------------
 mm/filemap.c    |    6 ++---
 4 files changed, 55 insertions(+), 54 deletions(-)

@@ -2228,6 +2228,7 @@ generic_file_write(struct file *file, co
 		unsigned long offset;
 		long page_fault;
 		char *kaddr;
+		struct copy_user_state copy_user_state;
 
 		offset = (pos & (PAGE_CACHE_SIZE -1)); /* Within page */
 		index = pos >> PAGE_CACHE_SHIFT;

@@ -2252,22 +2253,22 @@ generic_file_write(struct file *file, co
 			break;
 		}
 
-		kaddr = kmap(page);
 		status = a_ops->prepare_write(file, page, offset, offset+bytes);
 		if (unlikely(status)) {
 			/*
 			 * prepare_write() may have instantiated a few blocks
 			 * outside i_size.  Trim these off again.
 			 */
-			kunmap(page);
 			unlock_page(page);
 			page_cache_release(page);
 			if (pos + bytes > inode->i_size)
 				vmtruncate(inode, inode->i_size);
 			break;
 		}
+		kaddr = kmap_copy_user(&copy_user_state, page, KM_FILEMAP, 0);
 		page_fault = __copy_from_user(kaddr + offset, buf, bytes);
 		flush_dcache_page(page);
+		kunmap_copy_user(&copy_user_state);
 		status = a_ops->commit_write(file, page, offset, offset+bytes);
 		if (unlikely(page_fault)) {
 			status = -EFAULT;

@@ -2282,7 +2283,6 @@ generic_file_write(struct file *file, co
 				buf += status;
 			}
 		}
-		kunmap(page);
 		if (!PageReferenced(page))
 			SetPageReferenced(page);
 		unlock_page(page);

@@ -1048,16 +1048,6 @@ static int ext3_prepare_write(struct fil
 	if (ext3_should_journal_data(inode)) {
 		ret = walk_page_buffers(handle, page_buffers(page),
 				from, to, NULL, do_journal_get_write_access);
-		if (ret) {
-			/*
-			 * We're going to fail this prepare_write(),
-			 * so commit_write() will not be called.
-			 * We need to undo block_prepare_write()'s kmap().
-			 * AKPM: Do we need to clear PageUptodate?  I don't
-			 * think so.
-			 */
-			kunmap(page);
-		}
 	}
 prepare_write_failed:
 	if (ret)

@@ -1117,7 +1107,6 @@ static int ext3_commit_write(struct file
 			from, to, &partial, commit_write_fn);
 		if (!partial)
 			SetPageUptodate(page);
-		kunmap(page);
 		if (pos > inode->i_size)
 			inode->i_size = pos;
 		EXT3_I(inode)->i_state |= EXT3_STATE_JDATA;

@@ -1128,17 +1117,8 @@ static int ext3_commit_write(struct file
 		}
 		/* Be careful here if generic_commit_write becomes a
 		 * required invocation after block_prepare_write. */
-		if (ret == 0) {
+		if (ret == 0)
 			ret = generic_commit_write(file, page, from, to);
-		} else {
-			/*
-			 * block_prepare_write() was called, but we're not
-			 * going to call generic_commit_write().  So we
-			 * need to perform generic_commit_write()'s kunmap
-			 * by hand.
-			 */
-			kunmap(page);
-		}
 	}
 	if (inode->i_size > EXT3_I(inode)->i_disksize) {
 		EXT3_I(inode)->i_disksize = inode->i_size;

@@ -1433,6 +1413,7 @@ static int ext3_block_truncate_page(hand
 	struct page *page;
 	struct buffer_head *bh;
 	int err;
+	void *kaddr;
 
 	blocksize = inode->i_sb->s_blocksize;
 	length = offset & (blocksize - 1);

@@ -1488,10 +1469,11 @@ static int ext3_block_truncate_page(hand
 		if (err)
 			goto unlock;
 	}
-	
-	memset(kmap(page) + offset, 0, length);
+
+	kaddr = kmap_atomic(page, KM_USER0);
+	memset(kaddr + offset, 0, length);
 	flush_dcache_page(page);
-	kunmap(page);
+	kunmap_atomic(kaddr, KM_USER0);
 
 	BUFFER_TRACE(bh, "zeroed end of block");
 

@@ -595,12 +595,24 @@ ext2_readpages(struct address_space *map
 }
 
 static int
-ext2_prepare_write(struct file *file, struct page *page,
+ext2_prepare_write(struct file *file_may_be_null, struct page *page,
 			unsigned from, unsigned to)
 {
+	if (S_ISDIR(page->mapping->host->i_mode))
+		kmap(page);
 	return block_prepare_write(page,from,to,ext2_get_block);
 }
 
+static int ext2_commit_write(struct file *file, struct page *page,
+			unsigned from, unsigned to)
+{
+	int ret = generic_commit_write(file, page, from, to);
+
+	if (S_ISDIR(page->mapping->host->i_mode))
+		kunmap(page);
+	return ret;
+}
+
 static int ext2_bmap(struct address_space *mapping, long block)
 {
 	return generic_block_bmap(mapping,block,ext2_get_block);

@@ -633,7 +645,7 @@ struct address_space_operations ext2_aop
 	writepage:		ext2_writepage,
 	sync_page:		block_sync_page,
 	prepare_write:		ext2_prepare_write,
-	commit_write:		generic_commit_write,
+	commit_write:		ext2_commit_write,
 	bmap:			ext2_bmap,
 	direct_IO:		ext2_direct_IO,
 	writepages:		ext2_writepages,

-