67-mjb2 vs 68-mjb1 (sdet degredation)

67-mjb2 vs 68-mjb1 (sdet degredation)

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

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

Re: 67-mjb2 vs 68-mjb1 (sdet degredation)

[Andrew Morton]

"Martin J. Bligh" <mbligh@aracnet.com> wrote:
>
> 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?



diff -puN fs/fs-writeback.c~page_lock-is-rwlock fs/fs-writeback.c
--- 25/fs/fs-writeback.c~page_lock-is-rwlock	Mon Apr 21 14:44:55 2003
+++ 25-akpm/fs/fs-writeback.c	Mon Apr 21 14:45:00 2003
@@ -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);
diff -puN fs/inode.c~page_lock-is-rwlock fs/inode.c
--- 25/fs/inode.c~page_lock-is-rwlock	Mon Apr 21 14:44:55 2003
+++ 25-akpm/fs/inode.c	Mon Apr 21 14:45:00 2003
@@ -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);
diff -puN fs/mpage.c~page_lock-is-rwlock fs/mpage.c
--- 25/fs/mpage.c~page_lock-is-rwlock	Mon Apr 21 14:44:55 2003
+++ 25-akpm/fs/mpage.c	Mon Apr 21 14:45:00 2003
@@ -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;
diff -puN include/linux/fs.h~page_lock-is-rwlock include/linux/fs.h
--- 25/include/linux/fs.h~page_lock-is-rwlock	Mon Apr 21 14:44:55 2003
+++ 25-akpm/include/linux/fs.h	Mon Apr 21 14:45:00 2003
@@ -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 */
diff -puN mm/filemap.c~page_lock-is-rwlock mm/filemap.c
--- 25/mm/filemap.c~page_lock-is-rwlock	Mon Apr 21 14:44:55 2003
+++ 25-akpm/mm/filemap.c	Mon Apr 21 14:45:00 2003
@@ -99,9 +99,9 @@ void remove_from_page_cache(struct page 
 	if (unlikely(!PageLocked(page)))
 		PAGE_BUG(page);
 
-	spin_lock(&mapping->page_lock);
+	write_lock(&mapping->page_lock);
 	__remove_from_page_cache(page);
-	spin_unlock(&mapping->page_lock);
+	write_unlock(&mapping->page_lock);
 }
 
 static inline int sync_page(struct page *page)
@@ -133,9 +133,9 @@ static int __filemap_fdatawrite(struct a
 	if (mapping->backing_dev_info->memory_backed)
 		return 0;
 
-	spin_lock(&mapping->page_lock);
+	write_lock(&mapping->page_lock);
 	list_splice_init(&mapping->dirty_pages, &mapping->io_pages);
-	spin_unlock(&mapping->page_lock);
+	write_unlock(&mapping->page_lock);
 	ret = do_writepages(mapping, &wbc);
 	return ret;
 }
@@ -166,7 +166,7 @@ int filemap_fdatawait(struct address_spa
 
 restart:
 	progress = 0;
-	spin_lock(&mapping->page_lock);
+	write_lock(&mapping->page_lock);
         while (!list_empty(&mapping->locked_pages)) {
 		struct page *page;
 
@@ -180,7 +180,7 @@ restart:
 		if (!PageWriteback(page)) {
 			if (++progress > 32) {
 				if (need_resched()) {
-					spin_unlock(&mapping->page_lock);
+					write_unlock(&mapping->page_lock);
 					__cond_resched();
 					goto restart;
 				}
@@ -190,16 +190,16 @@ restart:
 
 		progress = 0;
 		page_cache_get(page);
-		spin_unlock(&mapping->page_lock);
+		write_unlock(&mapping->page_lock);
 
 		wait_on_page_writeback(page);
 		if (PageError(page))
 			ret = -EIO;
 
 		page_cache_release(page);
-		spin_lock(&mapping->page_lock);
+		write_lock(&mapping->page_lock);
 	}
-	spin_unlock(&mapping->page_lock);
+	write_unlock(&mapping->page_lock);
 	return ret;
 }
 
@@ -227,7 +227,7 @@ int add_to_page_cache(struct page *page,
 
 	if (error == 0) {
 		page_cache_get(page);
-		spin_lock(&mapping->page_lock);
+		write_lock(&mapping->page_lock);
 		error = radix_tree_insert(&mapping->page_tree, offset, page);
 		if (!error) {
 			SetPageLocked(page);
@@ -235,7 +235,7 @@ int add_to_page_cache(struct page *page,
 		} else {
 			page_cache_release(page);
 		}
-		spin_unlock(&mapping->page_lock);
+		write_unlock(&mapping->page_lock);
 		radix_tree_preload_end();
 	}
 	return error;
@@ -364,11 +364,11 @@ struct page * find_get_page(struct addre
 	 * We scan the hash list read-only. Addition to and removal from
 	 * the hash-list needs a held write-lock.
 	 */
-	spin_lock(&mapping->page_lock);
+	read_lock(&mapping->page_lock);
 	page = radix_tree_lookup(&mapping->page_tree, offset);
 	if (page)
 		page_cache_get(page);
-	spin_unlock(&mapping->page_lock);
+	read_unlock(&mapping->page_lock);
 	return page;
 }
 
@@ -379,11 +379,11 @@ struct page *find_trylock_page(struct ad
 {
 	struct page *page;
 
-	spin_lock(&mapping->page_lock);
+	read_lock(&mapping->page_lock);
 	page = radix_tree_lookup(&mapping->page_tree, offset);
 	if (page && TestSetPageLocked(page))
 		page = NULL;
-	spin_unlock(&mapping->page_lock);
+	read_unlock(&mapping->page_lock);
 	return page;
 }
 
@@ -403,15 +403,15 @@ struct page *find_lock_page(struct addre
 {
 	struct page *page;
 
-	spin_lock(&mapping->page_lock);
+	read_lock(&mapping->page_lock);
 repeat:
 	page = radix_tree_lookup(&mapping->page_tree, offset);
 	if (page) {
 		page_cache_get(page);
 		if (TestSetPageLocked(page)) {
-			spin_unlock(&mapping->page_lock);
+			read_unlock(&mapping->page_lock);
 			lock_page(page);
-			spin_lock(&mapping->page_lock);
+			read_lock(&mapping->page_lock);
 
 			/* Has the page been truncated while we slept? */
 			if (page->mapping != mapping || page->index != offset) {
@@ -421,7 +421,7 @@ repeat:
 			}
 		}
 	}
-	spin_unlock(&mapping->page_lock);
+	read_unlock(&mapping->page_lock);
 	return page;
 }
 
@@ -491,12 +491,12 @@ unsigned int find_get_pages(struct addre
 	unsigned int i;
 	unsigned int ret;
 
-	spin_lock(&mapping->page_lock);
+	read_lock(&mapping->page_lock);
 	ret = radix_tree_gang_lookup(&mapping->page_tree,
 				(void **)pages, start, nr_pages);
 	for (i = 0; i < ret; i++)
 		page_cache_get(pages[i]);
-	spin_unlock(&mapping->page_lock);
+	read_unlock(&mapping->page_lock);
 	return ret;
 }
 
diff -puN mm/page-writeback.c~page_lock-is-rwlock mm/page-writeback.c
--- 25/mm/page-writeback.c~page_lock-is-rwlock	Mon Apr 21 14:44:55 2003
+++ 25-akpm/mm/page-writeback.c	Mon Apr 21 14:45:00 2003
@@ -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);
 		}
 	}
diff -puN mm/readahead.c~page_lock-is-rwlock mm/readahead.c
--- 25/mm/readahead.c~page_lock-is-rwlock	Mon Apr 21 14:44:56 2003
+++ 25-akpm/mm/readahead.c	Mon Apr 21 14:45:00 2003
@@ -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
diff -puN mm/swapfile.c~page_lock-is-rwlock mm/swapfile.c
--- 25/mm/swapfile.c~page_lock-is-rwlock	Mon Apr 21 14:44:56 2003
+++ 25-akpm/mm/swapfile.c	Mon Apr 21 14:45:00 2003
@@ -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);
 
diff -puN mm/swap_state.c~page_lock-is-rwlock mm/swap_state.c
--- 25/mm/swap_state.c~page_lock-is-rwlock	Mon Apr 21 14:44:56 2003
+++ 25-akpm/mm/swap_state.c	Mon Apr 21 14:45:00 2003
@@ -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);
diff -puN mm/truncate.c~page_lock-is-rwlock mm/truncate.c
--- 25/mm/truncate.c~page_lock-is-rwlock	Mon Apr 21 14:44:56 2003
+++ 25-akpm/mm/truncate.c	Mon Apr 21 14:45:00 2003
@@ -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;
diff -puN mm/vmscan.c~page_lock-is-rwlock mm/vmscan.c
--- 25/mm/vmscan.c~page_lock-is-rwlock	Mon Apr 21 14:44:56 2003
+++ 25-akpm/mm/vmscan.c	Mon Apr 21 14:45:00 2003
@@ -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:

_

Re: 67-mjb2 vs 68-mjb1 (sdet degredation)

[William Lee Irwin III]

"Martin J. Bligh" <mbligh@aracnet.com> wrote:
>> 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).

On Mon, Apr 21, 2003 at 02:46:31PM -0700, Andrew Morton wrote:
> 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

Re: 67-mjb2 vs 68-mjb1 (sdet degredation)

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

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

Re: 67-mjb2 vs 68-mjb1 (sdet degredation)

[Andrew Morton]

"Martin J. Bligh" <mbligh@aracnet.com> wrote:
>
> > 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.

Re: 67-mjb2 vs 68-mjb1 (sdet degredation)

[Martin J. Bligh]

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

M.

Re: 67-mjb2 vs 68-mjb1 (sdet degredation)

[Martin J. Bligh]

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

M.