67-mjb2 vs 68-mjb1 (sdet degredation)

DISCUSSION Martin J. Bligh 3 participants 7 messages Apr 21, 2003 – Apr 22, 2003

Subsystems (6)

Memory management VFS MGLRU Page cache +2 more subsystems

lore

Share & tools

lore

Martin J. Bligh

author

2003-04-21 21:10 UTC #0 lore raw

Seem to loose about 2-3% on SDET syncing with 2.5.68. Not much change apart from 67-68 changes. The merge of the ext2 alloc stuff has made such a dramatic improvment for virgin 67-68, it's hard to see if there was any degredation in mainline ;-) I had those in my tree before though, so there should be much less change.

Just wondering if you can recognise / guess the problem from the profiles, else I'll poke at it some more (will probably just work out what's hitting .text.lock.filemap).

diffprofile {2.5.67-mjb2,2.5.68-mjb1}/sdetbench/128/profile

      1307     2.1% total
       432     0.0% .text.lock.filemap
       269     0.8% default_idle
       148    19.4% find_get_page
        99   165.0% grab_block
        97    26.6% __down
        85   146.6% __brelse
        78    22.2% do_no_page
        75   625.0% update_atime
        62     5.6% __d_lookup
        61   103.4% task_mem
        42    25.5% __wake_up
        39    52.7% __mark_inode_dirty
        38   211.1% read_inode_bitmap
        32     0.0% group_reserve_blocks
        31    79.5% generic_fillattr
...
       -10    -6.0% kmap_atomic
       -11   -26.8% truncate_inode_pages
       -12   -26.1% ext2_free_blocks
       -12    -5.2% do_page_fault
       -13    -7.8% .text.lock.file_table
       -13   -11.1% proc_check_root
       -14   -19.7% dentry_open
       -17    -4.1% .text.lock.namei
       -17   -17.2% fget
       -18   -10.3% .text.lock.attr
       -21   -12.1% number
       -24    -4.3% .text.lock.dcache
       -27    -2.1% page_remove_rmap
       -34   -54.8% ext2_new_block
       -36    -3.4% atomic_dec_and_lock
       -37    -3.1% copy_page_range
       -57    -7.6% __copy_to_user_ll
      -105   -95.5% task_vsize
      -113   -96.6% find_trylock_page

Andrew Morton

2003-04-21 21:38 UTC #1 lore raw

Quoted 11 lines · Martin J. Bligh

Seem to loose about 2-3% on SDET syncing with 2.5.68. Not much change apart from 67-68 changes. The merge of the ext2 alloc stuff has made such a dramatic improvment for virgin 67-68, it's hard to see if there was any degredation in mainline ;-) I had those in my tree before though, so there should be much less change.
Just wondering if you can recognise / guess the problem from the profiles, else I'll poke at it some more (will probably just work out what's hitting .text.lock.filemap).

erk. Looks like the rwlock->spinlock conversion of mapping->page_lock.

That was a small (1%?) win on small SMP, and looks to be a small lose on big SMP. No real surprise there.

Here's a backout patch. Does it fix it up?

fs/fs-writeback.c renamed from fs/fs-writeback.c~page_lock-is-rwlock +2 -2

diff -puN fs/fs-writeback.c~page_lock-is-rwlock fs/fs-writeback.c
@@ -149,10 +149,10 @@ __sync_single_inode(struct inode *inode,
 	 * read speculatively by this cpu before &= ~I_DIRTY  -- mikulas
 	 */
 
-	spin_lock(&mapping->page_lock);
+	write_lock(&mapping->page_lock);
 	if (wait || !wbc->for_kupdate || list_empty(&mapping->io_pages))
 		list_splice_init(&mapping->dirty_pages, &mapping->io_pages);
-	spin_unlock(&mapping->page_lock);
+	write_unlock(&mapping->page_lock);
 	spin_unlock(&inode_lock);
 
 	do_writepages(mapping, wbc);

fs/inode.c renamed from fs/inode.c~page_lock-is-rwlock +1 -1

diff -puN fs/inode.c~page_lock-is-rwlock fs/inode.c
@@ -181,7 +181,7 @@ void inode_init_once(struct inode *inode
 	INIT_LIST_HEAD(&inode->i_devices);
 	sema_init(&inode->i_sem, 1);
 	INIT_RADIX_TREE(&inode->i_data.page_tree, GFP_ATOMIC);
-	spin_lock_init(&inode->i_data.page_lock);
+	rwlock_init(&inode->i_data.page_lock);
 	init_MUTEX(&inode->i_data.i_shared_sem);
 	INIT_LIST_HEAD(&inode->i_data.private_list);
 	spin_lock_init(&inode->i_data.private_lock);

fs/mpage.c renamed from fs/mpage.c~page_lock-is-rwlock +4 -4

diff -puN fs/mpage.c~page_lock-is-rwlock fs/mpage.c
@@ -627,7 +627,7 @@ mpage_writepages(struct address_space *m
 		writepage = mapping->a_ops->writepage;
 
 	pagevec_init(&pvec, 0);
-	spin_lock(&mapping->page_lock);
+	write_lock(&mapping->page_lock);
 	while (!list_empty(&mapping->io_pages) && !done) {
 		struct page *page = list_entry(mapping->io_pages.prev,
 					struct page, list);

@@ -647,7 +647,7 @@ mpage_writepages(struct address_space *m
 		list_add(&page->list, &mapping->locked_pages);
 
 		page_cache_get(page);
-		spin_unlock(&mapping->page_lock);
+		write_unlock(&mapping->page_lock);
 
 		/*
 		 * At this point we hold neither mapping->page_lock nor

@@ -679,12 +679,12 @@ mpage_writepages(struct address_space *m
 			unlock_page(page);
 		}
 		page_cache_release(page);
-		spin_lock(&mapping->page_lock);
+		write_lock(&mapping->page_lock);
 	}
 	/*
 	 * Leave any remaining dirty pages on ->io_pages
 	 */
-	spin_unlock(&mapping->page_lock);
+	write_unlock(&mapping->page_lock);
 	if (bio)
 		mpage_bio_submit(WRITE, bio);
 	return ret;

include/linux/fs.h renamed from include/linux/fs.h~page_lock-is-rwlock +1 -1

diff -puN include/linux/fs.h~page_lock-is-rwlock include/linux/fs.h
@@ -313,7 +313,7 @@ struct backing_dev_info;
 struct address_space {
 	struct inode		*host;		/* owner: inode, block_device */
 	struct radix_tree_root	page_tree;	/* radix tree of all pages */
-	spinlock_t		page_lock;	/* and rwlock protecting it */
+	rwlock_t		page_lock;	/* and rwlock protecting it */
 	struct list_head	clean_pages;	/* list of clean pages */
 	struct list_head	dirty_pages;	/* list of dirty pages */
 	struct list_head	locked_pages;	/* list of locked pages */

mm/filemap.c renamed from mm/filemap.c~page_lock-is-rwlock +21 ~~-21~~

139 lines not on this page · raw message

mm/page-writeback.c renamed from mm/page-writeback.c~page_lock-is-rwlock +7 -7

diff -puN mm/page-writeback.c~page_lock-is-rwlock mm/page-writeback.c
@@ -439,12 +439,12 @@ int write_one_page(struct page *page, in
 	if (wait && PageWriteback(page))
 		wait_on_page_writeback(page);
 
-	spin_lock(&mapping->page_lock);
+	write_lock(&mapping->page_lock);
 	list_del(&page->list);
 	if (test_clear_page_dirty(page)) {
 		list_add(&page->list, &mapping->locked_pages);
 		page_cache_get(page);
-		spin_unlock(&mapping->page_lock);
+		write_unlock(&mapping->page_lock);
 		ret = mapping->a_ops->writepage(page, &wbc);
 		if (ret == 0 && wait) {
 			wait_on_page_writeback(page);

@@ -454,7 +454,7 @@ int write_one_page(struct page *page, in
 		page_cache_release(page);
 	} else {
 		list_add(&page->list, &mapping->clean_pages);
-		spin_unlock(&mapping->page_lock);
+		write_unlock(&mapping->page_lock);
 		unlock_page(page);
 	}
 	return ret;

@@ -527,14 +527,14 @@ int __set_page_dirty_buffers(struct page
 	spin_unlock(&mapping->private_lock);
 
 	if (!TestSetPageDirty(page)) {
-		spin_lock(&mapping->page_lock);
+		write_lock(&mapping->page_lock);
 		if (page->mapping) {	/* Race with truncate? */
 			if (!mapping->backing_dev_info->memory_backed)
 				inc_page_state(nr_dirty);
 			list_del(&page->list);
 			list_add(&page->list, &mapping->dirty_pages);
 		}
-		spin_unlock(&mapping->page_lock);
+		write_unlock(&mapping->page_lock);
 		__mark_inode_dirty(mapping->host, I_DIRTY_PAGES);
 	}

@@ -564,7 +564,7 @@ int __set_page_dirty_nobuffers(struct pa
 		struct address_space *mapping = page->mapping;
 
 		if (mapping) {
-			spin_lock(&mapping->page_lock);
+			write_lock(&mapping->page_lock);
 			if (page->mapping) {	/* Race with truncate? */
 				BUG_ON(page->mapping != mapping);
 				if (!mapping->backing_dev_info->memory_backed)

@@ -572,7 +572,7 @@ int __set_page_dirty_nobuffers(struct pa
 				list_del(&page->list);
 				list_add(&page->list, &mapping->dirty_pages);
 			}
-			spin_unlock(&mapping->page_lock);
+			write_unlock(&mapping->page_lock);
 			__mark_inode_dirty(mapping->host, I_DIRTY_PAGES);
 		}
 	}

mm/readahead.c renamed from mm/readahead.c~page_lock-is-rwlock +4 -4

diff -puN mm/readahead.c~page_lock-is-rwlock mm/readahead.c
@@ -217,7 +217,7 @@ __do_page_cache_readahead(struct address
 	/*
 	 * Preallocate as many pages as we will need.
 	 */
-	spin_lock(&mapping->page_lock);
+	read_lock(&mapping->page_lock);
 	for (page_idx = 0; page_idx < nr_to_read; page_idx++) {
 		unsigned long page_offset = offset + page_idx;

@@ -228,16 +228,16 @@ __do_page_cache_readahead(struct address
 		if (page)
 			continue;
 
-		spin_unlock(&mapping->page_lock);
+		read_unlock(&mapping->page_lock);
 		page = page_cache_alloc_cold(mapping);
-		spin_lock(&mapping->page_lock);
+		read_lock(&mapping->page_lock);
 		if (!page)
 			break;
 		page->index = page_offset;
 		list_add(&page->list, &page_pool);
 		ret++;
 	}
-	spin_unlock(&mapping->page_lock);
+	read_unlock(&mapping->page_lock);
 
 	/*
 	 * Now start the IO.  We ignore I/O errors - if the page is not

mm/swapfile.c renamed from mm/swapfile.c~page_lock-is-rwlock +4 -4

diff -puN mm/swapfile.c~page_lock-is-rwlock mm/swapfile.c
@@ -248,10 +248,10 @@ static int exclusive_swap_page(struct pa
 		/* Is the only swap cache user the cache itself? */
 		if (p->swap_map[swp_offset(entry)] == 1) {
 			/* Recheck the page count with the pagecache lock held.. */
-			spin_lock(&swapper_space.page_lock);
+			read_lock(&swapper_space.page_lock);
 			if (page_count(page) - !!PagePrivate(page) == 2)
 				retval = 1;
-			spin_unlock(&swapper_space.page_lock);
+			read_unlock(&swapper_space.page_lock);
 		}
 		swap_info_put(p);
 	}

@@ -319,13 +319,13 @@ int remove_exclusive_swap_page(struct pa
 	retval = 0;
 	if (p->swap_map[swp_offset(entry)] == 1) {
 		/* Recheck the page count with the pagecache lock held.. */
-		spin_lock(&swapper_space.page_lock);
+		write_lock(&swapper_space.page_lock);
 		if ((page_count(page) == 2) && !PageWriteback(page)) {
 			__delete_from_swap_cache(page);
 			SetPageDirty(page);
 			retval = 1;
 		}
-		spin_unlock(&swapper_space.page_lock);
+		write_unlock(&swapper_space.page_lock);
 	}
 	swap_info_put(p);

mm/swap_state.c renamed from mm/swap_state.c~page_lock-is-rwlock +11 ~~-11~~

diff -puN mm/swap_state.c~page_lock-is-rwlock mm/swap_state.c
@@ -34,7 +34,7 @@ extern struct address_space_operations s
 
 struct address_space swapper_space = {
 	.page_tree	= RADIX_TREE_INIT(GFP_ATOMIC),
-	.page_lock	= SPIN_LOCK_UNLOCKED,
+	.page_lock	= RW_LOCK_UNLOCKED,
 	.clean_pages	= LIST_HEAD_INIT(swapper_space.clean_pages),
 	.dirty_pages	= LIST_HEAD_INIT(swapper_space.dirty_pages),
 	.io_pages	= LIST_HEAD_INIT(swapper_space.io_pages),

@@ -191,9 +191,9 @@ void delete_from_swap_cache(struct page 
   
 	entry.val = page->index;
 
-	spin_lock(&swapper_space.page_lock);
+	write_lock(&swapper_space.page_lock);
 	__delete_from_swap_cache(page);
-	spin_unlock(&swapper_space.page_lock);
+	write_unlock(&swapper_space.page_lock);
 
 	swap_free(entry);
 	page_cache_release(page);

@@ -204,8 +204,8 @@ int move_to_swap_cache(struct page *page
 	struct address_space *mapping = page->mapping;
 	int err;
 
-	spin_lock(&swapper_space.page_lock);
-	spin_lock(&mapping->page_lock);
+	write_lock(&swapper_space.page_lock);
+	write_lock(&mapping->page_lock);
 
 	err = radix_tree_insert(&swapper_space.page_tree, entry.val, page);
 	if (!err) {

@@ -213,8 +213,8 @@ int move_to_swap_cache(struct page *page
 		___add_to_page_cache(page, &swapper_space, entry.val);
 	}
 
-	spin_unlock(&mapping->page_lock);
-	spin_unlock(&swapper_space.page_lock);
+	write_unlock(&mapping->page_lock);
+	write_unlock(&swapper_space.page_lock);
 
 	if (!err) {
 		if (!swap_duplicate(entry))

@@ -240,8 +240,8 @@ int move_from_swap_cache(struct page *pa
 
 	entry.val = page->index;
 
-	spin_lock(&swapper_space.page_lock);
-	spin_lock(&mapping->page_lock);
+	write_lock(&swapper_space.page_lock);
+	write_lock(&mapping->page_lock);
 
 	err = radix_tree_insert(&mapping->page_tree, index, page);
 	if (!err) {

@@ -249,8 +249,8 @@ int move_from_swap_cache(struct page *pa
 		___add_to_page_cache(page, mapping, index);
 	}
 
-	spin_unlock(&mapping->page_lock);
-	spin_unlock(&swapper_space.page_lock);
+	write_unlock(&mapping->page_lock);
+	write_unlock(&swapper_space.page_lock);
 
 	if (!err) {
 		swap_free(entry);

mm/truncate.c renamed from mm/truncate.c~page_lock-is-rwlock +3 -3

diff -puN mm/truncate.c~page_lock-is-rwlock mm/truncate.c
@@ -73,13 +73,13 @@ invalidate_complete_page(struct address_
 	if (PagePrivate(page) && !try_to_release_page(page, 0))
 		return 0;
 
-	spin_lock(&mapping->page_lock);
+	write_lock(&mapping->page_lock);
 	if (PageDirty(page)) {
-		spin_unlock(&mapping->page_lock);
+		write_unlock(&mapping->page_lock);
 		return 0;
 	}
 	__remove_from_page_cache(page);
-	spin_unlock(&mapping->page_lock);
+	write_unlock(&mapping->page_lock);
 	ClearPageUptodate(page);
 	page_cache_release(page);	/* pagecache ref */
 	return 1;

mm/vmscan.c renamed from mm/vmscan.c~page_lock-is-rwlock +7 -7

diff -puN mm/vmscan.c~page_lock-is-rwlock mm/vmscan.c
@@ -325,7 +325,7 @@ shrink_list(struct list_head *page_list,
 				goto keep_locked;
 			if (!may_write_to_queue(mapping->backing_dev_info))
 				goto keep_locked;
-			spin_lock(&mapping->page_lock);
+			write_lock(&mapping->page_lock);
 			if (test_clear_page_dirty(page)) {
 				int res;
 				struct writeback_control wbc = {

@@ -336,7 +336,7 @@ shrink_list(struct list_head *page_list,
 				};
 
 				list_move(&page->list, &mapping->locked_pages);
-				spin_unlock(&mapping->page_lock);
+				write_unlock(&mapping->page_lock);
 
 				SetPageReclaim(page);
 				res = mapping->a_ops->writepage(page, &wbc);

@@ -351,7 +351,7 @@ shrink_list(struct list_head *page_list,
 				}
 				goto keep;
 			}
-			spin_unlock(&mapping->page_lock);
+			write_unlock(&mapping->page_lock);
 		}
 
 		/*

@@ -385,7 +385,7 @@ shrink_list(struct list_head *page_list,
 		if (!mapping)
 			goto keep_locked;	/* truncate got there first */
 
-		spin_lock(&mapping->page_lock);
+		write_lock(&mapping->page_lock);
 
 		/*
 		 * The non-racy check for busy page.  It is critical to check

@@ -393,7 +393,7 @@ shrink_list(struct list_head *page_list,
 		 * not in use by anybody. 	(pagecache + us == 2)
 		 */
 		if (page_count(page) != 2 || PageDirty(page)) {
-			spin_unlock(&mapping->page_lock);
+			write_unlock(&mapping->page_lock);
 			goto keep_locked;
 		}

@@ -401,7 +401,7 @@ shrink_list(struct list_head *page_list,
 		if (PageSwapCache(page)) {
 			swp_entry_t swap = { .val = page->index };
 			__delete_from_swap_cache(page);
-			spin_unlock(&mapping->page_lock);
+			write_unlock(&mapping->page_lock);
 			swap_free(swap);
 			__put_page(page);	/* The pagecache ref */
 			goto free_it;

@@ -409,7 +409,7 @@ shrink_list(struct list_head *page_list,
 #endif /* CONFIG_SWAP */
 
 		__remove_from_page_cache(page);
-		spin_unlock(&mapping->page_lock);
+		write_unlock(&mapping->page_lock);
 		__put_page(page);
 
 free_it:

William Lee Irwin III

2003-04-21 22:44 UTC #2 lore raw

Quoted 3 lines · Martin J. Bligh

Quoted 3 lines
Just wondering if you can recognise / guess the problem from the profiles, else I'll poke at it some more (will probably just work out what's hitting .text.lock.filemap).

Quoted 4 lines · Andrew Morton

erk. Looks like the rwlock->spinlock conversion of mapping->page_lock. That was a small (1%?) win on small SMP, and looks to be a small lose on big SMP. No real surprise there.
Here's a backout patch. Does it fix it up?

I've yet to recover from merging against this and several others; hopefully mbligh can run the numbers you need.

-- wli

Martin J. Bligh

author

2003-04-22 03:10 UTC #3 lore raw

In reply to Andrew Morton

Quoted 11 lines

Quoted 10 lines
Seem to loose about 2-3% on SDET syncing with 2.5.68. Not much change apart from 67-68 changes. The merge of the ext2 alloc stuff has made such a dramatic improvment for virgin 67-68, it's hard to see if there was any degredation in mainline ;-) I had those in my tree before though, so there should be much less change.
Just wondering if you can recognise / guess the problem from the profiles, else I'll poke at it some more (will probably just work out what's hitting .text.lock.filemap).

Quoted 6 lines

erk. Looks like the rwlock->spinlock conversion of mapping->page_lock.
That was a small (1%?) win on small SMP, and looks to be a small lose on big SMP. No real surprise there.
Here's a backout patch. Does it fix it up?

Yeah, that fixes it. Ho hum ... I wonder if we can find something that works well for both cases? I guess the options would be:

1. Some way to make the rwlock mechanism itself faster.
2. Try to fix the contention itself somehow for this instance.

Not sure if 1 is fundamentally futile or not, but would obviously be better (more general) if it's possible ;-)

       121    26.2% __down
       115  2875.0% find_trylock_page
        37    17.9% __wake_up
        36     3.5% atomic_dec_and_lock
        24     8.6% path_release
        22    14.0% .text.lock.attr
        21     1.8% copy_page_range
        16     1.3% page_remove_rmap
        16     6.2% free_pages_and_swap_cache
        14     2.6% .text.lock.dcache
        14     9.2% number
        14    10.1% .text.lock.highmem
        12    21.1% dentry_open
        12     5.5% file_move
        11    11.0% flush_signal_handlers
        10    14.3% generic_fillattr
        10     3.4% schedule
        10     6.8% __fput
...
       -10    -1.4% __copy_to_user_ll
       -10    -5.8% fd_install
       -10    -1.5% path_lookup
       -10   -41.7% release_blocks
       -11    -5.8% __read_lock_failed
       -12    -4.5% proc_pid_stat
       -12    -0.8% zap_pte_range
       -12   -26.7% kunmap_high
       -13   -29.5% d_lookup
       -14    -9.8% __brelse
       -14    -8.9% kmap_atomic
       -14   -10.3% exit_notify
       -16    -7.5% filemap_nopage
       -17   -10.7% grab_block
       -19    -8.9% d_alloc
       -20   -11.2% __find_get_block
       -99    -0.3% default_idle
      -100   -23.3% do_no_page
      -171   -18.8% find_get_page
      -432  -100.0% .text.lock.filemap
      -484    -0.8% total

Andrew Morton

2003-04-22 03:40 UTC #4 lore raw

Quoted 8 lines · Martin J. Bligh

Quoted 2 lines
Here's a backout patch. Does it fix it up?
Yeah, that fixes it. Ho hum ... I wonder if we can find something that works well for both cases? I guess the options would be:
1. Some way to make the rwlock mechanism itself faster.
2. Try to fix the contention itself somehow for this instance.

You're using PIII's, which don't mind the additional buslocked operation. P4's hate it. We'd need to retest on big P4 and other architectures to decide.

I suspect the spinlock is better that the rwlock overall - having multiple CPUs looking up things in the same address_space is relatively uncommon. Having one CPU looking things up in one address space is very common, and that's the thing which the s/rwlock/spinlock/ speeds up.

Martin J. Bligh

author

2003-04-22 04:02 UTC #5 lore raw

Quoted 11 lines

Quoted 7 lines
Quoted 1 line
Here's a backout patch. Does it fix it up?
Yeah, that fixes it. Ho hum ... I wonder if we can find something that works well for both cases? I guess the options would be:
1. Some way to make the rwlock mechanism itself faster.
2. Try to fix the contention itself somehow for this instance.
You're using PIII's, which don't mind the additional buslocked operation. P4's hate it. We'd need to retest on big P4 and other architectures to decide.

OK, I can try do get someone to test on big x440s (P4 Xeon).

Quoted 4 lines

I suspect the spinlock is better that the rwlock overall - having multiple CPUs looking up things in the same address_space is relatively uncommon. Having one CPU looking things up in one address space is very common, and that's the thing which the s/rwlock/spinlock/ speeds up.

Yeah, wasn't really complaining - I think the spinlock is the right thing to do ... I just wanted to nail down what caused it, and be sure it was deliberate. Looks like it is, so that all seems OK. Thanks for the help ;-)

Martin J. Bligh

author

2003-04-22 03:54 UTC #6 lore raw

In reply to Martin J. Bligh

Quoted 8 lines

Yeah, that fixes it. Ho hum ... I wonder if we can find something that works well for both cases? I guess the options would be:
1. Some way to make the rwlock mechanism itself faster.
2. Try to fix the contention itself somehow for this instance.
Not sure if 1 is fundamentally futile or not, but would obviously be better (more general) if it's possible ;-)

Hmmm. Actually seems like more of a "which test you pick" thing than a machine thing. Maybe it's the balance of read / write accesses to the rwlock that matters, or something.

time dd if=/dev/zero of=foo bs=1 count=1M

with backout:

real    0m4.302s
user    0m0.900s
sys     0m3.370s

With patch:

real    0m4.016s
user    0m0.810s
sys     0m3.200s

So the patch helps that test at least.