Page Reclaim Scalability

From Nick Piggin's "keep mlocked pages off LRU" patch, this patch

[PATCH/RFC] 01/14 Reclaim Scalability: Convert anon_vma list lock a read/write lock

Against 2.6.23-rc4-mm1

Make the anon_vma list lock a read/write lock. Heaviest use of this lock is in the page_referenced()/try_to_unmap() calls from vmscan [shrink_page_list()]. These functions can use a read lock to allow some parallelism for different cpus trying to reclaim pages mapped via the same set of vmas.

This change should not change the footprint of the anon_vma in the non-debug case.

Note: I have seen systems livelock with all cpus in reclaim, down in page_referenced_anon() or try_to_unmap_anon() spinning on the anon_vma lock. I have only seen this with the AIM7 benchmark with workloads of 10s of thousands of tasks. All of these tasks are children of a single ancestor, so they all share the same anon_vma for each vm area in their respective mm's. I'm told that Apache can fork thousands of children to handle incoming connections, and I've seen similar livelocks--albeit on the i_mmap_lock [next patch] running 1000s of Oracle users on a large ia64 platform.

With this patch [along with Rik van Riel's split LRU patch] we were able to see the AIM7 workload start swapping, instead of hanging, for the first time. Same workload DID hang with just Rik's patch, so this patch is apparently useful.

Signed-off-by Lee Schermerhorn <[email protected]>

 include/linux/rmap.h |    9 ++++++---
 mm/migrate.c         |    4 ++--
 mm/mmap.c            |    4 ++--
 mm/rmap.c            |   20 ++++++++++----------
 4 files changed, 20 insertions(+), 17 deletions(-)

Index: Linux/include/linux/rmap.h
===================================================================

include/linux/rmap.h +6 -3

@@ -25,7 +25,7 @@
  * pointing to this anon_vma once its vma list is empty.
  */
 struct anon_vma {
-	spinlock_t lock;	/* Serialize access to vma list */
+	rwlock_t rwlock;	/* Serialize access to vma list */
 	struct list_head head;	/* List of private "related" vmas */
 };
 

@@ -43,18 +43,21 @@ static inline void anon_vma_free(struct 
 	kmem_cache_free(anon_vma_cachep, anon_vma);
 }
 
+/*
+ * This needs to be a write lock for __vma_link()
+ */
 static inline void anon_vma_lock(struct vm_area_struct *vma)
 {
 	struct anon_vma *anon_vma = vma->anon_vma;
 	if (anon_vma)
-		spin_lock(&anon_vma->lock);
+		write_lock(&anon_vma->rwlock);
 }
 
 static inline void anon_vma_unlock(struct vm_area_struct *vma)
 {
 	struct anon_vma *anon_vma = vma->anon_vma;
 	if (anon_vma)
-		spin_unlock(&anon_vma->lock);
+		write_unlock(&anon_vma->rwlock);
 }
 
 /*

Index: Linux/mm/rmap.c
===================================================================

mm/rmap.c +10 -10

@@ -25,7 +25,7 @@
  *   mm->mmap_sem
  *     page->flags PG_locked (lock_page)
  *       mapping->i_mmap_lock
- *         anon_vma->lock
+ *         anon_vma->rwlock
  *           mm->page_table_lock or pte_lock
  *             zone->lru_lock (in mark_page_accessed, isolate_lru_page)
  *             swap_lock (in swap_duplicate, swap_info_get)

@@ -68,7 +68,7 @@ int anon_vma_prepare(struct vm_area_stru
 		if (anon_vma) {
 			allocated = NULL;
 			locked = anon_vma;
-			spin_lock(&locked->lock);
+			write_lock(&locked->rwlock);
 		} else {
 			anon_vma = anon_vma_alloc();
 			if (unlikely(!anon_vma))

@@ -87,7 +87,7 @@ int anon_vma_prepare(struct vm_area_stru
 		spin_unlock(&mm->page_table_lock);
 
 		if (locked)
-			spin_unlock(&locked->lock);
+			write_unlock(&locked->rwlock);
 		if (unlikely(allocated))
 			anon_vma_free(allocated);
 	}

@@ -113,9 +113,9 @@ void anon_vma_link(struct vm_area_struct
 	struct anon_vma *anon_vma = vma->anon_vma;
 
 	if (anon_vma) {
-		spin_lock(&anon_vma->lock);
+		write_lock(&anon_vma->rwlock);
 		list_add_tail(&vma->anon_vma_node, &anon_vma->head);
-		spin_unlock(&anon_vma->lock);
+		write_unlock(&anon_vma->rwlock);
 	}
 }
 

@@ -127,12 +127,12 @@ void anon_vma_unlink(struct vm_area_stru
 	if (!anon_vma)
 		return;
 
-	spin_lock(&anon_vma->lock);
+	write_lock(&anon_vma->rwlock);
 	list_del(&vma->anon_vma_node);
 
 	/* We must garbage collect the anon_vma if it's empty */
 	empty = list_empty(&anon_vma->head);
-	spin_unlock(&anon_vma->lock);
+	write_unlock(&anon_vma->rwlock);
 
 	if (empty)
 		anon_vma_free(anon_vma);

@@ -143,7 +143,7 @@ static void anon_vma_ctor(void *data, st
 {
 	struct anon_vma *anon_vma = data;
 
-	spin_lock_init(&anon_vma->lock);
+ 	rwlock_init(&anon_vma->rwlock);
 	INIT_LIST_HEAD(&anon_vma->head);
 }
 

@@ -170,7 +170,7 @@ static struct anon_vma *page_lock_anon_v
 		goto out;
 
 	anon_vma = (struct anon_vma *) (anon_mapping - PAGE_MAPPING_ANON);
-	spin_lock(&anon_vma->lock);
+	read_lock(&anon_vma->rwlock);
 	return anon_vma;
 out:
 	rcu_read_unlock();

@@ -179,7 +179,7 @@ out:
 
 static void page_unlock_anon_vma(struct anon_vma *anon_vma)
 {
-	spin_unlock(&anon_vma->lock);
+	read_unlock(&anon_vma->rwlock);
 	rcu_read_unlock();
 }
 

Index: Linux/mm/mmap.c
===================================================================

mm/mmap.c +2 -2

@@ -570,7 +570,7 @@ again:			remove_next = 1 + (end > next->
 	if (vma->anon_vma)
 		anon_vma = vma->anon_vma;
 	if (anon_vma) {
-		spin_lock(&anon_vma->lock);
+		write_lock(&anon_vma->rwlock);
 		/*
 		 * Easily overlooked: when mprotect shifts the boundary,
 		 * make sure the expanding vma has anon_vma set if the

@@ -624,7 +624,7 @@ again:			remove_next = 1 + (end > next->
 	}
 
 	if (anon_vma)
-		spin_unlock(&anon_vma->lock);
+		write_unlock(&anon_vma->rwlock);
 	if (mapping)
 		spin_unlock(&mapping->i_mmap_lock);
 

Index: Linux/mm/migrate.c
===================================================================

mm/migrate.c +2 -2

@@ -234,12 +234,12 @@ static void remove_anon_migration_ptes(s
 	 * We hold the mmap_sem lock. So no need to call page_lock_anon_vma.
 	 */
 	anon_vma = (struct anon_vma *) (mapping - PAGE_MAPPING_ANON);
-	spin_lock(&anon_vma->lock);
+	read_lock(&anon_vma->rwlock);
 
 	list_for_each_entry(vma, &anon_vma->head, anon_vma_node)
 		remove_migration_pte(vma, old, new);
 
-	spin_unlock(&anon_vma->lock);
+	read_unlock(&anon_vma->rwlock);
 }
 
 /*

On (14/09/07 16:54), Lee Schermerhorn didst pronounce:

Quoted 9 lines

[PATCH/RFC] 01/14 Reclaim Scalability: Convert anon_vma list lock a read/write lock

Against 2.6.23-rc4-mm1

Make the anon_vma list lock a read/write lock. Heaviest use of this lock is in the page_referenced()/try_to_unmap() calls from vmscan [shrink_page_list()]. These functions can use a read lock to allow some parallelism for different cpus trying to reclaim pages mapped via the same set of vmas.

In light of what Peter and Linus said about rw-locks being more expensive than spinlocks, we'll need to measure this with some benchmark. The plus side is that this patch can be handled in isolation because it's either a scalability fix or it isn't. It's worth investigating because you say it fixed a real problem where under load the job was able to complete with this patch and live-locked without it.

When you decide on a test-case, I can test just this patch and see what results I find.

Quoted 10 lines · On Mon, 17 Sep 2007 12:02:35 +0100 [email protected] (Mel Gorman) wrote:

On (14/09/07 16:54), Lee Schermerhorn didst pronounce:

Quoted 9 lines

[PATCH/RFC] 01/14 Reclaim Scalability: Convert anon_vma list lock a read/write lock

Against 2.6.23-rc4-mm1

Make the anon_vma list lock a read/write lock. Heaviest use of this lock is in the page_referenced()/try_to_unmap() calls from vmscan [shrink_page_list()]. These functions can use a read lock to allow some parallelism for different cpus trying to reclaim pages mapped via the same set of vmas.

<snip>

Quoted 10 lines

In light of what Peter and Linus said about rw-locks being more expensive than spinlocks, we'll need to measure this with some benchmark. The plus side is that this patch can be handled in isolation because it's either a scalability fix or it isn't. It's worth investigating because you say it fixed a real problem where under load the job was able to complete with this patch and live-locked without it.

When you decide on a test-case, I can test just this patch and see what results I find.

One of the case I can imagine is..
==
1. Use NUMA.
2. create *large* anon_vma and use it with MPOL_INTERLEAVE
3. When memory is exhausted (on several nodes), all kswapd on nodes will see one anon_vma->lock.
==
Maybe the worst case.

On (18/09/07 11:41), KAMEZAWA Hiroyuki didst pronounce:

Quoted 24 lines

Quoted 10 lines · On Mon, 17 Sep 2007 12:02:35 +0100 [email protected] (Mel Gorman) wrote:

On (14/09/07 16:54), Lee Schermerhorn didst pronounce:

Quoted 9 lines

[PATCH/RFC] 01/14 Reclaim Scalability: Convert anon_vma list lock a read/write lock

Against 2.6.23-rc4-mm1

Make the anon_vma list lock a read/write lock. Heaviest use of this lock is in the page_referenced()/try_to_unmap() calls from vmscan [shrink_page_list()]. These functions can use a read lock to allow some parallelism for different cpus trying to reclaim pages mapped via the same set of vmas.

<snip>

Quoted 10 lines

In light of what Peter and Linus said about rw-locks being more expensive than spinlocks, we'll need to measure this with some benchmark. The plus side is that this patch can be handled in isolation because it's either a scalability fix or it isn't. It's worth investigating because you say it fixed a real problem where under load the job was able to complete with this patch and live-locked without it.

When you decide on a test-case, I can test just this patch and see what results I find.

Quoted 8 lines

One of the case I can imagine is..
==
1. Use NUMA.
2. create *large* anon_vma and use it with MPOL_INTERLEAVE
3. When memory is exhausted (on several nodes), all kswapd on nodes will see one anon_vma->lock.
==
Maybe the worst case.

It certainly sounds like a bad case. Would be very difficult to measure as part of a test though as latencies in kswapd are not very obvious.

Quoted 10 lines · On Mon, 17 Sep 2007 12:02:35 +0100 [email protected] (Mel Gorman) wrote:

On (14/09/07 16:54), Lee Schermerhorn didst pronounce:

Quoted 9 lines

[PATCH/RFC] 01/14 Reclaim Scalability: Convert anon_vma list lock a read/write lock

Against 2.6.23-rc4-mm1

Make the anon_vma list lock a read/write lock. Heaviest use of this lock is in the page_referenced()/try_to_unmap() calls from vmscan [shrink_page_list()]. These functions can use a read lock to allow some parallelism for different cpus trying to reclaim pages mapped via the same set of vmas.

<snip>

Quoted 10 lines

In light of what Peter and Linus said about rw-locks being more expensive than spinlocks, we'll need to measure this with some benchmark. The plus side is that this patch can be handled in isolation because it's either a scalability fix or it isn't. It's worth investigating because you say it fixed a real problem where under load the job was able to complete with this patch and live-locked without it.

When you decide on a test-case, I can test just this patch and see what results I find.

One of the case I can imagine is..
==
1. Use NUMA.
2. create *large* anon_vma and use it with MPOL_INTERLEAVE
3. When memory is exhausted (on several nodes), all kswapd on nodes will see one anon_vma->lock.
==
Maybe the worst case.

On (14/09/07 16:54), Lee Schermerhorn didst pronounce:

Quoted 23 lines

[PATCH/RFC] 01/14 Reclaim Scalability: Convert anon_vma list lock a read/write lock

Against 2.6.23-rc4-mm1

Make the anon_vma list lock a read/write lock. Heaviest use of this lock is in the page_referenced()/try_to_unmap() calls from vmscan [shrink_page_list()]. These functions can use a read lock to allow some parallelism for different cpus trying to reclaim pages mapped via the same set of vmas.

This change should not change the footprint of the anon_vma in the non-debug case.

Note: I have seen systems livelock with all cpus in reclaim, down in page_referenced_anon() or try_to_unmap_anon() spinning on the anon_vma lock. I have only seen this with the AIM7 benchmark with workloads of 10s of thousands of tasks. All of these tasks are children of a single ancestor, so they all share the same anon_vma for each vm area in their respective mm's. I'm told that Apache can fork thousands of children to handle incoming connections, and I've seen similar livelocks--albeit on the i_mmap_lock [next patch] running 1000s of Oracle users on a large ia64 platform.

Quoted 5 lines

With this patch [along with Rik van Riel's split LRU patch] we were able to see the AIM7 workload start swapping, instead of hanging, for the first time. Same workload DID hang with just Rik's patch, so this patch is apparently useful.

In light of what Peter and Linus said about rw-locks being more expensive than spinlocks, we'll need to measure this with some benchmark. The plus side is that this patch can be handled in isolation because it's either a scalability fix or it isn't. It's worth investigating because you say it fixed a real problem where under load the job was able to complete with this patch and live-locked without it.

kernbench is unlikely to show up anything useful here although it might be worth running anyway just in case. brk_test from aim9 might be useful as it's a micro-benchmark that uses brk() which is a path affected by this patch. As aim7 is exercising this path, it would be interesting to see does it show performance differences in the normal non-stressed case. Other suggestions?

Quoted 37 lines

On (14/09/07 16:54), Lee Schermerhorn didst pronounce:

Quoted 23 lines

[PATCH/RFC] 01/14 Reclaim Scalability: Convert anon_vma list lock a read/write lock

Against 2.6.23-rc4-mm1

Make the anon_vma list lock a read/write lock. Heaviest use of this lock is in the page_referenced()/try_to_unmap() calls from vmscan [shrink_page_list()]. These functions can use a read lock to allow some parallelism for different cpus trying to reclaim pages mapped via the same set of vmas.

This change should not change the footprint of the anon_vma in the non-debug case.

Note: I have seen systems livelock with all cpus in reclaim, down in page_referenced_anon() or try_to_unmap_anon() spinning on the anon_vma lock. I have only seen this with the AIM7 benchmark with workloads of 10s of thousands of tasks. All of these tasks are children of a single ancestor, so they all share the same anon_vma for each vm area in their respective mm's. I'm told that Apache can fork thousands of children to handle incoming connections, and I've seen similar livelocks--albeit on the i_mmap_lock [next patch] running 1000s of Oracle users on a large ia64 platform.

Quoted 5 lines

With this patch [along with Rik van Riel's split LRU patch] we were able to see the AIM7 workload start swapping, instead of hanging, for the first time. Same workload DID hang with just Rik's patch, so this patch is apparently useful.

In light of what Peter and Linus said about rw-locks being more expensive than spinlocks, we'll need to measure this with some benchmark. The plus side is that this patch can be handled in isolation because it's either a scalability fix or it isn't. It's worth investigating because you say it fixed a real problem where under load the job was able to complete with this patch and live-locked without it.

Quoted 5 lines

kernbench is unlikely to show up anything useful here although it might be worth running anyway just in case. brk_test from aim9 might be useful as it's a micro-benchmark that uses brk() which is a path affected by this patch. As aim7 is exercising this path, it would be interesting to see does it show performance differences in the normal non-stressed case. Other suggestions?

As Mel predicted, kernel builds don't seem to be affected by this patch, nor the i_mmap_lock rw_lock patch. Below I've included results for an old ia64 system that I have pretty much exclusive access to. I can't get 23-rc4-mm1 nor rc6-mm1 to boot on an x86_64 [AMD-based] right now--still trying to capture stack trace [not easy from a remote console :-(].

PATCH/RFC 02/14 Reclaim Scalability: make the inode i_mmap_lock a reader/writer lock

Against: 2.6.23-rc4-mm1

I have seen soft cpu lockups in page_referenced_file() due to contention on i_mmap_lock() for different pages. Making the i_mmap_lock a reader/writer lock should increase parallelism in vmscan for file back pages mapped into many address spaces.

Read lock the i_mmap_lock for all usage except:

1) mmap/munmap: linking vma into i_mmap prio_tree or removing
2) unmap_mapping_range: protecting vm_truncate_count

rmap: try_to_unmap_file() required new cond_resched_rwlock(). To reduce code duplication, I recast cond_resched_lock() as a [static inline] wrapper around reworked cond_sched_lock() => __cond_resched_lock(void *lock, int type).
New cond_resched_rwlock() implemented as another wrapper.

Note: This patch is meant to address a situation I've seen running large Oracle OLTP workload--1000s of users--on an large HP ia64 NUMA platform. The system hung, spitting out "soft lockup" messages on the console. Stack traces showed that all cpus were in page_referenced(), as mentioned above. I let the system run overnight in this state--it never recovered before I decided to reboot.

TODO: I've yet to test this patch with the same workload to see what happens. Don't have access to the system now.

Signed-off-by Lee Schermerhorn <[email protected]>

 fs/hugetlbfs/inode.c  |    7 +++--
 fs/inode.c            |    2 -
 fs/revoke.c           |   18 +++++++-------
 include/linux/fs.h    |    2 -
 include/linux/mm.h    |    2 -
 include/linux/sched.h |   17 ++++++++++---
 kernel/fork.c         |    4 +--
 kernel/sched.c        |   64 ++++++++++++++++++++++++++++++++++++++++++--------
 mm/filemap_xip.c      |    4 +--
 mm/fremap.c           |    4 +--
 mm/hugetlb.c          |    8 +++---
 mm/memory.c           |   13 +++++-----
 mm/migrate.c          |    4 +--
 mm/mmap.c             |   18 +++++++-------
 mm/mremap.c           |    4 +--
 mm/rmap.c             |   16 ++++++------
 16 files changed, 123 insertions(+), 64 deletions(-)

Index: Linux/include/linux/fs.h
===================================================================

include/linux/fs.h +1 -1

@@ -506,7 +506,7 @@ struct address_space {
 	unsigned int		i_mmap_writable;/* count VM_SHARED mappings */
 	struct prio_tree_root	i_mmap;		/* tree of private and shared mappings */
 	struct list_head	i_mmap_nonlinear;/*list VM_NONLINEAR mappings */
-	spinlock_t		i_mmap_lock;	/* protect tree, count, list */
+	rwlock_t		i_mmap_lock;	/* protect tree, count, list */
 	unsigned int		truncate_count;	/* Cover race condition with truncate */
 	unsigned long		nrpages;	/* number of total pages */
 	pgoff_t			writeback_index;/* writeback starts here */

Index: Linux/include/linux/mm.h
===================================================================

include/linux/mm.h +1 -1

@@ -684,7 +684,7 @@ struct zap_details {
 	struct address_space *check_mapping;	/* Check page->mapping if set */
 	pgoff_t	first_index;			/* Lowest page->index to unmap */
 	pgoff_t last_index;			/* Highest page->index to unmap */
-	spinlock_t *i_mmap_lock;		/* For unmap_mapping_range: */
+	rwlock_t *i_mmap_lock;			/* For unmap_mapping_range: */
 	unsigned long truncate_count;		/* Compare vm_truncate_count */
 };
 

Index: Linux/fs/inode.c
===================================================================

fs/inode.c +1 -1

@@ -203,7 +203,7 @@ void inode_init_once(struct inode *inode
 	init_rwsem(&inode->i_alloc_sem);
 	INIT_RADIX_TREE(&inode->i_data.page_tree, GFP_ATOMIC);
 	rwlock_init(&inode->i_data.tree_lock);
-	spin_lock_init(&inode->i_data.i_mmap_lock);
+	rwlock_init(&inode->i_data.i_mmap_lock);
 	INIT_LIST_HEAD(&inode->i_data.private_list);
 	spin_lock_init(&inode->i_data.private_lock);
 	INIT_RAW_PRIO_TREE_ROOT(&inode->i_data.i_mmap);

Index: Linux/fs/hugetlbfs/inode.c
===================================================================

fs/hugetlbfs/inode.c +5 -2

@@ -411,6 +411,9 @@ static void hugetlbfs_drop_inode(struct 
 		hugetlbfs_forget_inode(inode);
 }
 
+/*
+ * LOCKING:  __unmap_hugepage_range() requires write lock on i_mmap_lock
+ */
 static inline void
 hugetlb_vmtruncate_list(struct prio_tree_root *root, pgoff_t pgoff)
 {

@@ -445,10 +448,10 @@ static int hugetlb_vmtruncate(struct ino
 	pgoff = offset >> PAGE_SHIFT;
 
 	i_size_write(inode, offset);
-	spin_lock(&mapping->i_mmap_lock);
+	write_lock(&mapping->i_mmap_lock);
 	if (!prio_tree_empty(&mapping->i_mmap))
 		hugetlb_vmtruncate_list(&mapping->i_mmap, pgoff);
-	spin_unlock(&mapping->i_mmap_lock);
+	write_unlock(&mapping->i_mmap_lock);
 	truncate_hugepages(inode, offset);
 	return 0;
 }

Index: Linux/fs/revoke.c
===================================================================

fs/revoke.c +9 -9

@@ -272,7 +272,7 @@ static int revoke_break_cow(struct files
 
 /*
  *	 LOCKING: down_write(&mm->mmap_sem)
- *	 	    -> spin_lock(&mapping->i_mmap_lock)
+ *	 	    -> write_lock(&mapping->i_mmap_lock)
  */
 static int revoke_vma(struct vm_area_struct *vma, struct zap_details *details)
 {

@@ -298,14 +298,14 @@ static int revoke_vma(struct vm_area_str
 	return 0;
 
   out_need_break:
-	spin_unlock(details->i_mmap_lock);
+	write_unlock(details->i_mmap_lock);
 	cond_resched();
-	spin_lock(details->i_mmap_lock);
+	write_lock(details->i_mmap_lock);
 	return -EINTR;
 }
 
 /*
- *	LOCKING: spin_lock(&mapping->i_mmap_lock)
+ *	LOCKING: write_lock(&mapping->i_mmap_lock)
  */
 static int revoke_mm(struct mm_struct *mm, struct address_space *mapping,
 		     struct file *to_exclude)

@@ -335,7 +335,7 @@ static int revoke_mm(struct mm_struct *m
 		if (err)
 			break;
 
-		__unlink_file_vma(vma);
+		__unlink_file_vma(vma);	/* requires write_lock(i_mmap_lock) */
 		fput(vma->vm_file);
 		vma->vm_file = NULL;
 	}

@@ -345,7 +345,7 @@ static int revoke_mm(struct mm_struct *m
 }
 
 /*
- *	LOCKING: spin_lock(&mapping->i_mmap_lock)
+ *	LOCKING: write_lock(&mapping->i_mmap_lock)
  */
 static void revoke_mapping_tree(struct address_space *mapping,
 				struct file *to_exclude)

@@ -377,7 +377,7 @@ static void revoke_mapping_tree(struct a
 }
 
 /*
- *	LOCKING: spin_lock(&mapping->i_mmap_lock)
+ *	LOCKING: write_lock(&mapping->i_mmap_lock)
  */
 static void revoke_mapping_list(struct address_space *mapping,
 				struct file *to_exclude)

@@ -408,12 +408,12 @@ static void revoke_mapping_list(struct a
 
 static void revoke_mapping(struct address_space *mapping, struct file *to_exclude)
 {
-	spin_lock(&mapping->i_mmap_lock);
+	write_lock(&mapping->i_mmap_lock);
 	if (unlikely(!prio_tree_empty(&mapping->i_mmap)))
 		revoke_mapping_tree(mapping, to_exclude);
 	if (unlikely(!list_empty(&mapping->i_mmap_nonlinear)))
 		revoke_mapping_list(mapping, to_exclude);
-	spin_unlock(&mapping->i_mmap_lock);
+	write_unlock(&mapping->i_mmap_lock);
 }
 
 static void restore_file(struct revokefs_inode_info *info)

Index: Linux/kernel/fork.c
===================================================================

kernel/fork.c +2 -2

@@ -262,12 +262,12 @@ static inline int dup_mmap(struct mm_str
 				atomic_dec(&inode->i_writecount);
 
 			/* insert tmp into the share list, just after mpnt */
-			spin_lock(&file->f_mapping->i_mmap_lock);
+			write_lock(&file->f_mapping->i_mmap_lock);
 			tmp->vm_truncate_count = mpnt->vm_truncate_count;
 			flush_dcache_mmap_lock(file->f_mapping);
 			vma_prio_tree_add(tmp, mpnt);
 			flush_dcache_mmap_unlock(file->f_mapping);
-			spin_unlock(&file->f_mapping->i_mmap_lock);
+			write_unlock(&file->f_mapping->i_mmap_lock);
 		}
 
 		/*

Index: Linux/mm/filemap_xip.c
===================================================================

mm/filemap_xip.c +2 -2

@@ -182,7 +182,7 @@ __xip_unmap (struct address_space * mapp
 	if (!page)
 		return;
 
-	spin_lock(&mapping->i_mmap_lock);
+	read_lock(&mapping->i_mmap_lock);
 	vma_prio_tree_foreach(vma, &iter, &mapping->i_mmap, pgoff, pgoff) {
 		mm = vma->vm_mm;
 		address = vma->vm_start +

@@ -200,7 +200,7 @@ __xip_unmap (struct address_space * mapp
 			page_cache_release(page);
 		}
 	}
-	spin_unlock(&mapping->i_mmap_lock);
+	read_unlock(&mapping->i_mmap_lock);
 }
 
 /*

Index: Linux/mm/fremap.c
===================================================================

mm/fremap.c +2 -2

@@ -200,13 +200,13 @@ asmlinkage long sys_remap_file_pages(uns
 			}
 			goto out;
 		}
-		spin_lock(&mapping->i_mmap_lock);
+		write_lock(&mapping->i_mmap_lock);
 		flush_dcache_mmap_lock(mapping);
 		vma->vm_flags |= VM_NONLINEAR;
 		vma_prio_tree_remove(vma, &mapping->i_mmap);
 		vma_nonlinear_insert(vma, &mapping->i_mmap_nonlinear);
 		flush_dcache_mmap_unlock(mapping);
-		spin_unlock(&mapping->i_mmap_lock);
+		write_unlock(&mapping->i_mmap_lock);
 	}
 
 	err = populate_range(mm, vma, start, size, pgoff);

Index: Linux/mm/hugetlb.c
===================================================================

mm/hugetlb.c +4 -4

@@ -451,9 +451,9 @@ void unmap_hugepage_range(struct vm_area
 	 * do nothing in this case.
 	 */
 	if (vma->vm_file) {
-		spin_lock(&vma->vm_file->f_mapping->i_mmap_lock);
+		write_lock(&vma->vm_file->f_mapping->i_mmap_lock);
 		__unmap_hugepage_range(vma, start, end);
-		spin_unlock(&vma->vm_file->f_mapping->i_mmap_lock);
+		write_unlock(&vma->vm_file->f_mapping->i_mmap_lock);
 	}
 }
 

@@ -693,7 +693,7 @@ void hugetlb_change_protection(struct vm
 	BUG_ON(address >= end);
 	flush_cache_range(vma, address, end);
 
-	spin_lock(&vma->vm_file->f_mapping->i_mmap_lock);
+	read_lock(&vma->vm_file->f_mapping->i_mmap_lock);
 	spin_lock(&mm->page_table_lock);
 	for (; address < end; address += HPAGE_SIZE) {
 		ptep = huge_pte_offset(mm, address);

@@ -708,7 +708,7 @@ void hugetlb_change_protection(struct vm
 		}
 	}
 	spin_unlock(&mm->page_table_lock);
-	spin_unlock(&vma->vm_file->f_mapping->i_mmap_lock);
+	read_unlock(&vma->vm_file->f_mapping->i_mmap_lock);
 
 	flush_tlb_range(vma, start, end);
 }

Index: Linux/mm/memory.c
===================================================================

mm/memory.c +2 -2

@@ -816,7 +816,7 @@ unsigned long unmap_vmas(struct mmu_gath
 	unsigned long tlb_start = 0;	/* For tlb_finish_mmu */
 	int tlb_start_valid = 0;
 	unsigned long start = start_addr;
-	spinlock_t *i_mmap_lock = details? details->i_mmap_lock: NULL;
+	rwlock_t *i_mmap_lock = details? details->i_mmap_lock: NULL;
 	int fullmm = (*tlbp)->fullmm;
 
 	for ( ; vma && vma->vm_start < end_addr; vma = vma->vm_next) {

@@ -1728,7 +1728,7 @@ unwritable_page:
  * can't efficiently keep all vmas in step with mapping->truncate_count:
  * so instead reset them all whenever it wraps back to 0 (then go to 1).
  * mapping->truncate_count and vma->vm_truncate_count are protected by
- * i_mmap_lock.
+ * write locked i_mmap_lock.
  *
  * In order to make forward progress despite repeatedly restarting some
  * large vma, note the restart_addr from unmap_vmas when it breaks out:

mm/memory.c +2 -2

 	return -EINTR;
 }
 

@@ -1891,7 +1892,7 @@ void unmap_mapping_range(struct address_
 		details.last_index = ULONG_MAX;
 	details.i_mmap_lock = &mapping->i_mmap_lock;
 
-	spin_lock(&mapping->i_mmap_lock);
+	write_lock(&mapping->i_mmap_lock);
 
 	/* Protect against endless unmapping loops */
 	mapping->truncate_count++;

@@ -1906,7 +1907,7 @@ void unmap_mapping_range(struct address_
 		unmap_mapping_range_tree(&mapping->i_mmap, &details);
 	if (unlikely(!list_empty(&mapping->i_mmap_nonlinear)))
 		unmap_mapping_range_list(&mapping->i_mmap_nonlinear, &details);
-	spin_unlock(&mapping->i_mmap_lock);
+	write_unlock(&mapping->i_mmap_lock);
 }
 EXPORT_SYMBOL(unmap_mapping_range);
 

Index: Linux/mm/migrate.c
===================================================================

mm/migrate.c +2 -2

@@ -207,12 +207,12 @@ static void remove_file_migration_ptes(s
 	if (!mapping)
 		return;
 
-	spin_lock(&mapping->i_mmap_lock);
+	read_lock(&mapping->i_mmap_lock);
 
 	vma_prio_tree_foreach(vma, &iter, &mapping->i_mmap, pgoff, pgoff)
 		remove_migration_pte(vma, old, new);
 
-	spin_unlock(&mapping->i_mmap_lock);
+	read_unlock(&mapping->i_mmap_lock);
 }
 
 /*

Index: Linux/mm/mmap.c
===================================================================

mm/mmap.c +9 -9

@@ -182,7 +182,7 @@ error:
 }
 
 /*
- * Requires inode->i_mapping->i_mmap_lock
+ * Requires write locked inode->i_mapping->i_mmap_lock
  */
 static void __remove_shared_vm_struct(struct vm_area_struct *vma,
 		struct file *file, struct address_space *mapping)

@@ -201,7 +201,7 @@ static void __remove_shared_vm_struct(st
 }
 
 /*
- * Requires inode->i_mapping->i_mmap_lock
+ * Requires write locked inode->i_mapping->i_mmap_lock
  */
 void __unlink_file_vma(struct vm_area_struct *vma)
 {

@@ -221,9 +221,9 @@ void unlink_file_vma(struct vm_area_stru
 
 	if (file) {
 		struct address_space *mapping = file->f_mapping;
-		spin_lock(&mapping->i_mmap_lock);
+		write_lock(&mapping->i_mmap_lock);
 		__remove_shared_vm_struct(vma, file, mapping);
-		spin_unlock(&mapping->i_mmap_lock);
+		write_unlock(&mapping->i_mmap_lock);
 	}
 }
 

@@ -445,7 +445,7 @@ static void vma_link(struct mm_struct *m
 		mapping = vma->vm_file->f_mapping;
 
 	if (mapping) {
-		spin_lock(&mapping->i_mmap_lock);
+		write_lock(&mapping->i_mmap_lock);
 		vma->vm_truncate_count = mapping->truncate_count;
 	}
 	anon_vma_lock(vma);

@@ -455,7 +455,7 @@ static void vma_link(struct mm_struct *m
 
 	anon_vma_unlock(vma);
 	if (mapping)
-		spin_unlock(&mapping->i_mmap_lock);
+		write_unlock(&mapping->i_mmap_lock);
 
 	mm->map_count++;
 	validate_mm(mm);

@@ -542,7 +542,7 @@ again:			remove_next = 1 + (end > next->
 		mapping = file->f_mapping;
 		if (!(vma->vm_flags & VM_NONLINEAR))
 			root = &mapping->i_mmap;
-		spin_lock(&mapping->i_mmap_lock);
+		write_lock(&mapping->i_mmap_lock);
 		if (importer &&
 		    vma->vm_truncate_count != next->vm_truncate_count) {
 			/*

@@ -626,7 +626,7 @@ again:			remove_next = 1 + (end > next->
 	if (anon_vma)
 		write_unlock(&anon_vma->rwlock);
 	if (mapping)
-		spin_unlock(&mapping->i_mmap_lock);
+		write_unlock(&mapping->i_mmap_lock);
 
 	if (remove_next) {
 		if (file)

@@ -2064,7 +2064,7 @@ void exit_mmap(struct mm_struct *mm)
 
 /* Insert vm structure into process list sorted by address
  * and into the inode's i_mmap tree.  If vm_file is non-NULL
- * then i_mmap_lock is taken here.
+ * then i_mmap_lock is write locked here.
  */
 int insert_vm_struct(struct mm_struct * mm, struct vm_area_struct * vma)
 {

Index: Linux/mm/mremap.c
===================================================================

mm/mremap.c +2 -2

@@ -83,7 +83,7 @@ static void move_ptes(struct vm_area_str
 		 * and we propagate stale pages into the dst afterward.
 		 */
 		mapping = vma->vm_file->f_mapping;
-		spin_lock(&mapping->i_mmap_lock);
+		read_lock(&mapping->i_mmap_lock);
 		if (new_vma->vm_truncate_count &&
 		    new_vma->vm_truncate_count != vma->vm_truncate_count)
 			new_vma->vm_truncate_count = 0;

@@ -115,7 +115,7 @@ static void move_ptes(struct vm_area_str
 	pte_unmap_nested(new_pte - 1);
 	pte_unmap_unlock(old_pte - 1, old_ptl);
 	if (mapping)
-		spin_unlock(&mapping->i_mmap_lock);
+		read_unlock(&mapping->i_mmap_lock);
 }
 
 #define LATENCY_LIMIT	(64 * PAGE_SIZE)

Index: Linux/mm/rmap.c
===================================================================

mm/rmap.c +8 -8

@@ -365,7 +365,7 @@ static int page_referenced_file(struct p
 	 */
 	BUG_ON(!PageLocked(page));
 
-	spin_lock(&mapping->i_mmap_lock);
+	read_lock(&mapping->i_mmap_lock);
 
 	/*
 	 * i_mmap_lock does not stabilize mapcount at all, but mapcount

@@ -391,7 +391,7 @@ static int page_referenced_file(struct p
 			break;
 	}
 
-	spin_unlock(&mapping->i_mmap_lock);
+	read_unlock(&mapping->i_mmap_lock);
 	return referenced;
 }
 

@@ -472,12 +472,12 @@ static int page_mkclean_file(struct addr
 
 	BUG_ON(PageAnon(page));
 
-	spin_lock(&mapping->i_mmap_lock);
+	read_lock(&mapping->i_mmap_lock);
 	vma_prio_tree_foreach(vma, &iter, &mapping->i_mmap, pgoff, pgoff) {
 		if (vma->vm_flags & VM_SHARED)
 			ret += page_mkclean_one(page, vma);
 	}
-	spin_unlock(&mapping->i_mmap_lock);
+	read_unlock(&mapping->i_mmap_lock);
 	return ret;
 }
 

@@ -904,7 +904,7 @@ static int try_to_unmap_file(struct page
 	unsigned long max_nl_size = 0;
 	unsigned int mapcount;
 
-	spin_lock(&mapping->i_mmap_lock);
+	read_lock(&mapping->i_mmap_lock);
 	vma_prio_tree_foreach(vma, &iter, &mapping->i_mmap, pgoff, pgoff) {
 		ret = try_to_unmap_one(page, vma, migration);
 		if (ret == SWAP_FAIL || !page_mapped(page))

@@ -941,7 +941,7 @@ static int try_to_unmap_file(struct page
 	mapcount = page_mapcount(page);
 	if (!mapcount)
 		goto out;
-	cond_resched_lock(&mapping->i_mmap_lock);
+	cond_resched_rwlock(&mapping->i_mmap_lock, 0);
 
 	max_nl_size = (max_nl_size + CLUSTER_SIZE - 1) & CLUSTER_MASK;
 	if (max_nl_cursor == 0)

@@ -963,7 +963,7 @@ static int try_to_unmap_file(struct page
 			}
 			vma->vm_private_data = (void *) max_nl_cursor;
 		}
-		cond_resched_lock(&mapping->i_mmap_lock);
+		cond_resched_rwlock(&mapping->i_mmap_lock, 0);
 		max_nl_cursor += CLUSTER_SIZE;
 	} while (max_nl_cursor <= max_nl_size);
 

@@ -975,7 +975,7 @@ static int try_to_unmap_file(struct page
 	list_for_each_entry(vma, &mapping->i_mmap_nonlinear, shared.vm_set.list)
 		vma->vm_private_data = NULL;
 out:
-	spin_unlock(&mapping->i_mmap_lock);
+	read_unlock(&mapping->i_mmap_lock);
 	return ret;
 }
 

Index: Linux/include/linux/sched.h
===================================================================

include/linux/sched.h +14 -3

@@ -1823,12 +1823,23 @@ static inline int need_resched(void)
  * cond_resched() and cond_resched_lock(): latency reduction via
  * explicit rescheduling in places that are safe. The return
  * value indicates whether a reschedule was done in fact.
- * cond_resched_lock() will drop the spinlock before scheduling,
- * cond_resched_softirq() will enable bhs before scheduling.
+ * cond_resched_softirq() will enable bhs before scheduling,
+ * cond_resched_*lock() will drop the *lock before scheduling.
  */
 extern int cond_resched(void);
-extern int cond_resched_lock(spinlock_t * lock);
 extern int cond_resched_softirq(void);
+extern int __cond_resched_lock(void * lock, int lock_type);
+
+#define COND_RESCHED_SPIN  2
+static inline int cond_resched_lock(spinlock_t * lock)
+{
+	return __cond_resched_lock(lock, COND_RESCHED_SPIN);
+}
+
+static inline int cond_resched_rwlock(rwlock_t * lock, int write_lock)
+{
+	return __cond_resched_lock(lock, !!write_lock);
+}
 
 /*
  * Does a critical section need to be broken due to another

Index: Linux/kernel/sched.c
===================================================================

kernel/sched.c +52 -8

@@ -4635,34 +4635,78 @@ int __sched cond_resched(void)
 EXPORT_SYMBOL(cond_resched);
 
 /*
- * cond_resched_lock() - if a reschedule is pending, drop the given lock,
+ * helper functions for __cond_resched_lock()
+ */
+static int __need_lockbreak(void *lock, int type)
+{
+	if (likely(type == COND_RESCHED_SPIN))
+		return need_lockbreak((spinlock_t *)lock);
+	else
+		return need_lockbreak((rwlock_t *)lock);
+}
+
+static void __reacquire_lock(void *lock, int type)
+{
+	if (likely(type == COND_RESCHED_SPIN))
+		spin_lock((spinlock_t *)lock);
+	else if (type)
+		write_unlock((rwlock_t *)lock);
+	else
+		read_unlock((rwlock_t *)lock);
+}
+
+static void __drop_lock(void *lock, int type)
+{
+	if (likely(type == COND_RESCHED_SPIN))
+		spin_unlock((spinlock_t *)lock);
+	else if (type)
+		write_unlock((rwlock_t *)lock);
+	else
+		read_unlock((rwlock_t *)lock);
+}
+
+static void __release_lock(void *lock, int type)
+{
+	if (likely(type == COND_RESCHED_SPIN))
+		spin_release(&(spinlock_t *)lock->dep_map, 1, _RET_IP_);
+	else
+		rwlock_release(&(rwlock_t *)lock->dep_map, 1, _RET_IP_);
+}
+
+/*
+ * __cond_resched_lock() - if a reschedule is pending, drop the given lock,
  * call schedule, and on return reacquire the lock.
  *
+ * Lock type:
+ *  0 = rwlock held for read
+ *  1 = rwlock held for write
+ *  2 = COND_RESCHED_SPIN = spinlock
+ *
  * This works OK both with and without CONFIG_PREEMPT.  We do strange low-level
  * operations here to prevent schedule() from being called twice (once via
- * spin_unlock(), once by hand).
+ * *_unlock(), once by hand).
  */
-int cond_resched_lock(spinlock_t *lock)
+int __cond_resched_lock(void *lock, int type)
 {
 	int ret = 0;
 
-	if (need_lockbreak(lock)) {
-		spin_unlock(lock);
+	if (__need_lockbreak(lock, type)) {
+		__drop_lock(lock, type);
 		cpu_relax();
 		ret = 1;
-		spin_lock(lock);
+		__reacquire_lock(lock, type);
 	}
 	if (need_resched() && system_state == SYSTEM_RUNNING) {
-		spin_release(&lock->dep_map, 1, _THIS_IP_);
-		_raw_spin_unlock(lock);
+		__release_lock(lock, type);
+		__drop_lock(lock, type);
 		preempt_enable_no_resched();

kernel/sched.c +0 -0

 int __sched cond_resched_softirq(void)
 {

Note: This patch is meant to address a situation I've seen running large Oracle OLTP workload--1000s of users--on an large HP ia64 NUMA platform. The system hung, spitting out "soft lockup" messages on the console. Stack traces showed that all cpus were in page_referenced(), as mentioned above. I let the system run overnight in this state--it never recovered before I decided to reboot.

Quoted 7 lines · Lee Schermerhorn

Note: This patch is meant to address a situation I've seen running large Oracle OLTP workload--1000s of users--on an large HP ia64 NUMA platform. The system hung, spitting out "soft lockup" messages on the console. Stack traces showed that all cpus were in page_referenced(), as mentioned above. I let the system run overnight in this state--it never recovered before I decided to reboot.

Just to understand better, was that an oom condition? Can you press SYSRQ+M to check the RAM and swap levels? If it's an oom condition the problem may be quite different.

Still making those spinlocks rw sounds good to me.

Actually, the system never went OOM. Didn't get that far. I was trying to create an Oracle workload that would put me at the brink of reclaim, and then by running some app that would eat page cache, push it over the edge. But, I apparently went too far--too many Oracle users for this system--and it went into reclaim, got hung up with all cpus spinning on the i_mmap_lock in page_referenced_file().

I just got this system back for testing. Soon as I build a 23-rc6-mm1 kernel for it, I'll retest that with the same workload to demonstrate the problem. Then I'll try it with the rw_lock patch to see if that helps.

Well, except for the concern about the extra overhead of rw_locks. I'm more worried about this for the i_mmap_lock than the anon_vma lock. The only time we need to take the anon_vma lock for write is when adding a new vma to the list, or removing one [vma_link(), et al]. But, the i_mmap_lock is also used to protect the truncate_count, and must be taken for write there. I expected that a kernel build might show something with all the forks for parallel make, mapping of libc, cc executable, ... but nothing.

Note that we now have '__isolate_lru_page()', that does something quite different, visible outside of vmscan.c for use with memory controller. Methinks we need to
rationalize these names/purposes. --lts

/*
- * Isolate one page from the LRU lists. If successful put it onto
- * the indicated list with elevated page count.
- *
- * Result:
- * -EBUSY: page not on LRU list
- * 0: page removed from LRU list and added to the specified list.
- */

mm/migrate.c +0 -11

-int isolate_lru_page(struct page *page, struct list_head *pagelist)
-{
-	int ret = -EBUSY;
-
-	if (PageLRU(page)) {
-		struct zone *zone = page_zone(page);
-
-		spin_lock_irq(&zone->lru_lock);
-		if (PageLRU(page) && get_page_unless_zero(page)) {
-			ret = 0;
-			ClearPageLRU(page);

mm/vmscan.c +30 -0

+/**
+ * isolate_lru_page(@page)
+ *
+ * Isolate one @page from the LRU lists. Must be called with an elevated
+ * refcount on the page, which is a fundamentnal difference from
+ * isolate_lru_pages (which is called without a stable reference).
+ *
+ * The returned page will have PageLru() cleared, and PageActive set,
+ * if it was found on the active list. This flag generally will need to be
+ * cleared by the caller before letting the page go.
+ *
+ * The vmstat page counts corresponding to the list on which the page was
+ * found will be decremented.
+ *
+ * lru_lock must not be held, interrupts must be enabled.
+ *
+ * Returns:
+ *  -EBUSY: page not on LRU list
+ *  0: page removed from LRU list.
+ */
+int isolate_lru_page(struct page *page)
+{
+	int ret = -EBUSY;
+
+	if (PageLRU(page)) {
+		struct zone *zone = page_zone(page);
+
+		spin_lock_irq(&zone->lru_lock);
+		if (PageLRU(page)) {
+			ret = 0;

remarcable change is the dissapearance of get_page_unless_zero() in the new version.

Quoted 5 lines · Lee Schermerhorn

Note that we now have '__isolate_lru_page()', that does something quite different, visible outside of vmscan.c for use with memory controller. Methinks we need to
rationalize these names/purposes. --lts

Actually it comes from lumpy reclaim, and does something very similar to what this one does.

When one looks at the mainline version one could
write:

int isolate_lru_page(struct page *page, struct list_head *pagelist)

{
	int ret = -EBUSY;

	if (PageLRU(page)) {
		struct zone *zone = page_zone(page);

		spin_lock_irq(&zone->lru_lock);
		ret = __isolate_lru_page(page, ISOLATE_BOTH);
		if (!ret) {
			__dec_zone_state(zone, PageActive(page) 

				? NR_ACTIVE : NR_INACTIVE);
			list_move_tail(&page->lru, pagelist);
		}
		spin_unlock_irq(&zone->lru_lock);
	}

	return ret;
}

Obviously the container stuff somewhat complicates mattters in -mm.

Quoted 24 lines

/*
- * Isolate one page from the LRU lists. If successful put it onto
- * the indicated list with elevated page count.
- *
- * Result:
- * -EBUSY: page not on LRU list
- * 0: page removed from LRU list and added to the specified list.
- */

(quoted diff) +0 -16

-int isolate_lru_page(struct page *page, struct list_head *pagelist)
-{
-	int ret = -EBUSY;
-
-	if (PageLRU(page)) {
-		struct zone *zone = page_zone(page);
-
-		spin_lock_irq(&zone->lru_lock);
-		if (PageLRU(page) && get_page_unless_zero(page)) {
-			ret = 0;
-			ClearPageLRU(page);
-			if (PageActive(page))
-				del_page_from_active_list(zone, page);
-			else
-				del_page_from_inactive_list(zone, page);
-			list_add_tail(&page->lru, pagelist);

Review

Quoted 5 lines

(quoted diff) +0 -5

-		}
-		spin_unlock_irq(&zone->lru_lock);
-	}
-	return ret;
-}

remarcable change is the dissapearance of get_page_unless_zero() in the new version.

mm/vmscan.c +6 -0

+int isolate_lru_page(struct page *page)
+{
+	int ret = -EBUSY;
+
+	if (PageLRU(page)) {
+		struct zone *zone = page_zone(page);

Quoted 12 lines · Lee Schermerhorn

mm/vmscan.c +12 -0

+int isolate_lru_page(struct page *page)
+{
+	int ret = -EBUSY;
+
+	if (PageLRU(page)) {
+		struct zone *zone = page_zone(page);
+
+		spin_lock_irq(&zone->lru_lock);
+		if (PageLRU(page)) {
+			ret = 0;
+			ClearPageLRU(page);
+			if (PageActive(page))

mm/vmscan.c static unsigned long clear_active_flags( · line 848

Quoted 4 lines

mm/vmscan.c +4 -0

+				del_page_from_active_list(zone, page);
+			else
+				del_page_from_inactive_list(zone, page);
+		}

Wouldn't using a pagelist as an argument and moving to that be easier? Are there any cases where we just remove from the list and not move it elsewhere?

mm/vmscan.c static unsigned long clear_active_flags( · line 853

Quoted 6 lines

mm/vmscan.c +5 -0

+		spin_unlock_irq(&zone->lru_lock);
+	}
+	return ret;
+}
+

Any chance we could merge __isolate_lru_page() and isolate_lru_page()?

PATCH/RFC 03/14 Reclaim Scalability: Define page_anon() function
to answer the question: is page backed by swap space?

Against: 2.6.23-rc4-mm1

Originally part of Rik van Riel's split-lru patch. Extracted to make available for other, independent reclaim patches.

Originally posted, but not Signed-off-by: Rik van Riel <[email protected]>

Signed-off-by Lee Schermerhorn <[email protected]>

PATCH/RFC 03/14 Reclaim Scalability: Define page_anon() function
to answer the question: is page backed by swap space?

Against: 2.6.23-rc4-mm1

Originally part of Rik van Riel's split-lru patch. Extracted to make available for other, independent reclaim patches.

Moved page_anon() inline function to linux/mm_inline.h where it will be needed by subsequent "split LRU" and "noreclaim" patches.

page_anon() requires the definition of struct address_space() from linux/fs.h. A patch in 2.6.23-rc1-mm* removed the include of fs.h
from linux/mm.h in favor of including it where it's needed. Add it
back to mm_inline.h, which is included from very few places. These include all the places where page_anon() is needed--so far.

Originally posted, but not Signed-off-by: Rik van Riel <[email protected]>

Signed-off-by Lee Schermerhorn <[email protected]>

 include/linux/mm_inline.h |   29 +++++++++++++++++++++++++++++
 mm/shmem.c                |    4 ++--
 2 files changed, 31 insertions(+), 2 deletions(-)

Index: Linux/include/linux/mm_inline.h
===================================================================

include/linux/mm_inline.h +13 -0

@@ -1,3 +1,31 @@
+#ifndef LINUX_MM_INLINE_H
+#define LINUX_MM_INLINE_H
+
+#include <linux/fs.h> 	/* need struct address_space for page_anon() */
+
+/*
+ * Returns true if this page is anonymous, tmpfs or otherwise swap backed.
+ */
+extern const struct address_space_operations shmem_aops;
+static inline int page_anon(struct page *page)
+{
+	struct address_space *mapping;
+

include/linux/mm_inline.h +6 -0

+
+	/* The page is page cache backed by a normal filesystem. */
+	return 0;
+}
+
 static inline void
 add_page_to_active_list(struct zone *zone, struct page *page)
 {

@@ -38,3 +66,4 @@ del_page_from_lru(struct zone *zone, str
 	}
 }
 
+#endif

Index: Linux/mm/shmem.c
===================================================================

mm/shmem.c +2 -2

@@ -180,7 +180,7 @@ static inline void shmem_unacct_blocks(u
 }
 
 static const struct super_operations shmem_ops;
-static const struct address_space_operations shmem_aops;
+const struct address_space_operations shmem_aops;
 static const struct file_operations shmem_file_operations;
 static const struct inode_operations shmem_inode_operations;
 static const struct inode_operations shmem_dir_inode_operations;

@@ -2353,7 +2353,7 @@ static void destroy_inodecache(void)
 	kmem_cache_destroy(shmem_inode_cachep);
 }
 
-static const struct address_space_operations shmem_aops = {
+const struct address_space_operations shmem_aops = {
 	.writepage	= shmem_writepage,
 	.set_page_dirty	= __set_page_dirty_no_writeback,
 #ifdef CONFIG_TMPFS

include/linux/mm_inline.h +13 -0

+/*
+ * Returns true if this page is anonymous, tmpfs or otherwise swap backed.
+ */
+extern const struct address_space_operations shmem_aops;
+static inline int page_anon(struct page *page)
+{
+	struct address_space *mapping;
+
+	if (PageAnon(page) || PageSwapCache(page))
+		return 1;
+	mapping = page_mapping(page);
+	if (!mapping || !mapping->a_ops)
+		return 0;

Quoted 19 lines · On Fri, 14 Sep 2007 16:54:25 -0400 Lee Schermerhorn <[email protected]>…

include/linux/mm_inline.h +19 -0

+/*
+ * Returns true if this page is anonymous, tmpfs or otherwise swap backed.
+ */
+extern const struct address_space_operations shmem_aops;
+static inline int page_anon(struct page *page)
+{
+	struct address_space *mapping;
+
+	if (PageAnon(page) || PageSwapCache(page))
+		return 1;
+	mapping = page_mapping(page);
+	if (!mapping || !mapping->a_ops)
+		return 0;
+	if (mapping->a_ops == &shmem_aops)
+		return 1;
+	/* Should ramfs pages go onto an mlocked list instead? */
+	if ((unlikely(mapping->a_ops->writepage == NULL && PageDirty(page))))
+		return 1;
+

include/linux/mm_inline.h line 28

Quoted 4 lines

include/linux/mm_inline.h +4 -0

+	/* The page is page cache backed by a normal filesystem. */
+	return 0;
+}
+

Hi, it seems the name 'page_anon()' is not clear..
In my understanding, an anonymous page is a MAP_ANONYMOUS page. Can't we have better name ?

Quoted 4 lines

Hi, it seems the name 'page_anon()' is not clear..
In my understanding, an anonymous page is a MAP_ANONYMOUS page. Can't we have better name ?

The idea is to distinguish pages that are (or could be) swap backed from pages that are filesystem backed.

Quoted 19 lines · On Fri, 14 Sep 2007 16:54:25 -0400 Lee Schermerhorn <[email protected]>…

include/linux/mm_inline.h +19 -0

+/*
+ * Returns true if this page is anonymous, tmpfs or otherwise swap backed.
+ */
+extern const struct address_space_operations shmem_aops;
+static inline int page_anon(struct page *page)
+{
+	struct address_space *mapping;
+
+	if (PageAnon(page) || PageSwapCache(page))
+		return 1;
+	mapping = page_mapping(page);
+	if (!mapping || !mapping->a_ops)
+		return 0;
+	if (mapping->a_ops == &shmem_aops)
+		return 1;
+	/* Should ramfs pages go onto an mlocked list instead? */
+	if ((unlikely(mapping->a_ops->writepage == NULL && PageDirty(page))))
+		return 1;
+

include/linux/mm_inline.h line 28

Quoted 4 lines

include/linux/mm_inline.h +4 -0

+	/* The page is page cache backed by a normal filesystem. */
+	return 0;
+}
+

Hi, it seems the name 'page_anon()' is not clear..
In my understanding, an anonymous page is a MAP_ANONYMOUS page. Can't we have better name ?

Quoted 11 lines

Quoted 11 lines

include/linux/mm_inline.h +11 -0

+static inline int page_anon(struct page *page)
+{
+	struct address_space *mapping;
+
+	if (PageAnon(page) || PageSwapCache(page))
+		return 1;
+	mapping = page_mapping(page);
+	if (!mapping || !mapping->a_ops)
+		return 0;
+	if (mapping->a_ops == &shmem_aops)
+		return 1;

Quoted 7 lines

include/linux/mm_inline.h line 27

Quoted 7 lines

include/linux/mm_inline.h +7 -0

+	/* Should ramfs pages go onto an mlocked list instead? */
+	if ((unlikely(mapping->a_ops->writepage == NULL && PageDirty(page))))
+		return 1;
+
+	/* The page is page cache backed by a normal filesystem. */
+	return 0;
+}

Other ideas?

Quoted 3 lines

Hi, it seems the name 'page_anon()' is not clear..
In my understanding, an anonymous page is a MAP_ANONYMOUS page. Can't we have better name ?

Hi, Kame-san:

I'm open to a "better name". Probably Rik, too -- it's his original name.

How about one of these?

- page_is_swap_backed() or page_is_backed_by_swap_space()
- page_needs_swap_space() or page_uses_swap_space()
- pageNeedSwapSpaceToBeReclaimable() [X11-style :-)]

Quoted 11 lines · On Tue, 18 Sep 2007 11:04:46 -0400 Lee Schermerhorn <[email protected]>…

Quoted 3 lines

Hi, it seems the name 'page_anon()' is not clear..
In my understanding, an anonymous page is a MAP_ANONYMOUS page. Can't we have better name ?

Hi, Kame-san:

I'm open to a "better name". Probably Rik, too -- it's his original name.

How about one of these?

Quoted 4 lines

- page_is_swap_backed() or page_is_backed_by_swap_space()
- page_needs_swap_space() or page_uses_swap_space()
- pageNeedSwapSpaceToBeReclaimable() [X11-style :-)]

My point is that the word "anonymous" is traditionally used for user's work memory. and page_anon() page seems not to be swap-backed always. (you includes ramfs etc..)

Hmm...how about page_anon_cache() ?

But finally, please name it as you like. sorry for nitpicks.

Quoted 18 lines · KAMEZAWA Hiroyuki

Quoted 15 lines · On Tue, 18 Sep 2007 11:04:46 -0400 Lee Schermerhorn <[email protected]>…

Quoted 3 lines

Hi, it seems the name 'page_anon()' is not clear..
In my understanding, an anonymous page is a MAP_ANONYMOUS page. Can't we have better name ?

Hi, Kame-san:

I'm open to a "better name". Probably Rik, too -- it's his original name.

How about one of these?

- page_is_swap_backed() or page_is_backed_by_swap_space()
- page_needs_swap_space() or page_uses_swap_space()
- pageNeedSwapSpaceToBeReclaimable() [X11-style :-)]

Quoted 3 lines

My point is that the word "anonymous" is traditionally used for user's work memory. and page_anon() page seems not to be swap-backed always. (you includes ramfs etc..)

I did understand your point. Sorry if my response was confusing.

Next respin will remove ramfs. That will be treated as a non-reclaimable address space.

[PATCH/RFC] 05/15 Reclaim Scalability: Use an indexed array for LRU variables

From [email protected] Wed Aug 29 11:39:51 2007

Currently we are defining explicit variables for the inactive and active list. An indexed array can be more generic and avoid repeating similar code in several places in the reclaim code.

We are saving a few bytes in terms of code size:

Before:

text data bss dec hex filename
4097753 573120 4092484 8763357 85b7dd vmlinux

After:

text data bss dec hex filename
4097729 573120 4092484 8763333 85b7c5 vmlinux

Having an easy way to add new lru lists may ease future work on the reclaim code.

[CL's signoff added by lts based on mail from CL]

Signed-off-by Christoph Lameter <[email protected]>

 include/linux/mm_inline.h |   33 ++++++++---
 include/linux/mmzone.h    |   17 +++--
 mm/page_alloc.c           |    9 +--
 mm/swap.c                 |    2 
 mm/vmscan.c               |  132 ++++++++++++++++++++++------------------------
 mm/vmstat.c               |    3 -
 6 files changed, 107 insertions(+), 89 deletions(-)

Index: Linux/include/linux/mmzone.h
===================================================================

include/linux/mmzone.h +11 -6

@@ -81,8 +81,8 @@ struct zone_padding {
 enum zone_stat_item {
 	/* First 128 byte cacheline (assuming 64 bit words) */
 	NR_FREE_PAGES,
-	NR_INACTIVE,
-	NR_ACTIVE,
+	NR_INACTIVE,	/* must match order of LRU_[IN]ACTIVE */
+	NR_ACTIVE,	/*  "     "     "   "       "         */
 	NR_ANON_PAGES,	/* Mapped anonymous pages */
 	NR_FILE_MAPPED,	/* pagecache pages mapped into pagetables.
 			   only modified from process context */

@@ -106,6 +106,13 @@ enum zone_stat_item {
 #endif
 	NR_VM_ZONE_STAT_ITEMS };
 
+enum lru_list {
+	LRU_INACTIVE,	/* must match order of NR_[IN]ACTIVE */
+	LRU_ACTIVE,	/*  "     "     "   "       "        */
+	NR_LRU_LISTS };
+
+#define for_each_lru(l) for (l = 0; l < NR_LRU_LISTS; l++)
+
 struct per_cpu_pages {
 	int count;		/* number of pages in the list */
 	int high;		/* high watermark, emptying needed */

@@ -259,10 +266,8 @@ struct zone {
 
 	/* Fields commonly accessed by the page reclaim scanner */
 	spinlock_t		lru_lock;	
-	struct list_head	active_list;
-	struct list_head	inactive_list;
-	unsigned long		nr_scan_active;
-	unsigned long		nr_scan_inactive;
+	struct list_head	list[NR_LRU_LISTS];
+	unsigned long		nr_scan[NR_LRU_LISTS];
 	unsigned long		pages_scanned;	   /* since last reclaim */
 	int			all_unreclaimable; /* All pages pinned */
 

Index: Linux/include/linux/mm_inline.h
===================================================================

include/linux/mm_inline.h +20 -8

@@ -27,43 +27,56 @@ static inline int page_anon(struct page 
 }
 
 static inline void
+add_page_to_lru_list(struct zone *zone, struct page *page, enum lru_list l)
+{
+	list_add(&page->lru, &zone->list[l]);
+	__inc_zone_state(zone, NR_INACTIVE + l);
+}
+
+static inline void
+del_page_from_lru_list(struct zone *zone, struct page *page, enum lru_list l)
+{
+	list_del(&page->lru);
+	__dec_zone_state(zone, NR_INACTIVE + l);
+}
+
+
+static inline void
 add_page_to_active_list(struct zone *zone, struct page *page)
 {
-	list_add(&page->lru, &zone->active_list);
-	__inc_zone_state(zone, NR_ACTIVE);
+	add_page_to_lru_list(zone, page, LRU_ACTIVE);
 }
 
 static inline void
 add_page_to_inactive_list(struct zone *zone, struct page *page)
 {
-	list_add(&page->lru, &zone->inactive_list);
-	__inc_zone_state(zone, NR_INACTIVE);
+	add_page_to_lru_list(zone, page, LRU_INACTIVE);
 }
 
 static inline void
 del_page_from_active_list(struct zone *zone, struct page *page)
 {
-	list_del(&page->lru);
-	__dec_zone_state(zone, NR_ACTIVE);
+	del_page_from_lru_list(zone, page, LRU_ACTIVE);
 }
 
 static inline void
 del_page_from_inactive_list(struct zone *zone, struct page *page)
 {
-	list_del(&page->lru);
-	__dec_zone_state(zone, NR_INACTIVE);
+	del_page_from_lru_list(zone, page, LRU_INACTIVE);
 }
 
 static inline void
 del_page_from_lru(struct zone *zone, struct page *page)
 {
+	enum lru_list l = LRU_INACTIVE;

include/linux/mm_inline.h +0 -0

 }
 
 #endif

Index: Linux/mm/page_alloc.c
===================================================================

mm/page_alloc.c +5 -4

@@ -3422,6 +3422,7 @@ static void __meminit free_area_init_cor
 	for (j = 0; j < MAX_NR_ZONES; j++) {
 		struct zone *zone = pgdat->node_zones + j;
 		unsigned long size, realsize, memmap_pages;
+		enum lru_list l;
 
 		size = zone_spanned_pages_in_node(nid, j, zones_size);
 		realsize = size - zone_absent_pages_in_node(nid, j,

@@ -3471,10 +3472,10 @@ static void __meminit free_area_init_cor
 		zone->prev_priority = DEF_PRIORITY;
 
 		zone_pcp_init(zone);
-		INIT_LIST_HEAD(&zone->active_list);
-		INIT_LIST_HEAD(&zone->inactive_list);
-		zone->nr_scan_active = 0;
-		zone->nr_scan_inactive = 0;
+		for_each_lru(l) {
+			INIT_LIST_HEAD(&zone->list[l]);
+			zone->nr_scan[l] = 0;
+		}
 		zap_zone_vm_stats(zone);
 		atomic_set(&zone->reclaim_in_progress, 0);
 		if (!size)

Index: Linux/mm/swap.c
===================================================================

mm/swap.c +1 -1

@@ -124,7 +124,7 @@ int rotate_reclaimable_page(struct page 
 	zone = page_zone(page);
 	spin_lock_irqsave(&zone->lru_lock, flags);
 	if (PageLRU(page) && !PageActive(page)) {
-		list_move_tail(&page->lru, &zone->inactive_list);
+		list_move_tail(&page->lru, &zone->list[LRU_INACTIVE]);
 		__count_vm_event(PGROTATED);
 	}
 	if (!test_clear_page_writeback(page))

Index: Linux/mm/vmscan.c
===================================================================

mm/vmscan.c +65 -67

@@ -785,10 +785,10 @@ static unsigned long isolate_pages_globa
 					int active)
 {
 	if (active)
-		return isolate_lru_pages(nr, &z->active_list, dst,
+		return isolate_lru_pages(nr, &z->list[LRU_ACTIVE], dst,
 						scanned, order, mode);
 	else
-		return isolate_lru_pages(nr, &z->inactive_list, dst,
+		return isolate_lru_pages(nr, &z->list[LRU_INACTIVE], dst,
 						scanned, order, mode);
 }
 

@@ -928,10 +928,7 @@ static unsigned long shrink_inactive_lis
 			VM_BUG_ON(PageLRU(page));
 			SetPageLRU(page);
 			list_del(&page->lru);
-			if (PageActive(page))
-				add_page_to_active_list(zone, page);
-			else
-				add_page_to_inactive_list(zone, page);
+			add_page_to_lru_list(zone, page, PageActive(page));
 			if (!pagevec_add(&pvec, page)) {
 				spin_unlock_irq(&zone->lru_lock);
 				__pagevec_release(&pvec);

@@ -990,11 +987,14 @@ static void shrink_active_list(unsigned 
 	int pgdeactivate = 0;
 	unsigned long pgscanned;
 	LIST_HEAD(l_hold);	/* The pages which were snipped off */
-	LIST_HEAD(l_inactive);	/* Pages to go onto the inactive_list */
-	LIST_HEAD(l_active);	/* Pages to go onto the active_list */
+	struct list_head list[NR_LRU_LISTS];
 	struct page *page;
 	struct pagevec pvec;
 	int reclaim_mapped = 0;
+	enum lru_list l;
+
+	for_each_lru(l)
+		INIT_LIST_HEAD(&list[l]);
 
 	if (sc->may_swap) {
 		long mapped_ratio;

@@ -1101,28 +1101,28 @@ force_reclaim_mapped:
 			if (!reclaim_mapped ||
 			    (total_swap_pages == 0 && PageAnon(page)) ||
 			    page_referenced(page, 0, sc->mem_container)) {
-				list_add(&page->lru, &l_active);
+				list_add(&page->lru, &list[LRU_ACTIVE]);
 				continue;
 			}
 		} else if (TestClearPageReferenced(page)) {
-			list_add(&page->lru, &l_active);
+			list_add(&page->lru, &list[LRU_ACTIVE]);
 			continue;
 		}
-		list_add(&page->lru, &l_inactive);
+		list_add(&page->lru, &list[LRU_INACTIVE]);
 	}
 
 	pagevec_init(&pvec, 1);
 	pgmoved = 0;
 	spin_lock_irq(&zone->lru_lock);
-	while (!list_empty(&l_inactive)) {
-		page = lru_to_page(&l_inactive);
-		prefetchw_prev_lru_page(page, &l_inactive, flags);
+	while (!list_empty(&list[LRU_INACTIVE])) {
+		page = lru_to_page(&list[LRU_INACTIVE]);
+		prefetchw_prev_lru_page(page, &list[LRU_INACTIVE], flags);
 		VM_BUG_ON(PageLRU(page));
 		SetPageLRU(page);
 		VM_BUG_ON(!PageActive(page));
 		ClearPageActive(page);
 
-		list_move(&page->lru, &zone->inactive_list);
+		list_move(&page->lru, &zone->list[LRU_INACTIVE]);
 		mem_container_move_lists(page_get_page_container(page), false);
 		pgmoved++;
 		if (!pagevec_add(&pvec, page)) {

@@ -1145,13 +1145,13 @@ force_reclaim_mapped:
 	}
 
 	pgmoved = 0;
-	while (!list_empty(&l_active)) {
-		page = lru_to_page(&l_active);
-		prefetchw_prev_lru_page(page, &l_active, flags);
+	while (!list_empty(&list[LRU_ACTIVE])) {
+		page = lru_to_page(&list[LRU_ACTIVE]);
+		prefetchw_prev_lru_page(page, &list[LRU_ACTIVE], flags);
 		VM_BUG_ON(PageLRU(page));
 		SetPageLRU(page);
 		VM_BUG_ON(!PageActive(page));
-		list_move(&page->lru, &zone->active_list);
+		list_move(&page->lru, &zone->list[LRU_ACTIVE]);
 		mem_container_move_lists(page_get_page_container(page), true);
 		pgmoved++;
 		if (!pagevec_add(&pvec, page)) {

@@ -1171,16 +1171,26 @@ force_reclaim_mapped:
 	pagevec_release(&pvec);
 }
 
+static unsigned long shrink_list(enum lru_list l, unsigned long nr_to_scan,
+	struct zone *zone, struct scan_control *sc, int priority)
+{
+	if (l == LRU_ACTIVE) {
+		shrink_active_list(nr_to_scan, zone, sc, priority);
+		return 0;
+	}
+	return shrink_inactive_list(nr_to_scan, zone, sc);
+}
+
 /*
  * This is a basic per-zone page freer.  Used by both kswapd and direct reclaim.
  */
 static unsigned long shrink_zone(int priority, struct zone *zone,
 				struct scan_control *sc)
 {
-	unsigned long nr_active;
-	unsigned long nr_inactive;
+	unsigned long nr[NR_LRU_LISTS];
 	unsigned long nr_to_scan;
 	unsigned long nr_reclaimed = 0;
+	enum lru_list l;
 
 	atomic_inc(&zone->reclaim_in_progress);
 

@@ -1188,36 +1198,26 @@ static unsigned long shrink_zone(int pri
 	 * Add one to `nr_to_scan' just to make sure that the kernel will
 	 * slowly sift through the active list.
 	 */
-	zone->nr_scan_active +=
-		(zone_page_state(zone, NR_ACTIVE) >> priority) + 1;
-	nr_active = zone->nr_scan_active;
-	if (nr_active >= sc->swap_cluster_max)
-		zone->nr_scan_active = 0;
-	else
-		nr_active = 0;
-
-	zone->nr_scan_inactive +=
-		(zone_page_state(zone, NR_INACTIVE) >> priority) + 1;
-	nr_inactive = zone->nr_scan_inactive;
-	if (nr_inactive >= sc->swap_cluster_max)
-		zone->nr_scan_inactive = 0;
-	else
-		nr_inactive = 0;
+	for_each_lru(l) {
+		zone->nr_scan[l] += (zone_page_state(zone, NR_INACTIVE + l)
+							>> priority) + 1;
+		nr[l] = zone->nr_scan[l];
+		if (nr[l] >= sc->swap_cluster_max)
+			zone->nr_scan[l] = 0;
+		else
+			nr[l] = 0;
+	}
 
-	while (nr_active || nr_inactive) {
-		if (nr_active) {
-			nr_to_scan = min(nr_active,
+	while (nr[LRU_ACTIVE] || nr[LRU_INACTIVE]) {
+		for_each_lru(l) {
+			if (nr[l]) {
+				nr_to_scan = min(nr[l],
 					(unsigned long)sc->swap_cluster_max);
-			nr_active -= nr_to_scan;
-			shrink_active_list(nr_to_scan, zone, sc, priority);
-		}
+				nr[l] -= nr_to_scan;
 
-		if (nr_inactive) {
-			nr_to_scan = min(nr_inactive,
-					(unsigned long)sc->swap_cluster_max);
-			nr_inactive -= nr_to_scan;
-			nr_reclaimed += shrink_inactive_list(nr_to_scan, zone,
-								sc);
+				nr_reclaimed += shrink_list(l, nr_to_scan,
+							zone, sc, priority);
+			}
 		}
 	}
 

@@ -1733,6 +1733,7 @@ static unsigned long shrink_all_zones(un
 {
 	struct zone *zone;
 	unsigned long nr_to_scan, ret = 0;
+	enum lru_list l;
 
 	for_each_zone(zone) {
 

@@ -1742,28 +1743,25 @@ static unsigned long shrink_all_zones(un
 		if (zone->all_unreclaimable && prio != DEF_PRIORITY)
 			continue;
 
-		/* For pass = 0 we don't shrink the active list */
-		if (pass > 0) {
-			zone->nr_scan_active +=
-				(zone_page_state(zone, NR_ACTIVE) >> prio) + 1;
-			if (zone->nr_scan_active >= nr_pages || pass > 3) {
-				zone->nr_scan_active = 0;
+		for_each_lru(l) {
+			/* For pass = 0 we don't shrink the active list */
+			if (pass == 0 && l == LRU_ACTIVE)
+				continue;
+
+			zone->nr_scan[l] +=
+				(zone_page_state(zone, NR_INACTIVE + l)
+								>> prio) + 1;
+			if (zone->nr_scan[l] >= nr_pages || pass > 3) {
+				zone->nr_scan[l] = 0;
 				nr_to_scan = min(nr_pages,
-					zone_page_state(zone, NR_ACTIVE));
-				shrink_active_list(nr_to_scan, zone, sc, prio);
+					zone_page_state(zone,
+							NR_INACTIVE + l));
+				ret += shrink_list(l, nr_to_scan, zone,
+								sc, prio);
+				if (ret >= nr_pages)
+					return ret;
 			}
 		}
-
-		zone->nr_scan_inactive +=
-			(zone_page_state(zone, NR_INACTIVE) >> prio) + 1;
-		if (zone->nr_scan_inactive >= nr_pages || pass > 3) {
-			zone->nr_scan_inactive = 0;
-			nr_to_scan = min(nr_pages,
-				zone_page_state(zone, NR_INACTIVE));
-			ret += shrink_inactive_list(nr_to_scan, zone, sc);
-			if (ret >= nr_pages)
-				return ret;
-		}
 	}
 
 	return ret;

Index: Linux/mm/vmstat.c
===================================================================

mm/vmstat.c +2 -1

@@ -756,7 +756,8 @@ static void zoneinfo_show_print(struct s
 		   zone->pages_low,
 		   zone->pages_high,
 		   zone->pages_scanned,
-		   zone->nr_scan_active, zone->nr_scan_inactive,
+		   zone->nr_scan[LRU_ACTIVE],
+		   zone->nr_scan[LRU_INACTIVE],
 		   zone->spanned_pages,
 		   zone->present_pages);
 

On (14/09/07 16:54), Lee Schermerhorn didst pronounce:

Quoted 4 lines

[PATCH/RFC] 05/15 Reclaim Scalability: Use an indexed array for LRU variables

From [email protected] Wed Aug 29 11:39:51 2007

Quoted 5 lines

static inline void
del_page_from_lru(struct zone *zone, struct page *page) {

(quoted diff) +1 -0

+	enum lru_list l = LRU_INACTIVE;

Review

Quoted 10 lines

(quoted diff) +3 -2

+
 	list_del(&page->lru);
 	if (PageActive(page)) {
 		__ClearPageActive(page);
 		__dec_zone_state(zone, NR_ACTIVE);
-	} else {
-		__dec_zone_state(zone, NR_INACTIVE);
+		l = LRU_ACTIVE;
 	}
+	__dec_zone_state(zone, NR_INACTIVE + l);

It looks like you can call __dec_zone_state() twice for active pages here. Was it meant to be?

enum lru_list l = LRU_INACTIVE;

If (PageActive(page)) {
	__ClearPageActive(page);
	l = LRU_INACTIVE;
} else {
	l = LRU_ACTIVE;
}

__dec_zone_state(zone, NR_INACTIVE + l);

?

Quoted 5 lines · Mel Gorman

On (14/09/07 16:54), Lee Schermerhorn didst pronounce:

Quoted 4 lines

[PATCH/RFC] 05/15 Reclaim Scalability: Use an indexed array for LRU variables

From [email protected] Wed Aug 29 11:39:51 2007

<snip>

Quoted 29 lines

Quoted 5 lines

static inline void
del_page_from_lru(struct zone *zone, struct page *page) {

(quoted diff) +1 -0

+	enum lru_list l = LRU_INACTIVE;

Review

Quoted 10 lines

(quoted diff) +3 -2

+
 	list_del(&page->lru);
 	if (PageActive(page)) {
 		__ClearPageActive(page);
 		__dec_zone_state(zone, NR_ACTIVE);
-	} else {
-		__dec_zone_state(zone, NR_INACTIVE);
+		l = LRU_ACTIVE;
 	}
+	__dec_zone_state(zone, NR_INACTIVE + l);

It looks like you can call __dec_zone_state() twice for active pages here. Was it meant to be?

enum lru_list l = LRU_INACTIVE;

If (PageActive(page)) {
	__ClearPageActive(page);
	l = LRU_INACTIVE;
} else {
	l = LRU_ACTIVE;
}

__dec_zone_state(zone, NR_INACTIVE + l);

Quoted 1 line

?

Yes, another botched merge :-(. This does explain why I'm seeing the active memory in meminfo going to zero and staying there after I kill off the tests.

Will fix and retest.

It's great to have other eyes looking at these!

Thanks.

Lee

<snip>

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

[PATCH/RFC] 05/15 Reclaim Scalability: Use an indexed array for LRU variables

From [email protected] Wed Aug 29 11:39:51 2007

Currently we are defining explicit variables for the inactive and active list. An indexed array can be more generic and avoid repeating similar code in several places in the reclaim code.

We are saving a few bytes in terms of code size:

Before:

text data bss dec hex filename
4097753 573120 4092484 8763357 85b7dd vmlinux

After:

text data bss dec hex filename
4097729 573120 4092484 8763333 85b7c5 vmlinux

Having an easy way to add new lru lists may ease future work on the reclaim code.

[CL's signoff added by lts based on mail from CL]

Signed-off-by Christoph Lameter <[email protected]>

 include/linux/mm_inline.h |   33 ++++++++---
 include/linux/mmzone.h    |   17 +++--
 mm/page_alloc.c           |    9 +--
 mm/swap.c                 |    2 
 mm/vmscan.c               |  132 ++++++++++++++++++++++------------------------
 mm/vmstat.c               |    3 -
 6 files changed, 107 insertions(+), 89 deletions(-)

Index: Linux/include/linux/mmzone.h
===================================================================

include/linux/mmzone.h +9 -2

@@ -81,8 +81,8 @@ struct zone_padding {
 enum zone_stat_item {
 	/* First 128 byte cacheline (assuming 64 bit words) */
 	NR_FREE_PAGES,
-	NR_INACTIVE,
-	NR_ACTIVE,
+	NR_INACTIVE,	/* must match order of LRU_[IN]ACTIVE */
+	NR_ACTIVE,	/*  "     "     "   "       "         */
 	NR_ANON_PAGES,	/* Mapped anonymous pages */
 	NR_FILE_MAPPED,	/* pagecache pages mapped into pagetables.
 			   only modified from process context */

@@ -106,6 +106,13 @@ enum zone_stat_item {
 #endif
 	NR_VM_ZONE_STAT_ITEMS };
 
+enum lru_list {
+	LRU_INACTIVE,	/* must match order of NR_[IN]ACTIVE */
+	LRU_ACTIVE,	/*  "     "     "   "       "        */
+	NR_LRU_LISTS };
+
+#define for_each_lru(l) for (l = 0; l < NR_LRU_LISTS; l++)
+
 struct per_cpu_pages {
 	int count;		/* number of pages in the list */
 	int high;		/* high watermark, emptying needed */

include/linux/mmzone.h +1 -0

 static inline void
 del_page_from_lru(struct zone *zone, struct page *page)
 {
+	enum lru_list l = LRU_INACTIVE;

}

Quoted 64 lines · Lee Schermerhorn

[PATCH/RFC] 05/15 Reclaim Scalability: Use an indexed array for LRU variables

From [email protected] Wed Aug 29 11:39:51 2007

Currently we are defining explicit variables for the inactive and active list. An indexed array can be more generic and avoid repeating similar code in several places in the reclaim code.

We are saving a few bytes in terms of code size:

Before:

text data bss dec hex filename
4097753 573120 4092484 8763357 85b7dd vmlinux

After:

text data bss dec hex filename
4097729 573120 4092484 8763333 85b7c5 vmlinux

Having an easy way to add new lru lists may ease future work on the reclaim code.

[CL's signoff added by lts based on mail from CL]

Signed-off-by Christoph Lameter <[email protected]>

 include/linux/mm_inline.h |   33 ++++++++---
 include/linux/mmzone.h    |   17 +++--
 mm/page_alloc.c           |    9 +--
 mm/swap.c                 |    2 
 mm/vmscan.c               |  132 ++++++++++++++++++++++------------------------
 mm/vmstat.c               |    3 -
 6 files changed, 107 insertions(+), 89 deletions(-)

Index: Linux/include/linux/mmzone.h
===================================================================

include/linux/mmzone.h +9 -2

@@ -81,8 +81,8 @@ struct zone_padding {
 enum zone_stat_item {
 	/* First 128 byte cacheline (assuming 64 bit words) */
 	NR_FREE_PAGES,
-	NR_INACTIVE,
-	NR_ACTIVE,
+	NR_INACTIVE,	/* must match order of LRU_[IN]ACTIVE */
+	NR_ACTIVE,	/*  "     "     "   "       "         */
 	NR_ANON_PAGES,	/* Mapped anonymous pages */
 	NR_FILE_MAPPED,	/* pagecache pages mapped into pagetables.
 			   only modified from process context */

@@ -106,6 +106,13 @@ enum zone_stat_item {
 #endif
 	NR_VM_ZONE_STAT_ITEMS };
 
+enum lru_list {
+	LRU_INACTIVE,	/* must match order of NR_[IN]ACTIVE */
+	LRU_ACTIVE,	/*  "     "     "   "       "        */
+	NR_LRU_LISTS };
+
+#define for_each_lru(l) for (l = 0; l < NR_LRU_LISTS; l++)
+
 struct per_cpu_pages {
 	int count;		/* number of pages in the list */
 	int high;		/* high watermark, emptying needed */

include/linux/mmzone.h struct zone { · line 270

Quoted 9 lines

include/linux/mmzone.h +2 -4

@@ -259,10 +266,8 @@ struct zone {
 
 	/* Fields commonly accessed by the page reclaim scanner */
 	spinlock_t		lru_lock;	
-	struct list_head	active_list;
-	struct list_head	inactive_list;
-	unsigned long		nr_scan_active;
-	unsigned long		nr_scan_inactive;
+	struct list_head	list[NR_LRU_LISTS];
+	unsigned long		nr_scan[NR_LRU_LISTS];

I wonder if it makes sense to have an array of the form

struct reclaim_lists {
	struct list_head list[NR_LRU_LISTS];
	unsigned long nr_scan[NR_LRU_LISTS];
	reclaim_function_t list_reclaim_function[NR_LRU_LISTS];
}

where reclaim_function is an array of reclaim functions for each list (in our case shrink_active_list/shrink_inactive_list).

Quoted 6 lines

include/linux/mmzone.h +1 -0

 static inline void
 del_page_from_lru(struct zone *zone, struct page *page)
 {
+	enum lru_list l = LRU_INACTIVE;

include/linux/mmzone.h struct zone {

Quoted 10 lines

include/linux/mmzone.h +3 -2

+
 	list_del(&page->lru);
 	if (PageActive(page)) {
 		__ClearPageActive(page);
 		__dec_zone_state(zone, NR_ACTIVE);
-	} else {
-		__dec_zone_state(zone, NR_INACTIVE);
+		l = LRU_ACTIVE;
 	}
+	__dec_zone_state(zone, NR_INACTIVE + l);

This is unconditional, does not seem right.

Quoted 76 lines · Balbir Singh

Quoted 64 lines · Lee Schermerhorn

[PATCH/RFC] 05/15 Reclaim Scalability: Use an indexed array for LRU variables

From [email protected] Wed Aug 29 11:39:51 2007

Currently we are defining explicit variables for the inactive and active list. An indexed array can be more generic and avoid repeating similar code in several places in the reclaim code.

We are saving a few bytes in terms of code size:

Before:

text data bss dec hex filename
4097753 573120 4092484 8763357 85b7dd vmlinux

After:

text data bss dec hex filename
4097729 573120 4092484 8763333 85b7c5 vmlinux

Having an easy way to add new lru lists may ease future work on the reclaim code.

[CL's signoff added by lts based on mail from CL]

Signed-off-by Christoph Lameter <[email protected]>

 include/linux/mm_inline.h |   33 ++++++++---
 include/linux/mmzone.h    |   17 +++--
 mm/page_alloc.c           |    9 +--
 mm/swap.c                 |    2 
 mm/vmscan.c               |  132 ++++++++++++++++++++++------------------------
 mm/vmstat.c               |    3 -
 6 files changed, 107 insertions(+), 89 deletions(-)

Index: Linux/include/linux/mmzone.h
===================================================================

include/linux/mmzone.h +9 -2

@@ -81,8 +81,8 @@ struct zone_padding {
 enum zone_stat_item {
 	/* First 128 byte cacheline (assuming 64 bit words) */
 	NR_FREE_PAGES,
-	NR_INACTIVE,
-	NR_ACTIVE,
+	NR_INACTIVE,	/* must match order of LRU_[IN]ACTIVE */
+	NR_ACTIVE,	/*  "     "     "   "       "         */
 	NR_ANON_PAGES,	/* Mapped anonymous pages */
 	NR_FILE_MAPPED,	/* pagecache pages mapped into pagetables.
 			   only modified from process context */

@@ -106,6 +106,13 @@ enum zone_stat_item {
 #endif
 	NR_VM_ZONE_STAT_ITEMS };
 
+enum lru_list {
+	LRU_INACTIVE,	/* must match order of NR_[IN]ACTIVE */
+	LRU_ACTIVE,	/*  "     "     "   "       "        */
+	NR_LRU_LISTS };
+
+#define for_each_lru(l) for (l = 0; l < NR_LRU_LISTS; l++)
+
 struct per_cpu_pages {
 	int count;		/* number of pages in the list */
 	int high;		/* high watermark, emptying needed */

include/linux/mmzone.h struct zone { · line 270

Quoted 9 lines

include/linux/mmzone.h +2 -4

@@ -259,10 +266,8 @@ struct zone {
 
 	/* Fields commonly accessed by the page reclaim scanner */
 	spinlock_t		lru_lock;	
-	struct list_head	active_list;
-	struct list_head	inactive_list;
-	unsigned long		nr_scan_active;
-	unsigned long		nr_scan_inactive;
+	struct list_head	list[NR_LRU_LISTS];
+	unsigned long		nr_scan[NR_LRU_LISTS];

I wonder if it makes sense to have an array of the form

Quoted 8 lines

struct reclaim_lists {
	struct list_head list[NR_LRU_LISTS];
	unsigned long nr_scan[NR_LRU_LISTS];
	reclaim_function_t list_reclaim_function[NR_LRU_LISTS];
}

where reclaim_function is an array of reclaim functions for each list (in our case shrink_active_list/shrink_inactive_list).

Are you thinking that memory controller would use the reclaim functions switch--e.g., because of it's private lru lists? And what sort of
reclaim functions do you have in mind? Would it add additional
indirection in the fault path where we add pages to the LRU and move them between LRU lists in the case of page activiation? That could be a concern. In any case, maybe should be named something like 'lru_lists' and 'lru_list_functions'?

Quoted 16 lines

Quoted 5 lines

include/linux/mmzone.h +1 -0

 static inline void
 del_page_from_lru(struct zone *zone, struct page *page)
 {
+	enum lru_list l = LRU_INACTIVE;

include/linux/mmzone.h struct zone {

Quoted 10 lines

include/linux/mmzone.h +3 -2

+
 	list_del(&page->lru);
 	if (PageActive(page)) {
 		__ClearPageActive(page);
 		__dec_zone_state(zone, NR_ACTIVE);
-	} else {
-		__dec_zone_state(zone, NR_INACTIVE);
+		l = LRU_ACTIVE;
 	}
+	__dec_zone_state(zone, NR_INACTIVE + l);

This is unconditional, does not seem right.

It's not the unconditional one that wrong. As Mel pointed out earlier, I forgot to remove the explicit decrement of NR_ACTIVE. Turns out that I unknowingly fixed this in the subsequent noreclaim infrastructure patch, but I need to fix it in this patch so that it stands alone. Next respin.

Lee

I wonder if it makes sense to have an array of the form

struct reclaim_lists {
	struct list_head list[NR_LRU_LISTS];
	unsigned long nr_scan[NR_LRU_LISTS];
	reclaim_function_t list_reclaim_function[NR_LRU_LISTS];
}

where reclaim_function is an array of reclaim functions for each list (in our case shrink_active_list/shrink_inactive_list).