[PATCH 0/7] mm: cleanups

[PATCH 0/7] mm: cleanups

[Hugh Dickins]

Here's a batch of 7 mm cleanups, intended for 2.6.29: just assorted
things that have been annoying me.  Three or four slightly more
interesting batches should follow in the days ahead.

Though most of the testing has been against 2.6.28-rc5 and its
precursors, these patches are diffed to slot in to the mmotm series
just before "mmend": that is, before the "memcgroup" changes,
with which there's one trivial clash on 5/7.

 Documentation/filesystems/Locking |    6 -----
 Documentation/filesystems/vfs.txt |   13 ++++++++---
 fs/buffer.c                       |   12 ++++------
 fs/inode.c                        |    4 +--
 include/linux/cgroup.h            |   14 -----------
 include/linux/fs.h                |   10 --------
 include/linux/gfp.h               |    6 -----
 include/linux/page-flags.h        |    1 
 include/linux/rmap.h              |    3 --
 include/linux/swap.h              |    2 -
 kernel/cgroup.c                   |   33 ----------------------------
 kernel/exit.c                     |   16 +++++--------
 mm/memory.c                       |    6 -----
 mm/memory_hotplug.c               |    9 ++-----
 mm/migrate.c                      |    9 +------
 mm/page-writeback.c               |   27 +++++++++-------------
 mm/page_io.c                      |    4 +--
 mm/rmap.c                         |   11 ++++++---
 mm/shmem.c                        |    2 -
 mm/swap.c                         |   19 ----------------
 mm/swap_state.c                   |   19 +++++++---------
 mm/swapfile.c                     |    8 ++----
 mm/vmscan.c                       |   24 +++++++++-----------
 23 files changed, 76 insertions(+), 182 deletions(-)

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

[PATCH 1/7] mm: remove cgroup_mm_owner_callbacks

[Hugh Dickins]

cgroup_mm_owner_callbacks() was brought in to support the memrlimit
controller, but sneaked into mainline ahead of it.  That controller
has now been shelved, and the mm_owner_changed() args were inadequate
for it anyway (they needed an mm pointer instead of a task pointer).

Remove the dead code, and restore mm_update_next_owner() locking to
how it was before: taking mmap_sem there does nothing for memcontrol.c,
now the only user of mm->owner.

Signed-off-by: Hugh Dickins <hugh@veritas.com>
---

 include/linux/cgroup.h |   14 +-------------
 kernel/cgroup.c        |   33 ---------------------------------
 kernel/exit.c          |   16 ++++++----------
 3 files changed, 7 insertions(+), 56 deletions(-)

--- mmclean0/include/linux/cgroup.h	2008-11-02 23:17:56.000000000 +0000
+++ mmclean1/include/linux/cgroup.h	2008-11-19 15:26:10.000000000 +0000
@@ -329,13 +329,7 @@ struct cgroup_subsys {
 			struct cgroup *cgrp);
 	void (*post_clone)(struct cgroup_subsys *ss, struct cgroup *cgrp);
 	void (*bind)(struct cgroup_subsys *ss, struct cgroup *root);
-	/*
-	 * This routine is called with the task_lock of mm->owner held
-	 */
-	void (*mm_owner_changed)(struct cgroup_subsys *ss,
-					struct cgroup *old,
-					struct cgroup *new,
-					struct task_struct *p);
+
 	int subsys_id;
 	int active;
 	int disabled;
@@ -400,9 +394,6 @@ void cgroup_iter_end(struct cgroup *cgrp
 int cgroup_scan_tasks(struct cgroup_scanner *scan);
 int cgroup_attach_task(struct cgroup *, struct task_struct *);
 
-void cgroup_mm_owner_callbacks(struct task_struct *old,
-			       struct task_struct *new);
-
 #else /* !CONFIG_CGROUPS */
 
 static inline int cgroup_init_early(void) { return 0; }
@@ -420,9 +411,6 @@ static inline int cgroupstats_build(stru
 	return -EINVAL;
 }
 
-static inline void cgroup_mm_owner_callbacks(struct task_struct *old,
-					     struct task_struct *new) {}
-
 #endif /* !CONFIG_CGROUPS */
 
 #endif /* _LINUX_CGROUP_H */
--- mmclean0/kernel/cgroup.c	2008-11-19 15:25:12.000000000 +0000
+++ mmclean1/kernel/cgroup.c	2008-11-19 15:26:10.000000000 +0000
@@ -116,7 +116,6 @@ static int root_count;
  * be called.
  */
 static int need_forkexit_callback __read_mostly;
-static int need_mm_owner_callback __read_mostly;
 
 /* convenient tests for these bits */
 inline int cgroup_is_removed(const struct cgroup *cgrp)
@@ -2539,7 +2538,6 @@ static void __init cgroup_init_subsys(st
 	init_css_set.subsys[ss->subsys_id] = dummytop->subsys[ss->subsys_id];
 
 	need_forkexit_callback |= ss->fork || ss->exit;
-	need_mm_owner_callback |= !!ss->mm_owner_changed;
 
 	/* At system boot, before all subsystems have been
 	 * registered, no tasks have been forked, so we don't
@@ -2789,37 +2787,6 @@ void cgroup_fork_callbacks(struct task_s
 	}
 }
 
-#ifdef CONFIG_MM_OWNER
-/**
- * cgroup_mm_owner_callbacks - run callbacks when the mm->owner changes
- * @p: the new owner
- *
- * Called on every change to mm->owner. mm_init_owner() does not
- * invoke this routine, since it assigns the mm->owner the first time
- * and does not change it.
- *
- * The callbacks are invoked with mmap_sem held in read mode.
- */
-void cgroup_mm_owner_callbacks(struct task_struct *old, struct task_struct *new)
-{
-	struct cgroup *oldcgrp, *newcgrp = NULL;
-
-	if (need_mm_owner_callback) {
-		int i;
-		for (i = 0; i < CGROUP_SUBSYS_COUNT; i++) {
-			struct cgroup_subsys *ss = subsys[i];
-			oldcgrp = task_cgroup(old, ss->subsys_id);
-			if (new)
-				newcgrp = task_cgroup(new, ss->subsys_id);
-			if (oldcgrp == newcgrp)
-				continue;
-			if (ss->mm_owner_changed)
-				ss->mm_owner_changed(ss, oldcgrp, newcgrp, new);
-		}
-	}
-}
-#endif /* CONFIG_MM_OWNER */
-
 /**
  * cgroup_post_fork - called on a new task after adding it to the task list
  * @child: the task in question
--- mmclean0/kernel/exit.c	2008-11-19 15:25:12.000000000 +0000
+++ mmclean1/kernel/exit.c	2008-11-19 15:26:10.000000000 +0000
@@ -638,35 +638,31 @@ retry:
 	/*
 	 * We found no owner yet mm_users > 1: this implies that we are
 	 * most likely racing with swapoff (try_to_unuse()) or /proc or
-	 * ptrace or page migration (get_task_mm()).  Mark owner as NULL,
-	 * so that subsystems can understand the callback and take action.
+	 * ptrace or page migration (get_task_mm()).  Mark owner as NULL.
 	 */
-	down_write(&mm->mmap_sem);
-	cgroup_mm_owner_callbacks(mm->owner, NULL);
 	mm->owner = NULL;
-	up_write(&mm->mmap_sem);
 	return;
 
 assign_new_owner:
 	BUG_ON(c == p);
 	get_task_struct(c);
-	read_unlock(&tasklist_lock);
-	down_write(&mm->mmap_sem);
 	/*
 	 * The task_lock protects c->mm from changing.
 	 * We always want mm->owner->mm == mm
 	 */
 	task_lock(c);
+	/*
+	 * Delay read_unlock() till we have the task_lock()
+	 * to ensure that c does not slip away underneath us
+	 */
+	read_unlock(&tasklist_lock);
 	if (c->mm != mm) {
 		task_unlock(c);
-		up_write(&mm->mmap_sem);
 		put_task_struct(c);
 		goto retry;
 	}
-	cgroup_mm_owner_callbacks(mm->owner, c);
 	mm->owner = c;
 	task_unlock(c);
-	up_write(&mm->mmap_sem);
 	put_task_struct(c);
 }
 #endif /* CONFIG_MM_OWNER */

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

Re: [PATCH 1/7] mm: remove cgroup_mm_owner_callbacks

[Paul Menage]

On Wed, Nov 19, 2008 at 5:11 PM, Hugh Dickins <hugh@veritas.com> wrote:
>
>  assign_new_owner:
>        BUG_ON(c == p);
>        get_task_struct(c);
> -       read_unlock(&tasklist_lock);
> -       down_write(&mm->mmap_sem);
>        /*
>         * The task_lock protects c->mm from changing.
>         * We always want mm->owner->mm == mm
>         */
>        task_lock(c);
> +       /*
> +        * Delay read_unlock() till we have the task_lock()
> +        * to ensure that c does not slip away underneath us
> +        */
> +       read_unlock(&tasklist_lock);

How can c slip away when we've done get_task_struct(c) earlier?

Paul

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

Re: [PATCH 1/7] mm: remove cgroup_mm_owner_callbacks

[Hugh Dickins]

On Wed, 19 Nov 2008, Paul Menage wrote:
> On Wed, Nov 19, 2008 at 5:11 PM, Hugh Dickins <hugh@veritas.com> wrote:
> >
> >  assign_new_owner:
> >        BUG_ON(c == p);
> >        get_task_struct(c);
> > -       read_unlock(&tasklist_lock);
> > -       down_write(&mm->mmap_sem);
> >        /*
> >         * The task_lock protects c->mm from changing.
> >         * We always want mm->owner->mm == mm
> >         */
> >        task_lock(c);
> > +       /*
> > +        * Delay read_unlock() till we have the task_lock()
> > +        * to ensure that c does not slip away underneath us
> > +        */
> > +       read_unlock(&tasklist_lock);
> 
> How can c slip away when we've done get_task_struct(c) earlier?

I don't know, I did vaguely wonder the same myself: just putting
this back to how it was before (including that comment),
maybe Balbir can enlighten us.

Hugh

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

Re: [PATCH 1/7] mm: remove cgroup_mm_owner_callbacks

[Balbir Singh]

Hugh Dickins wrote:
> On Wed, 19 Nov 2008, Paul Menage wrote:
>> On Wed, Nov 19, 2008 at 5:11 PM, Hugh Dickins <hugh@veritas.com> wrote:
>>>  assign_new_owner:
>>>        BUG_ON(c == p);
>>>        get_task_struct(c);
>>> -       read_unlock(&tasklist_lock);
>>> -       down_write(&mm->mmap_sem);
>>>        /*
>>>         * The task_lock protects c->mm from changing.
>>>         * We always want mm->owner->mm == mm
>>>         */
>>>        task_lock(c);
>>> +       /*
>>> +        * Delay read_unlock() till we have the task_lock()
>>> +        * to ensure that c does not slip away underneath us
>>> +        */
>>> +       read_unlock(&tasklist_lock);
>> How can c slip away when we've done get_task_struct(c) earlier?
> 

"c" cannot slip away, but c->mm can change, but see below.

> I don't know, I did vaguely wonder the same myself: just putting
> this back to how it was before (including that comment),
> maybe Balbir can enlighten us.

Looking at the patch, we do handle a c->mm != mm case. The code seems to keep
the task on the global tasklist and protects task->mm, which might not be
necessary here. It would seem reasonable to allow the read_unlock() to occur
prior to task_lock().

-- 
	Balbir

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

[PATCH 2/7] mm: remove AOP_WRITEPAGE_ACTIVATE

[Hugh Dickins]

AOP_WRITEPAGE_ACTIVATE is a special return value from a_ops->writepage(),
for the filesystem to tell reclaim that it didn't write the page, and
there's little chance of being able to write it soon, so please defer
its reclaim as long as possible by recycling to head of active list.

(The Unevictable LRU has replaced this functionality to some extent;
but not entirely, and CONFIG_UNEVICTABLE_LRU may anyway not be set.)

It has only one user in tree, shmem_writepage(), and it only makes
sense in reclaim; but it has the confusing peculiarity of returning
with the page still locked, unlike both success and error cases.

It would be more intuitive if shmem_writepage() were to return the
page unlocked with PageActive set, and vmscan observe that to activate
the page: then nowhere else need worry about this peculiar return value.

(We already have the case of an unlocked page marked PageActive
but not yet PageLRU, that's not new.)

One anomaly - NR_VMSCAN_WRITE stats were not updated when AOP_WRITEPAGE
_ACTIVATE, but were updated when the filesystem just redirtied the page
and feigned success: NR_VMSCAN_WRITE stats now updated in neither case.

Signed-off-by: Hugh Dickins <hugh@veritas.com>
---
This has only taken me one year!

Google codesearch shows just one out-of-tree project actually using it:
openafs (in osi_vnodeops.c).  unionfs no longer refers to it; btrfs
refers to it only in #if code for kernel versions before 2.6.22.

Would it be okay for me to advise the OpenAFS people, and we remove the
definition in 2.6.29; or do we need to go through a deprecation process?

 Documentation/filesystems/Locking |    6 +-----
 Documentation/filesystems/vfs.txt |   13 ++++++++++---
 fs/buffer.c                       |   12 +++++-------
 include/linux/fs.h                |   10 ----------
 mm/migrate.c                      |    5 ++---
 mm/page-writeback.c               |   27 +++++++++++----------------
 mm/shmem.c                        |    2 +-
 mm/vmscan.c                       |   24 +++++++++++-------------
 8 files changed, 41 insertions(+), 58 deletions(-)

--- mmclean1/Documentation/filesystems/Locking	2008-11-02 23:17:53.000000000 +0000
+++ mmclean2/Documentation/filesystems/Locking	2008-11-19 15:26:13.000000000 +0000
@@ -225,11 +225,7 @@ If the filesystem is called for sync the
 in-progress I/O and then start new I/O.
 
 The filesystem should unlock the page synchronously, before returning to the
-caller, unless ->writepage() returns special WRITEPAGE_ACTIVATE
-value. WRITEPAGE_ACTIVATE means that page cannot really be written out
-currently, and VM should stop calling ->writepage() on this page for some
-time. VM does this by moving page to the head of the active list, hence the
-name.
+caller.
 
 Unless the filesystem is going to redirty_page_for_writepage(), unlock the page
 and return zero, writepage *must* run set_page_writeback() against the page,
--- mmclean1/Documentation/filesystems/vfs.txt	2008-11-19 15:24:49.000000000 +0000
+++ mmclean2/Documentation/filesystems/vfs.txt	2008-11-19 15:26:13.000000000 +0000
@@ -551,9 +551,16 @@ struct address_space_operations {
       If wbc->sync_mode is WB_SYNC_NONE, ->writepage doesn't have to
       try too hard if there are problems, and may choose to write out
       other pages from the mapping if that is easier (e.g. due to
-      internal dependencies).  If it chooses not to start writeout, it
-      should return AOP_WRITEPAGE_ACTIVATE so that the VM will not keep
-      calling ->writepage on that page.
+      internal dependencies); then redirty_page_for_writepage() on the
+      given page, before unlocking it and returning 0 for success.
+
+      If wbc->for_reclaim, but the filesystem will be unable to write
+      out the page for the foreseeable future (perhaps it cannot even
+      implement writepage in reclaim), it may redirty_page_for_writepage()
+      then SetPageActive(), before unlocking the page and returning 0: to
+      delay page reclaim from reconsidering this page for a little longer.
+      But this is unusual: SetPageActive() should never be used unless
+      wbc->for_reclaim, and only shmem_writepage() actually does this.
 
       See the file "Locking" for more details.
 
--- mmclean1/fs/buffer.c	2008-11-19 15:25:12.000000000 +0000
+++ mmclean2/fs/buffer.c	2008-11-19 15:26:13.000000000 +0000
@@ -3324,7 +3324,6 @@ EXPORT_SYMBOL(bh_submit_read);
 static void trigger_write(struct page *page)
 {
 	struct address_space *mapping = page_mapping(page);
-	int rc;
 	struct writeback_control wbc = {
 		.sync_mode = WB_SYNC_NONE,
 		.nr_to_write = 1,
@@ -3345,13 +3344,12 @@ static void trigger_write(struct page *p
 		/* Someone else already triggered a write */
 		goto unlock;
 
-	rc = mapping->a_ops->writepage(page, &wbc);
-	if (rc < 0)
-		/* I/O Error writing */
-		return;
+	mapping->a_ops->writepage(page, &wbc);
+	/* Ignore any error in writing */
+	return;
 
-	if (rc == AOP_WRITEPAGE_ACTIVATE)
-unlock:		unlock_page(page);
+unlock:
+	unlock_page(page);
 }
 
 /*
--- mmclean1/include/linux/fs.h	2008-11-19 15:25:12.000000000 +0000
+++ mmclean2/include/linux/fs.h	2008-11-19 15:26:13.000000000 +0000
@@ -385,15 +385,6 @@ struct iattr {
 /** 
  * enum positive_aop_returns - aop return codes with specific semantics
  *
- * @AOP_WRITEPAGE_ACTIVATE: Informs the caller that page writeback has
- * 			    completed, that the page is still locked, and
- * 			    should be considered active.  The VM uses this hint
- * 			    to return the page to the active list -- it won't
- * 			    be a candidate for writeback again in the near
- * 			    future.  Other callers must be careful to unlock
- * 			    the page if they get this return.  Returned by
- * 			    writepage(); 
- *
  * @AOP_TRUNCATED_PAGE: The AOP method that was handed a locked page has
  *  			unlocked it and the page might have been truncated.
  *  			The caller should back up to acquiring a new page and
@@ -409,7 +400,6 @@ struct iattr {
  */
 
 enum positive_aop_returns {
-	AOP_WRITEPAGE_ACTIVATE	= 0x80000,
 	AOP_TRUNCATED_PAGE	= 0x80001,
 };
 
--- mmclean1/mm/migrate.c	2008-11-19 15:25:12.000000000 +0000
+++ mmclean2/mm/migrate.c	2008-11-19 15:26:13.000000000 +0000
@@ -523,9 +523,8 @@ static int writeout(struct address_space
 
 	rc = mapping->a_ops->writepage(page, &wbc);
 
-	if (rc != AOP_WRITEPAGE_ACTIVATE)
-		/* unlocked. Relock */
-		lock_page(page);
+	/* Unlocked: relock */
+	lock_page(page);
 
 	return (rc < 0) ? -EIO : -EAGAIN;
 }
--- mmclean1/mm/page-writeback.c	2008-11-19 15:25:12.000000000 +0000
+++ mmclean2/mm/page-writeback.c	2008-11-19 15:26:13.000000000 +0000
@@ -963,22 +963,17 @@ continue_unlock:
 
 			ret = (*writepage)(page, wbc, data);
 			if (unlikely(ret)) {
-				if (ret == AOP_WRITEPAGE_ACTIVATE) {
-					unlock_page(page);
-					ret = 0;
-				} else {
-					/*
-					 * done_index is set past this page,
-					 * so media errors will not choke
-					 * background writeout for the entire
-					 * file. This has consequences for
-					 * range_cyclic semantics (ie. it may
-					 * not be suitable for data integrity
-					 * writeout).
-					 */
-					done = 1;
-					break;
-				}
+				/*
+				 * done_index is set past this page,
+				 * so media errors will not choke
+				 * background writeout for the entire
+				 * file. This has consequences for
+				 * range_cyclic semantics (ie. it may
+				 * not be suitable for data integrity
+				 * writeout).
+				 */
+				done = 1;
+				break;
  			}
 
 			if (wbc->sync_mode == WB_SYNC_NONE) {
--- mmclean1/mm/shmem.c	2008-11-19 15:25:12.000000000 +0000
+++ mmclean2/mm/shmem.c	2008-11-19 15:26:13.000000000 +0000
@@ -1074,7 +1074,7 @@ unlock:
 redirty:
 	set_page_dirty(page);
 	if (wbc->for_reclaim)
-		return AOP_WRITEPAGE_ACTIVATE;	/* Return with page locked */
+		SetPageActive(page);
 	unlock_page(page);
 	return 0;
 }
--- mmclean1/mm/vmscan.c	2008-11-19 15:25:12.000000000 +0000
+++ mmclean2/mm/vmscan.c	2008-11-19 15:26:13.000000000 +0000
@@ -349,8 +349,6 @@ typedef enum {
 	/* failed to write page out, page is locked */
 	PAGE_KEEP,
 	/* move page to the active list, page is locked */
-	PAGE_ACTIVATE,
-	/* page has been sent to the disk successfully, page is unlocked */
 	PAGE_SUCCESS,
 	/* page is clean and locked */
 	PAGE_CLEAN,
@@ -396,8 +394,10 @@ static pageout_t pageout(struct page *pa
 		}
 		return PAGE_KEEP;
 	}
-	if (mapping->a_ops->writepage == NULL)
-		return PAGE_ACTIVATE;
+	if (mapping->a_ops->writepage == NULL) {
+		SetPageActive(page);
+		return PAGE_KEEP;
+	}
 	if (!may_write_to_queue(mapping->backing_dev_info))
 		return PAGE_KEEP;
 
@@ -416,10 +416,6 @@ static pageout_t pageout(struct page *pa
 		res = mapping->a_ops->writepage(page, &wbc);
 		if (res < 0)
 			handle_write_error(mapping, page, res);
-		if (res == AOP_WRITEPAGE_ACTIVATE) {
-			ClearPageReclaim(page);
-			return PAGE_ACTIVATE;
-		}
 
 		/*
 		 * Wait on writeback if requested to. This happens when
@@ -430,10 +426,13 @@ static pageout_t pageout(struct page *pa
 			wait_on_page_writeback(page);
 
 		if (!PageWriteback(page)) {
-			/* synchronous write or broken a_ops? */
+			/* synchronous write, or writepage passed over it */
 			ClearPageReclaim(page);
 		}
-		inc_zone_page_state(page, NR_VMSCAN_WRITE);
+		if (!PageDirty(page)) {
+			/* writepage did not pass over this write */
+			inc_zone_page_state(page, NR_VMSCAN_WRITE);
+		}
 		return PAGE_SUCCESS;
 	}
 
@@ -720,8 +719,6 @@ static unsigned long shrink_page_list(st
 			switch (pageout(page, mapping, sync_writeback)) {
 			case PAGE_KEEP:
 				goto keep_locked;
-			case PAGE_ACTIVATE:
-				goto activate_locked;
 			case PAGE_SUCCESS:
 				if (PageWriteback(page) || PageDirty(page))
 					goto keep;
@@ -811,10 +808,11 @@ activate_locked:
 			remove_exclusive_swap_page_ref(page);
 		VM_BUG_ON(PageActive(page));
 		SetPageActive(page);
-		pgactivate++;
 keep_locked:
 		unlock_page(page);
 keep:
+		if (PageActive(page))
+			pgactivate++;
 		list_add(&page->lru, &ret_pages);
 		VM_BUG_ON(PageLRU(page) || PageUnevictable(page));
 	}

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

[PATCH 3/7] mm: remove GFP_HIGHUSER_PAGECACHE

[Hugh Dickins]

GFP_HIGHUSER_PAGECACHE is just an alias for GFP_HIGHUSER_MOVABLE,
making that harder to track down: remove it, and its out-of-work
brothers GFP_NOFS_PAGECACHE and GFP_USER_PAGECACHE.

Since we're making that improvement to hotremove_migrate_alloc(),
I think we can now also remove one of the "o"s from its comment.

Signed-off-by: Hugh Dickins <hugh@veritas.com>
---

 fs/inode.c          |    4 ++--
 include/linux/gfp.h |    6 ------
 mm/memory_hotplug.c |    9 +++------
 3 files changed, 5 insertions(+), 14 deletions(-)

--- mmclean2/fs/inode.c	2008-11-19 15:25:12.000000000 +0000
+++ mmclean3/fs/inode.c	2008-11-19 15:26:16.000000000 +0000
@@ -164,7 +164,7 @@ struct inode *inode_init_always(struct s
 	mapping->a_ops = &empty_aops;
 	mapping->host = inode;
 	mapping->flags = 0;
-	mapping_set_gfp_mask(mapping, GFP_HIGHUSER_PAGECACHE);
+	mapping_set_gfp_mask(mapping, GFP_HIGHUSER_MOVABLE);
 	mapping->assoc_mapping = NULL;
 	mapping->backing_dev_info = &default_backing_dev_info;
 	mapping->writeback_index = 0;
@@ -599,7 +599,7 @@ EXPORT_SYMBOL_GPL(inode_add_to_lists);
  *	@sb: superblock
  *
  *	Allocates a new inode for given superblock. The default gfp_mask
- *	for allocations related to inode->i_mapping is GFP_HIGHUSER_PAGECACHE.
+ *	for allocations related to inode->i_mapping is GFP_HIGHUSER_MOVABLE.
  *	If HIGHMEM pages are unsuitable or it is known that pages allocated
  *	for the page cache are not reclaimable or migratable,
  *	mapping_set_gfp_mask() must be called with suitable flags on the
--- mmclean2/include/linux/gfp.h	2008-11-19 15:25:12.000000000 +0000
+++ mmclean3/include/linux/gfp.h	2008-11-19 15:26:16.000000000 +0000
@@ -70,12 +70,6 @@ struct vm_area_struct;
 #define GFP_HIGHUSER_MOVABLE	(__GFP_WAIT | __GFP_IO | __GFP_FS | \
 				 __GFP_HARDWALL | __GFP_HIGHMEM | \
 				 __GFP_MOVABLE)
-#define GFP_NOFS_PAGECACHE	(__GFP_WAIT | __GFP_IO | __GFP_MOVABLE)
-#define GFP_USER_PAGECACHE	(__GFP_WAIT | __GFP_IO | __GFP_FS | \
-				 __GFP_HARDWALL | __GFP_MOVABLE)
-#define GFP_HIGHUSER_PAGECACHE	(__GFP_WAIT | __GFP_IO | __GFP_FS | \
-				 __GFP_HARDWALL | __GFP_HIGHMEM | \
-				 __GFP_MOVABLE)
 
 #ifdef CONFIG_NUMA
 #define GFP_THISNODE	(__GFP_THISNODE | __GFP_NOWARN | __GFP_NORETRY)
--- mmclean2/mm/memory_hotplug.c	2008-11-19 15:25:12.000000000 +0000
+++ mmclean3/mm/memory_hotplug.c	2008-11-19 15:26:16.000000000 +0000
@@ -626,15 +626,12 @@ int scan_lru_pages(unsigned long start, 
 }
 
 static struct page *
-hotremove_migrate_alloc(struct page *page,
-			unsigned long private,
-			int **x)
+hotremove_migrate_alloc(struct page *page, unsigned long private, int **x)
 {
-	/* This should be improoooooved!! */
-	return alloc_page(GFP_HIGHUSER_PAGECACHE);
+	/* This should be improooooved!! */
+	return alloc_page(GFP_HIGHUSER_MOVABLE);
 }
 
-
 #define NR_OFFLINE_AT_ONCE_PAGES	(256)
 static int
 do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)

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

Re: [PATCH 3/7] mm: remove GFP_HIGHUSER_PAGECACHE

[Mel Gorman]

Hi Hugh,

On Thu, Nov 20, 2008 at 01:16:16AM +0000, Hugh Dickins wrote:
> GFP_HIGHUSER_PAGECACHE is just an alias for GFP_HIGHUSER_MOVABLE,
> making that harder to track down: remove it, and its out-of-work
> brothers GFP_NOFS_PAGECACHE and GFP_USER_PAGECACHE.
> 
> Since we're making that improvement to hotremove_migrate_alloc(),
> I think we can now also remove one of the "o"s from its comment.
> 

The use of GFP_HIGHUSER_PAGECACHE instead of GFP_HIGHUSER_MOVABLE was a
deliberate decision at the time. I do not have an exact patch to point
you at but the intention behind GFP_HIGHUSER_PAGECACHE was that it be
self-documenting. i.e. one could easily find what GFP placement decisions
have been made for page-cache allocations.

GFP_NOFS_PAGECACHE is redundant and should have been dropped around the
time grow_dev_page() stopped using GFP_NOFS. GFP_USER_PAGECACHE is
similarly useless as bdget() no longer uses it which it did during some
point of patch revision.

So, I'm happy with GFP_NOFS_PAGECACHE and GFP_USER_PAGECACHE going away and
it makes perfect sense. GFP_HIGHUSER_PAGECACHE I'm not as keen on backing
out. I like it's self-documenting aspect but aliases sometimes make peoples
teeth itch. If it's really hated, then could a comment to the affect of
"Marked movable for a page cache allocation" be placed near the call-sites
instead?


> Signed-off-by: Hugh Dickins <hugh@veritas.com>
> ---
> 
>  fs/inode.c          |    4 ++--
>  include/linux/gfp.h |    6 ------
>  mm/memory_hotplug.c |    9 +++------
>  3 files changed, 5 insertions(+), 14 deletions(-)
> 
> --- mmclean2/fs/inode.c	2008-11-19 15:25:12.000000000 +0000
> +++ mmclean3/fs/inode.c	2008-11-19 15:26:16.000000000 +0000
> @@ -164,7 +164,7 @@ struct inode *inode_init_always(struct s
>  	mapping->a_ops = &empty_aops;
>  	mapping->host = inode;
>  	mapping->flags = 0;
> -	mapping_set_gfp_mask(mapping, GFP_HIGHUSER_PAGECACHE);
> +	mapping_set_gfp_mask(mapping, GFP_HIGHUSER_MOVABLE);
>  	mapping->assoc_mapping = NULL;
>  	mapping->backing_dev_info = &default_backing_dev_info;
>  	mapping->writeback_index = 0;
> @@ -599,7 +599,7 @@ EXPORT_SYMBOL_GPL(inode_add_to_lists);
>   *	@sb: superblock
>   *
>   *	Allocates a new inode for given superblock. The default gfp_mask
> - *	for allocations related to inode->i_mapping is GFP_HIGHUSER_PAGECACHE.
> + *	for allocations related to inode->i_mapping is GFP_HIGHUSER_MOVABLE.
>   *	If HIGHMEM pages are unsuitable or it is known that pages allocated
>   *	for the page cache are not reclaimable or migratable,
>   *	mapping_set_gfp_mask() must be called with suitable flags on the
> --- mmclean2/include/linux/gfp.h	2008-11-19 15:25:12.000000000 +0000
> +++ mmclean3/include/linux/gfp.h	2008-11-19 15:26:16.000000000 +0000
> @@ -70,12 +70,6 @@ struct vm_area_struct;
>  #define GFP_HIGHUSER_MOVABLE	(__GFP_WAIT | __GFP_IO | __GFP_FS | \
>  				 __GFP_HARDWALL | __GFP_HIGHMEM | \
>  				 __GFP_MOVABLE)
> -#define GFP_NOFS_PAGECACHE	(__GFP_WAIT | __GFP_IO | __GFP_MOVABLE)
> -#define GFP_USER_PAGECACHE	(__GFP_WAIT | __GFP_IO | __GFP_FS | \
> -				 __GFP_HARDWALL | __GFP_MOVABLE)
> -#define GFP_HIGHUSER_PAGECACHE	(__GFP_WAIT | __GFP_IO | __GFP_FS | \
> -				 __GFP_HARDWALL | __GFP_HIGHMEM | \
> -				 __GFP_MOVABLE)
>  
>  #ifdef CONFIG_NUMA
>  #define GFP_THISNODE	(__GFP_THISNODE | __GFP_NOWARN | __GFP_NORETRY)
> --- mmclean2/mm/memory_hotplug.c	2008-11-19 15:25:12.000000000 +0000
> +++ mmclean3/mm/memory_hotplug.c	2008-11-19 15:26:16.000000000 +0000
> @@ -626,15 +626,12 @@ int scan_lru_pages(unsigned long start, 
>  }
>  
>  static struct page *
> -hotremove_migrate_alloc(struct page *page,
> -			unsigned long private,
> -			int **x)
> +hotremove_migrate_alloc(struct page *page, unsigned long private, int **x)
>  {
> -	/* This should be improoooooved!! */
> -	return alloc_page(GFP_HIGHUSER_PAGECACHE);
> +	/* This should be improooooved!! */
> +	return alloc_page(GFP_HIGHUSER_MOVABLE);
>  }
>  
> -
>  #define NR_OFFLINE_AT_ONCE_PAGES	(256)
>  static int
>  do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
> 
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
> 

-- 
Mel Gorman
Part-time Phd Student                          Linux Technology Center
University of Limerick                         IBM Dublin Software Lab

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

Re: [PATCH 3/7] mm: remove GFP_HIGHUSER_PAGECACHE

[Hugh Dickins]

On Thu, 20 Nov 2008, Mel Gorman wrote:
> On Thu, Nov 20, 2008 at 01:16:16AM +0000, Hugh Dickins wrote:
> > GFP_HIGHUSER_PAGECACHE is just an alias for GFP_HIGHUSER_MOVABLE,
> > making that harder to track down: remove it, and its out-of-work
> > brothers GFP_NOFS_PAGECACHE and GFP_USER_PAGECACHE.
> 
> The use of GFP_HIGHUSER_PAGECACHE instead of GFP_HIGHUSER_MOVABLE was a
> deliberate decision at the time. I do not have an exact patch to point

I realize it didn't happen by accident!

> you at but the intention behind GFP_HIGHUSER_PAGECACHE was that it be
> self-documenting. i.e. one could easily find what GFP placement decisions
> have been made for page-cache allocations.

I see it as self-obscuring, not self-documenting: of course pagecache
pages will normally be allocated with the GFP mask for pagecache pages,
but what is that?  Ah, it's GFP_HIGHUSER_MOVABLE.

Please let's not go down the road, I mean, let's retrace our steps
up the road, of assigning a unique GFP name for every use of pages.

> So, I'm happy with GFP_NOFS_PAGECACHE and GFP_USER_PAGECACHE going away and
> it makes perfect sense. GFP_HIGHUSER_PAGECACHE I'm not as keen on backing
> out. I like it's self-documenting aspect but aliases sometimes make peoples
> teeth itch.

(No, what made my teeth itch was "is this safe?" in memory.c ;)

> If it's really hated, then could a comment to the affect of
> "Marked movable for a page cache allocation" be placed near the call-sites
> instead?

I'd prefer not.

The only places where GFP_HIGHUSER_PAGECACHE appeared
were the mapping_set_gfp_mask when initializing an inode, and
hotremove_migrate_alloc().  The latter allocating for anonymous
pages also, like most places where GFP_HIGHUSER_MOVABLE is specified.

But I'd better not complain that it's not obvious to me which
should be marked with your comment and which not: you'll point to
that as evidence that we're missing out on the self-documentation!

Perhaps the problem is that nobody else has been following your lead.

Hugh

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

Re: [PATCH 3/7] mm: remove GFP_HIGHUSER_PAGECACHE

[Mel Gorman]

On Thu, Nov 20, 2008 at 06:58:49PM +0000, Hugh Dickins wrote:
> On Thu, 20 Nov 2008, Mel Gorman wrote:
> > On Thu, Nov 20, 2008 at 01:16:16AM +0000, Hugh Dickins wrote:
> > > GFP_HIGHUSER_PAGECACHE is just an alias for GFP_HIGHUSER_MOVABLE,
> > > making that harder to track down: remove it, and its out-of-work
> > > brothers GFP_NOFS_PAGECACHE and GFP_USER_PAGECACHE.
> > 
> > The use of GFP_HIGHUSER_PAGECACHE instead of GFP_HIGHUSER_MOVABLE was a
> > deliberate decision at the time. I do not have an exact patch to point
> 
> I realize it didn't happen by accident!
> 
> > you at but the intention behind GFP_HIGHUSER_PAGECACHE was that it be
> > self-documenting. i.e. one could easily find what GFP placement decisions
> > have been made for page-cache allocations.
> 
> I see it as self-obscuring, not self-documenting: of course pagecache
> pages will normally be allocated with the GFP mask for pagecache pages,
> but what is that?  Ah, it's GFP_HIGHUSER_MOVABLE.
> 
> Please let's not go down the road, I mean, let's retrace our steps
> up the road, of assigning a unique GFP name for every use of pages.
> 

Hmm.... Ok. Whatever sense it made when there was NOFS and USER
variants, it doesn't help as much when there is only one variant now and
used in two fairly-obvious callsites.

> > So, I'm happy with GFP_NOFS_PAGECACHE and GFP_USER_PAGECACHE going away and
> > it makes perfect sense. GFP_HIGHUSER_PAGECACHE I'm not as keen on backing
> > out. I like it's self-documenting aspect but aliases sometimes make peoples
> > teeth itch.
> 
> (No, what made my teeth itch was "is this safe?" in memory.c ;)
> 
> > If it's really hated, then could a comment to the affect of
> > "Marked movable for a page cache allocation" be placed near the call-sites
> > instead?
> 
> I'd prefer not.
> 
> The only places where GFP_HIGHUSER_PAGECACHE appeared
> were the mapping_set_gfp_mask when initializing an inode, and
> hotremove_migrate_alloc().  The latter allocating for anonymous
> pages also, like most places where GFP_HIGHUSER_MOVABLE is specified.
> 
> But I'd better not complain that it's not obvious to me which
> should be marked with your comment and which not: you'll point to
> that as evidence that we're missing out on the self-documentation!
> 
> Perhaps the problem is that nobody else has been following your lead.
> 

You've convinced me. Thanks.

Acked-by: Mel Gorman <mel@csn.ul.ie>

-- 
Mel Gorman
Part-time Phd Student                          Linux Technology Center
University of Limerick                         IBM Dublin Software Lab

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

[PATCH 4/7] mm: add Set,ClearPageSwapCache stubs

[Hugh Dickins]

If we add NOOP stubs for SetPageSwapCache() and ClearPageSwapCache(),
then we can remove the #ifdef CONFIG_SWAPs from mm/migrate.c.

Signed-off-by: Hugh Dickins <hugh@veritas.com>
---

 include/linux/page-flags.h |    1 +
 mm/migrate.c               |    4 ----
 2 files changed, 1 insertion(+), 4 deletions(-)

--- mmclean3/include/linux/page-flags.h	2008-11-19 15:25:12.000000000 +0000
+++ mmclean4/include/linux/page-flags.h	2008-11-19 15:26:18.000000000 +0000
@@ -230,6 +230,7 @@ PAGEFLAG_FALSE(HighMem)
 PAGEFLAG(SwapCache, swapcache)
 #else
 PAGEFLAG_FALSE(SwapCache)
+	SETPAGEFLAG_NOOP(SwapCache) CLEARPAGEFLAG_NOOP(SwapCache)
 #endif
 
 #ifdef CONFIG_UNEVICTABLE_LRU
--- mmclean3/mm/migrate.c	2008-11-19 15:26:13.000000000 +0000
+++ mmclean4/mm/migrate.c	2008-11-19 15:26:18.000000000 +0000
@@ -300,12 +300,10 @@ static int migrate_page_move_mapping(str
 	 * Now we know that no one else is looking at the page.
 	 */
 	get_page(newpage);	/* add cache reference */
-#ifdef CONFIG_SWAP
 	if (PageSwapCache(page)) {
 		SetPageSwapCache(newpage);
 		set_page_private(newpage, page_private(page));
 	}
-#endif
 
 	radix_tree_replace_slot(pslot, newpage);
 
@@ -373,9 +371,7 @@ static void migrate_page_copy(struct pag
 
 	mlock_migrate_page(newpage, page);
 
-#ifdef CONFIG_SWAP
 	ClearPageSwapCache(page);
-#endif
 	ClearPagePrivate(page);
 	set_page_private(page, 0);
 	/* page->mapping contains a flag for PageAnon() */

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

Re: [PATCH 4/7] mm: add Set,ClearPageSwapCache stubs

[Christoph Lameter]

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

[PATCH 5/7] mm: replace some BUG_ONs by VM_BUG_ONs

[Hugh Dickins]

The swap code is over-provisioned with BUG_ONs on assorted page flags,
mostly dating back to 2.3.  They're good documentation, and guard against
developer error, but a waste of space on most systems: change them to
VM_BUG_ONs, conditional on CONFIG_DEBUG_VM.  Just delete the PagePrivate
ones: they're later, from 2.5.69, but even less interesting now.

Signed-off-by: Hugh Dickins <hugh@veritas.com>
---
Could be taken further: this now looks like a random selection
which caught my eye at some point working in these files.

This patch clashes trivially in __delete_from_swap_cache with
memcg-memswap-controller-core.patch, which I'm assuming comes
later: sorry, but you'll have no difficulty in fixing it up.

 mm/page_io.c    |    4 ++--
 mm/swap_state.c |   19 +++++++++----------
 mm/swapfile.c   |    8 +++-----
 3 files changed, 14 insertions(+), 17 deletions(-)

--- mmclean4/mm/page_io.c	2008-04-17 03:49:44.000000000 +0100
+++ mmclean5/mm/page_io.c	2008-11-19 15:26:26.000000000 +0000
@@ -125,8 +125,8 @@ int swap_readpage(struct file *file, str
 	struct bio *bio;
 	int ret = 0;
 
-	BUG_ON(!PageLocked(page));
-	BUG_ON(PageUptodate(page));
+	VM_BUG_ON(!PageLocked(page));
+	VM_BUG_ON(PageUptodate(page));
 	bio = get_swap_bio(GFP_KERNEL, page_private(page), page,
 				end_swap_bio_read);
 	if (bio == NULL) {
--- mmclean4/mm/swap_state.c	2008-10-24 09:28:26.000000000 +0100
+++ mmclean5/mm/swap_state.c	2008-11-19 15:26:26.000000000 +0000
@@ -72,10 +72,10 @@ int add_to_swap_cache(struct page *page,
 {
 	int error;
 
-	BUG_ON(!PageLocked(page));
-	BUG_ON(PageSwapCache(page));
-	BUG_ON(PagePrivate(page));
-	BUG_ON(!PageSwapBacked(page));
+	VM_BUG_ON(!PageLocked(page));
+	VM_BUG_ON(PageSwapCache(page));
+	VM_BUG_ON(!PageSwapBacked(page));
+
 	error = radix_tree_preload(gfp_mask);
 	if (!error) {
 		page_cache_get(page);
@@ -108,10 +108,9 @@ int add_to_swap_cache(struct page *page,
  */
 void __delete_from_swap_cache(struct page *page)
 {
-	BUG_ON(!PageLocked(page));
-	BUG_ON(!PageSwapCache(page));
-	BUG_ON(PageWriteback(page));
-	BUG_ON(PagePrivate(page));
+	VM_BUG_ON(!PageLocked(page));
+	VM_BUG_ON(!PageSwapCache(page));
+	VM_BUG_ON(PageWriteback(page));
 
 	radix_tree_delete(&swapper_space.page_tree, page_private(page));
 	set_page_private(page, 0);
@@ -134,8 +133,8 @@ int add_to_swap(struct page * page, gfp_
 	swp_entry_t entry;
 	int err;
 
-	BUG_ON(!PageLocked(page));
-	BUG_ON(!PageUptodate(page));
+	VM_BUG_ON(!PageLocked(page));
+	VM_BUG_ON(!PageUptodate(page));
 
 	for (;;) {
 		entry = get_swap_page();
--- mmclean4/mm/swapfile.c	2008-10-24 09:28:26.000000000 +0100
+++ mmclean5/mm/swapfile.c	2008-11-19 15:26:26.000000000 +0000
@@ -333,7 +333,7 @@ int can_share_swap_page(struct page *pag
 {
 	int count;
 
-	BUG_ON(!PageLocked(page));
+	VM_BUG_ON(!PageLocked(page));
 	count = page_mapcount(page);
 	if (count <= 1 && PageSwapCache(page))
 		count += page_swapcount(page);
@@ -350,8 +350,7 @@ static int remove_exclusive_swap_page_co
 	struct swap_info_struct * p;
 	swp_entry_t entry;
 
-	BUG_ON(PagePrivate(page));
-	BUG_ON(!PageLocked(page));
+	VM_BUG_ON(!PageLocked(page));
 
 	if (!PageSwapCache(page))
 		return 0;
@@ -432,7 +431,6 @@ void free_swap_and_cache(swp_entry_t ent
 	if (page) {
 		int one_user;
 
-		BUG_ON(PagePrivate(page));
 		one_user = (page_count(page) == 2);
 		/* Only cache user (+us), or swap space full? Free it! */
 		/* Also recheck PageSwapCache after page is locked (above) */
@@ -1209,7 +1207,7 @@ int page_queue_congested(struct page *pa
 {
 	struct backing_dev_info *bdi;
 
-	BUG_ON(!PageLocked(page));	/* It pins the swap_info_struct */
+	VM_BUG_ON(!PageLocked(page));	/* It pins the swap_info_struct */
 
 	if (PageSwapCache(page)) {
 		swp_entry_t entry = { .val = page_private(page) };

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

Re: [PATCH 5/7] mm: replace some BUG_ONs by VM_BUG_ONs

[Christoph Lameter]

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

[PATCH 6/7] mm: add_active_or_unevictable into rmap

[Hugh Dickins]

lru_cache_add_active_or_unevictable() and page_add_new_anon_rmap()
always appear together.  Save some symbol table space and some jumping
around by removing lru_cache_add_active_or_unevictable(), folding its
code into page_add_new_anon_rmap(): like how we add file pages to lru
just after adding them to page cache.

Remove the nearby "TODO: is this safe?" comments (yes, it is safe),
and change page_add_new_anon_rmap()'s address BUG_ON to VM_BUG_ON
as originally intended.

Signed-off-by: Hugh Dickins <hugh@veritas.com>
---

 include/linux/swap.h |    2 --
 mm/memory.c          |    6 ------
 mm/rmap.c            |    7 ++++++-
 mm/swap.c            |   19 -------------------
 4 files changed, 6 insertions(+), 28 deletions(-)

--- mmclean5/include/linux/swap.h	2008-11-19 15:25:12.000000000 +0000
+++ mmclean6/include/linux/swap.h	2008-11-19 15:26:28.000000000 +0000
@@ -174,8 +174,6 @@ extern unsigned int nr_free_pagecache_pa
 /* linux/mm/swap.c */
 extern void __lru_cache_add(struct page *, enum lru_list lru);
 extern void lru_cache_add_lru(struct page *, enum lru_list lru);
-extern void lru_cache_add_active_or_unevictable(struct page *,
-					struct vm_area_struct *);
 extern void activate_page(struct page *);
 extern void mark_page_accessed(struct page *);
 extern void lru_add_drain(void);
--- mmclean5/mm/memory.c	2008-11-19 15:25:12.000000000 +0000
+++ mmclean6/mm/memory.c	2008-11-19 15:26:28.000000000 +0000
@@ -1920,10 +1920,7 @@ gotten:
 		 */
 		ptep_clear_flush_notify(vma, address, page_table);
 		SetPageSwapBacked(new_page);
-		lru_cache_add_active_or_unevictable(new_page, vma);
 		page_add_new_anon_rmap(new_page, vma, address);
-
-//TODO:  is this safe?  do_anonymous_page() does it this way.
 		set_pte_at(mm, address, page_table, entry);
 		update_mmu_cache(vma, address, entry);
 		if (old_page) {
@@ -2419,7 +2416,6 @@ static int do_anonymous_page(struct mm_s
 		goto release;
 	inc_mm_counter(mm, anon_rss);
 	SetPageSwapBacked(page);
-	lru_cache_add_active_or_unevictable(page, vma);
 	page_add_new_anon_rmap(page, vma, address);
 	set_pte_at(mm, address, page_table, entry);
 
@@ -2568,7 +2564,6 @@ static int __do_fault(struct mm_struct *
 		if (anon) {
 			inc_mm_counter(mm, anon_rss);
 			SetPageSwapBacked(page);
-			lru_cache_add_active_or_unevictable(page, vma);
 			page_add_new_anon_rmap(page, vma, address);
 		} else {
 			inc_mm_counter(mm, file_rss);
@@ -2578,7 +2573,6 @@ static int __do_fault(struct mm_struct *
 				get_page(dirty_page);
 			}
 		}
-//TODO:  is this safe?  do_anonymous_page() does it this way.
 		set_pte_at(mm, address, page_table, entry);
 
 		/* no need to invalidate: a not-present page won't be cached */
--- mmclean5/mm/rmap.c	2008-11-19 15:25:12.000000000 +0000
+++ mmclean6/mm/rmap.c	2008-11-19 15:26:28.000000000 +0000
@@ -47,6 +47,7 @@
 #include <linux/rmap.h>
 #include <linux/rcupdate.h>
 #include <linux/module.h>
+#include <linux/mm_inline.h>
 #include <linux/kallsyms.h>
 #include <linux/memcontrol.h>
 #include <linux/mmu_notifier.h>
@@ -671,9 +672,13 @@ void page_add_anon_rmap(struct page *pag
 void page_add_new_anon_rmap(struct page *page,
 	struct vm_area_struct *vma, unsigned long address)
 {
-	BUG_ON(address < vma->vm_start || address >= vma->vm_end);
+	VM_BUG_ON(address < vma->vm_start || address >= vma->vm_end);
 	atomic_set(&page->_mapcount, 0); /* elevate count by 1 (starts at -1) */
 	__page_set_anon_rmap(page, vma, address);
+	if (page_evictable(page, vma))
+		lru_cache_add_lru(page, LRU_ACTIVE + page_is_file_cache(page));
+	else
+		add_page_to_unevictable_list(page);
 }
 
 /**
--- mmclean5/mm/swap.c	2008-11-19 15:25:12.000000000 +0000
+++ mmclean6/mm/swap.c	2008-11-19 15:26:28.000000000 +0000
@@ -246,25 +246,6 @@ void add_page_to_unevictable_list(struct
 	spin_unlock_irq(&zone->lru_lock);
 }
 
-/**
- * lru_cache_add_active_or_unevictable
- * @page:  the page to be added to LRU
- * @vma:   vma in which page is mapped for determining reclaimability
- *
- * place @page on active or unevictable LRU list, depending on
- * page_evictable().  Note that if the page is not evictable,
- * it goes directly back onto it's zone's unevictable list.  It does
- * NOT use a per cpu pagevec.
- */
-void lru_cache_add_active_or_unevictable(struct page *page,
-					struct vm_area_struct *vma)
-{
-	if (page_evictable(page, vma))
-		lru_cache_add_lru(page, LRU_ACTIVE + page_is_file_cache(page));
-	else
-		add_page_to_unevictable_list(page);
-}
-
 /*
  * Drain pages out of the cpu's pagevecs.
  * Either "cpu" is the current CPU, and preemption has already been

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

Re: [PATCH 6/7] mm: add_active_or_unevictable into rmap

[Rik van Riel]

Hugh Dickins wrote:
> lru_cache_add_active_or_unevictable() and page_add_new_anon_rmap()
> always appear together.  Save some symbol table space and some jumping
> around by removing lru_cache_add_active_or_unevictable(), folding its
> code into page_add_new_anon_rmap(): like how we add file pages to lru
> just after adding them to page cache.
> 
> Remove the nearby "TODO: is this safe?" comments (yes, it is safe),
> and change page_add_new_anon_rmap()'s address BUG_ON to VM_BUG_ON
> as originally intended.
> 
> Signed-off-by: Hugh Dickins <hugh@veritas.com>
Acked-by: Rik van Riel <riel@redhat.com>

-- 
All rights reversed.

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

Re: [PATCH 6/7] mm: add_active_or_unevictable into rmap

[Lee Schermerhorn]

On Wed, 2008-11-19 at 21:08 -0500, Rik van Riel wrote:
> Hugh Dickins wrote:
> > lru_cache_add_active_or_unevictable() and page_add_new_anon_rmap()
> > always appear together.  Save some symbol table space and some jumping
> > around by removing lru_cache_add_active_or_unevictable(), folding its
> > code into page_add_new_anon_rmap(): like how we add file pages to lru
> > just after adding them to page cache.
> > 
> > Remove the nearby "TODO: is this safe?" comments (yes, it is safe),
> > and change page_add_new_anon_rmap()'s address BUG_ON to VM_BUG_ON
> > as originally intended.

Hugh:  

Thanks for doing this [removing the comment].  I have a patch queued up
to do that, but recently swamped with other things.

Interesting that this is the first I've seen any comment on the comment,
tho'.  I would have thought the "//TODO"--a glaring violation of coding
style--would have at least provoked a comment on that account.

Lee
> > 
> > Signed-off-by: Hugh Dickins <hugh@veritas.com>
> Acked-by: Rik van Riel <riel@redhat.com>
> 

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

[PATCH 8/7] mm: further cleanup page_add_new_anon_rmap

[Hugh Dickins]

Moving lru_cache_add_active_or_unevictable() into page_add_new_anon_rmap()
was good but done stupidly: we should SetPageSwapBacked() there too; and
we know for sure that this anonymous, swap-backed page is not file cache.

Signed-off-by: Hugh Dickins <hugh@veritas.com>
---
I suspect Nick might like to change it to __SetPageSwapBacked later.

 mm/memory.c |    3 ---
 mm/rmap.c   |    6 +++---
 2 files changed, 3 insertions(+), 6 deletions(-)

--- mmclean7/mm/memory.c	2008-11-19 15:26:28.000000000 +0000
+++ mmclean8/mm/memory.c	2008-11-22 19:02:35.000000000 +0000
@@ -1919,7 +1919,6 @@ gotten:
 		 * thread doing COW.
 		 */
 		ptep_clear_flush_notify(vma, address, page_table);
-		SetPageSwapBacked(new_page);
 		page_add_new_anon_rmap(new_page, vma, address);
 		set_pte_at(mm, address, page_table, entry);
 		update_mmu_cache(vma, address, entry);
@@ -2415,7 +2414,6 @@ static int do_anonymous_page(struct mm_s
 	if (!pte_none(*page_table))
 		goto release;
 	inc_mm_counter(mm, anon_rss);
-	SetPageSwapBacked(page);
 	page_add_new_anon_rmap(page, vma, address);
 	set_pte_at(mm, address, page_table, entry);
 
@@ -2563,7 +2561,6 @@ static int __do_fault(struct mm_struct *
 			entry = maybe_mkwrite(pte_mkdirty(entry), vma);
 		if (anon) {
 			inc_mm_counter(mm, anon_rss);
-			SetPageSwapBacked(page);
 			page_add_new_anon_rmap(page, vma, address);
 		} else {
 			inc_mm_counter(mm, file_rss);
--- mmclean7/mm/rmap.c	2008-11-19 15:26:33.000000000 +0000
+++ mmclean8/mm/rmap.c	2008-11-22 19:02:35.000000000 +0000
@@ -47,7 +47,6 @@
 #include <linux/rmap.h>
 #include <linux/rcupdate.h>
 #include <linux/module.h>
-#include <linux/mm_inline.h>
 #include <linux/kallsyms.h>
 #include <linux/memcontrol.h>
 #include <linux/mmu_notifier.h>
@@ -673,10 +672,11 @@ void page_add_new_anon_rmap(struct page 
 	struct vm_area_struct *vma, unsigned long address)
 {
 	VM_BUG_ON(address < vma->vm_start || address >= vma->vm_end);
-	atomic_set(&page->_mapcount, 0); /* elevate count by 1 (starts at -1) */
+	SetPageSwapBacked(page);
+	atomic_set(&page->_mapcount, 0); /* increment count (starts at -1) */
 	__page_set_anon_rmap(page, vma, address);
 	if (page_evictable(page, vma))
-		lru_cache_add_lru(page, LRU_ACTIVE + page_is_file_cache(page));
+		lru_cache_add_lru(page, LRU_ACTIVE);
 	else
 		add_page_to_unevictable_list(page);
 }

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

Re: [PATCH 8/7] mm: further cleanup page_add_new_anon_rmap

[Rik van Riel]

Hugh Dickins wrote:
> Moving lru_cache_add_active_or_unevictable() into page_add_new_anon_rmap()
> was good but done stupidly: we should SetPageSwapBacked() there too; and
> we know for sure that this anonymous, swap-backed page is not file cache.

True, but ...

>  	if (page_evictable(page, vma))
> -		lru_cache_add_lru(page, LRU_ACTIVE + page_is_file_cache(page));
> +		lru_cache_add_lru(page, LRU_ACTIVE);

Then you will want to s/LRU_ACTIVE/LRU_ACTIVE_ANON/ here.

-- 
All rights reversed.

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

Re: [PATCH 8/7] mm: further cleanup page_add_new_anon_rmap

[Hugh Dickins]

On Sun, 23 Nov 2008, Rik van Riel wrote:
> Hugh Dickins wrote:
> > Moving lru_cache_add_active_or_unevictable() into page_add_new_anon_rmap()
> > was good but done stupidly: we should SetPageSwapBacked() there too; and
> > we know for sure that this anonymous, swap-backed page is not file cache.
> 
> True, but ...
> 
> > 	if (page_evictable(page, vma))
> > -		lru_cache_add_lru(page, LRU_ACTIVE +
> > page_is_file_cache(page));
> > +		lru_cache_add_lru(page, LRU_ACTIVE);
> 
> Then you will want to s/LRU_ACTIVE/LRU_ACTIVE_ANON/ here.

I thought it was deeply ingrained that LRU_ACTIVE == LRU_ACTIVE_ANON?
But it certainly looks better as you suggest, thanks - updated patch:


[PATCH 8/7] mm: further cleanup page_add_new_anon_rmap

Moving lru_cache_add_active_or_unevictable() into page_add_new_anon_rmap()
was good but stupid: we can and should SetPageSwapBacked() there too; and
we know for sure that this anonymous, swap-backed page is not file cache.

Signed-off-by: Hugh Dickins <hugh@veritas.com>
___
I suspect Nick might like to change it to __SetPageSwapBacked later.

 mm/memory.c |    3 ---
 mm/rmap.c   |    6 +++---
 2 files changed, 3 insertions(+), 6 deletions(-)

--- mmclean7/mm/memory.c	2008-11-19 15:26:28.000000000 +0000
+++ mmclean8/mm/memory.c	2008-11-22 19:02:35.000000000 +0000
@@ -1919,7 +1919,6 @@ gotten:
 		 * thread doing COW.
 		 */
 		ptep_clear_flush_notify(vma, address, page_table);
-		SetPageSwapBacked(new_page);
 		page_add_new_anon_rmap(new_page, vma, address);
 		set_pte_at(mm, address, page_table, entry);
 		update_mmu_cache(vma, address, entry);
@@ -2415,7 +2414,6 @@ static int do_anonymous_page(struct mm_s
 	if (!pte_none(*page_table))
 		goto release;
 	inc_mm_counter(mm, anon_rss);
-	SetPageSwapBacked(page);
 	page_add_new_anon_rmap(page, vma, address);
 	set_pte_at(mm, address, page_table, entry);
 
@@ -2563,7 +2561,6 @@ static int __do_fault(struct mm_struct *
 			entry = maybe_mkwrite(pte_mkdirty(entry), vma);
 		if (anon) {
 			inc_mm_counter(mm, anon_rss);
-			SetPageSwapBacked(page);
 			page_add_new_anon_rmap(page, vma, address);
 		} else {
 			inc_mm_counter(mm, file_rss);
--- mmclean7/mm/rmap.c	2008-11-19 15:26:33.000000000 +0000
+++ mmclean8/mm/rmap.c	2008-11-22 19:02:35.000000000 +0000
@@ -47,7 +47,6 @@
 #include <linux/rmap.h>
 #include <linux/rcupdate.h>
 #include <linux/module.h>
-#include <linux/mm_inline.h>
 #include <linux/kallsyms.h>
 #include <linux/memcontrol.h>
 #include <linux/mmu_notifier.h>
@@ -673,10 +672,11 @@ void page_add_new_anon_rmap(struct page 
 	struct vm_area_struct *vma, unsigned long address)
 {
 	VM_BUG_ON(address < vma->vm_start || address >= vma->vm_end);
-	atomic_set(&page->_mapcount, 0); /* elevate count by 1 (starts at -1) */
+	SetPageSwapBacked(page);
+	atomic_set(&page->_mapcount, 0); /* increment count (starts at -1) */
 	__page_set_anon_rmap(page, vma, address);
 	if (page_evictable(page, vma))
-		lru_cache_add_lru(page, LRU_ACTIVE + page_is_file_cache(page));
+		lru_cache_add_lru(page, LRU_ACTIVE_ANON);
 	else
 		add_page_to_unevictable_list(page);
 }

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

Re: [PATCH 8/7] mm: further cleanup page_add_new_anon_rmap

[Rik van Riel]

Hugh Dickins wrote:

> I thought it was deeply ingrained that LRU_ACTIVE == LRU_ACTIVE_ANON?
> But it certainly looks better as you suggest, thanks - updated patch:

Future proofing is good :)

> Signed-off-by: Hugh Dickins <hugh@veritas.com>

Acked-by: Rik van Riel <riel@redhat.com>

-- 
All rights reversed.

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

[PATCH 7/7] mm: make page_lock_anon_vma static

[Hugh Dickins]

page_lock_anon_vma() and page_unlock_anon_vma() were made available to
show_page_path() in vmscan.c; but now that has been removed, make them
static in rmap.c again, they're better kept private if possible.

Signed-off-by: Hugh Dickins <hugh@veritas.com>
---

 include/linux/rmap.h |    3 ---
 mm/rmap.c            |    4 ++--
 2 files changed, 2 insertions(+), 5 deletions(-)

--- mmclean6/include/linux/rmap.h	2008-10-24 09:28:24.000000000 +0100
+++ mmclean7/include/linux/rmap.h	2008-11-19 15:26:33.000000000 +0000
@@ -63,9 +63,6 @@ void anon_vma_unlink(struct vm_area_stru
 void anon_vma_link(struct vm_area_struct *);
 void __anon_vma_link(struct vm_area_struct *);
 
-extern struct anon_vma *page_lock_anon_vma(struct page *page);
-extern void page_unlock_anon_vma(struct anon_vma *anon_vma);
-
 /*
  * rmap interfaces called when adding or removing pte of page
  */
--- mmclean6/mm/rmap.c	2008-11-19 15:26:28.000000000 +0000
+++ mmclean7/mm/rmap.c	2008-11-19 15:26:33.000000000 +0000
@@ -193,7 +193,7 @@ void __init anon_vma_init(void)
  * Getting a lock on a stable anon_vma from a page off the LRU is
  * tricky: page_lock_anon_vma rely on RCU to guard against the races.
  */
-struct anon_vma *page_lock_anon_vma(struct page *page)
+static struct anon_vma *page_lock_anon_vma(struct page *page)
 {
 	struct anon_vma *anon_vma;
 	unsigned long anon_mapping;
@@ -213,7 +213,7 @@ out:
 	return NULL;
 }
 
-void page_unlock_anon_vma(struct anon_vma *anon_vma)
+static void page_unlock_anon_vma(struct anon_vma *anon_vma)
 {
 	spin_unlock(&anon_vma->lock);
 	rcu_read_unlock();

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

Re: [PATCH 7/7] mm: make page_lock_anon_vma static

[KOSAKI Motohiro]

> page_lock_anon_vma() and page_unlock_anon_vma() were made available to
> show_page_path() in vmscan.c; but now that has been removed, make them
> static in rmap.c again, they're better kept private if possible.
> 
> Signed-off-by: Hugh Dickins <hugh@veritas.com>

Absolutely.
Thank you.

	Reviewed-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>


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