[PATCH] splitlru: shmem_getpage SetPageSwapBacked sooner

[PATCH] splitlru: shmem_getpage SetPageSwapBacked sooner

[Hugh Dickins]

shmem_getpage must SetPageSwapBacked before the mem_cgroup_cache_charge,
otherwise try_to_free_mem_cgroup_pages cannot find the shmem/tmpfs pages
it needs to free, and OOMs result.

Signed-off-by: Hugh Dickins <hugh@veritas.com>
---
Should follow mmotm's define-page_file_cache-function.patch

 mm/shmem.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- mmotm/mm/shmem.c	2008-06-27 13:39:20.000000000 +0100
+++ linux/mm/shmem.c	2008-06-27 17:25:41.000000000 +0100
@@ -1369,6 +1369,7 @@ repeat:
 				error = -ENOMEM;
 				goto failed;
 			}
+			SetPageSwapBacked(filepage);
 
 			/* Precharge page while we can wait, compensate after */
 			error = mem_cgroup_cache_charge(filepage, current->mm,
@@ -1381,7 +1382,6 @@ repeat:
 				goto failed;
 			}
 
-			SetPageSwapBacked(filepage);
 			spin_lock(&info->lock);
 			entry = shmem_swp_alloc(info, idx, sgp);
 			if (IS_ERR(entry))

[PATCH] splitlru: memcg swapbacked pages active

[Hugh Dickins]

add_to_page_cache_lru puts PageSwapBacked pages on the active_anon lru,
so shouldn't mem_cgroup_charge_common mirror that by setting FLAG_ACTIVE?

Signed-off-by: Hugh Dickins <hugh@veritas.com>
---
Should follow mmotm's vmscan-split-lru-lists-into-anon-file-sets.patch

 mm/memcontrol.c |    2 ++
 1 file changed, 2 insertions(+)

--- mmotm/mm/memcontrol.c	2008-06-27 13:39:20.000000000 +0100
+++ linux/mm/memcontrol.c	2008-06-27 17:32:29.000000000 +0100
@@ -560,6 +560,8 @@ static int mem_cgroup_charge_common(stru
 		pc->flags = PAGE_CGROUP_FLAG_CACHE;
 		if (page_is_file_cache(page))
 			pc->flags |= PAGE_CGROUP_FLAG_FILE;
+		else
+			pc->flags |= PAGE_CGROUP_FLAG_ACTIVE;
 	} else
 		pc->flags = PAGE_CGROUP_FLAG_ACTIVE;
 

Re: [PATCH] splitlru: memcg swapbacked pages active

[KOSAKI Motohiro]

> add_to_page_cache_lru puts PageSwapBacked pages on the active_anon lru,
> so shouldn't mem_cgroup_charge_common mirror that by setting FLAG_ACTIVE?
> 
> Signed-off-by: Hugh Dickins <hugh@veritas.com>

I don't think so.

in global lru, active vs inactive anon ratio is controlled by 
inactive_anon_is_low().
but memcg doesn't have any sililar things.

thus, this patch increase active anon. and too many active page cause
decrease reclaim throuput.

yes, We should implement SEQ replacement reclaim (like global reclaim)
for memcg.
I'm working on this.

Re: [PATCH] splitlru: memcg swapbacked pages active

[Hugh Dickins]

On Sun, 29 Jun 2008, KOSAKI Motohiro wrote:
> > add_to_page_cache_lru puts PageSwapBacked pages on the active_anon lru,
> > so shouldn't mem_cgroup_charge_common mirror that by setting FLAG_ACTIVE?
> > 
> > Signed-off-by: Hugh Dickins <hugh@veritas.com>
> 
> I don't think so.
> 
> in global lru, active vs inactive anon ratio is controlled by 
> inactive_anon_is_low().
> but memcg doesn't have any sililar things.
> 
> thus, this patch increase active anon. and too many active page cause
> decrease reclaim throuput.
> 
> yes, We should implement SEQ replacement reclaim (like global reclaim)
> for memcg.
> I'm working on this.

Hmm.  Thanks for looking into it (and the others).  This is something
I noticed as a discrepancy in the source, rather than something from
which I observed any OOMs.  So if you're already working on bringing
the global and memcg approaches into line, I defer to your judgement.

But it seems to me worrying and fragile while they diverge in this way.
If the global has anon, active anon, file, active file and unevictable
lrus for the page; and the memcg has anon, active anon, file, active file
and unevictable lrus for the page cgroup; but different choices are made
which to put on where, then we're heading for confusion and trouble.

And when called, __mem_cgroup_move_lists tries to keep the activ-ity of
the memcg lrus in step with the activ-ity of the global lrus, doesn't
it?  So it's beyond my comprehension to start them off out of step.

Hugh

Re: [PATCH] splitlru: memcg swapbacked pages active

[KOSAKI Motohiro]

> But it seems to me worrying and fragile while they diverge in this way.
> If the global has anon, active anon, file, active file and unevictable
> lrus for the page; and the memcg has anon, active anon, file, active file
> and unevictable lrus for the page cgroup; but different choices are made
> which to put on where, then we're heading for confusion and trouble.
>
> And when called, __mem_cgroup_move_lists tries to keep the activ-ity of
> the memcg lrus in step with the activ-ity of the global lrus, doesn't
> it?  So it's beyond my comprehension to start them off out of step.

Well...
you are right.

Hmm.. OK, I propse alternative way.

step1: commit this patch
step2: implement active/inactive anon balancing routine

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

Thank you for good patch.


btw, furtunately, memcg reclaim has some retry.
thus, lru imbalancing doesn't cause OOM, it only cause small performance degression.
IMHO your patch doesn't have any risk.

Re: [PATCH] splitlru: memcg swapbacked pages active

[Hugh Dickins]

On Sun, 29 Jun 2008, KOSAKI Motohiro wrote:
> 
> Well...
> you are right.

Not proved!  This is all quite complex.  But it looks that way.

> 
> Hmm.. OK, I propse alternative way.
> 
> step1: commit this patch
> step2: implement active/inactive anon balancing routine

That sounds a good plan to me.

> 
> 	Reviewed-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
> 
> Thank you for good patch.

Thank you; and please accept my apology for not Cc'ing you on
the patches - I had intended to, but forgot just when sending.

> 
> btw, furtunately, memcg reclaim has some retry.
> thus, lru imbalancing doesn't cause OOM, it only cause small performance degression.
> IMHO your patch doesn't have any risk.

These things can work out so differently in practice than one would
expect.  Tests seem to be chugging along okay with the change in,
but I haven't explicitly tested performance with and without.

It just seems a sensible starting point to have the global and
memcg views in synch: if departing from that proves to work better,
then we should do so later.

I've no view yet on the performance characteristics of any of this,
just focussed on getting tmpfs working correctly again.  The notion
of keeping unevictable pages away from the regularly scanned lists
seems an obvious gain; but the swapbacked/filebacked distinction
is not so obvious to me (and particularly problematic for tmpfs).

Hugh

Re: [PATCH] splitlru: memcg swapbacked pages active

[KAMEZAWA Hiroyuki]

On Sun, 29 Jun 2008 01:22:22 +0100 (BST)
Hugh Dickins <hugh@veritas.com> wrote:

> add_to_page_cache_lru puts PageSwapBacked pages on the active_anon lru,
> so shouldn't mem_cgroup_charge_common mirror that by setting FLAG_ACTIVE?
> 
> Signed-off-by: Hugh Dickins <hugh@veritas.com>
> ---
> Should follow mmotm's vmscan-split-lru-lists-into-anon-file-sets.patch
> 
>  mm/memcontrol.c |    2 ++
>  1 file changed, 2 insertions(+)
> 
> --- mmotm/mm/memcontrol.c	2008-06-27 13:39:20.000000000 +0100
> +++ linux/mm/memcontrol.c	2008-06-27 17:32:29.000000000 +0100
> @@ -560,6 +560,8 @@ static int mem_cgroup_charge_common(stru
>  		pc->flags = PAGE_CGROUP_FLAG_CACHE;
>  		if (page_is_file_cache(page))
>  			pc->flags |= PAGE_CGROUP_FLAG_FILE;
> +		else
> +			pc->flags |= PAGE_CGROUP_FLAG_ACTIVE;
>  	} else
>  		pc->flags = PAGE_CGROUP_FLAG_ACTIVE;
>  
> 
Hmm, maybe no trouble but we'll have to care shmem's page is not marked as
PAGE_CGROUP_FLAG_CACHE but charged by mem_cgroup_charge_cache_page(). 
(I think why shmem is charged as cache is because shmem is used for tmpfs)

pc->flags |= PAGE_CGROUP_FLAG_FILE |  PAGE_CGROUP_FLAG_ACTIVE;

is bad ? (or not work ? I can't catch yet.)


And we have to change this again ;)
==
        [MEM_CGROUP_STAT_RSS] = { "anon/swapcache", PAGE_SIZE, },
==


Thanks,
-Kame

Re: [PATCH] splitlru: memcg swapbacked pages active

[Hugh Dickins]

On Mon, 30 Jun 2008, KAMEZAWA Hiroyuki wrote:
> On Sun, 29 Jun 2008 01:22:22 +0100 (BST)
> Hugh Dickins <hugh@veritas.com> wrote:
> > 
> > --- mmotm/mm/memcontrol.c	2008-06-27 13:39:20.000000000 +0100
> > +++ linux/mm/memcontrol.c	2008-06-27 17:32:29.000000000 +0100
> > @@ -560,6 +560,8 @@ static int mem_cgroup_charge_common(stru
> >  		pc->flags = PAGE_CGROUP_FLAG_CACHE;
> >  		if (page_is_file_cache(page))
> >  			pc->flags |= PAGE_CGROUP_FLAG_FILE;
> > +		else
> > +			pc->flags |= PAGE_CGROUP_FLAG_ACTIVE;
> >  	} else
> >  		pc->flags = PAGE_CGROUP_FLAG_ACTIVE;
> >  
>
> Hmm, maybe no trouble but we'll have to care shmem's page is not marked as
> PAGE_CGROUP_FLAG_CACHE but charged by mem_cgroup_charge_cache_page(). 
> (I think why shmem is charged as cache is because shmem is used for tmpfs)
> 
> pc->flags |= PAGE_CGROUP_FLAG_FILE |  PAGE_CGROUP_FLAG_ACTIVE;
> 
> is bad ? (or not work ? I can't catch yet.)

Sorry, I've tried several times to understand you here, but still don't.

I thought shmem's page is marked as PAGE_CGROUP_FLAG_CACHE, and is
charged by mem_cgroup_charge_cache_page(), and that's what memcg wants?

It is a bit weird that PAGE_CGROUP_FLAG_CACHE and PAGE_CGROUP_FLAG_FILE
diverge, I'm not thrilled with that: that's an aspect of splitlru
treating shmem/tmpfs pages as not-file pages, which I'm unhappy with.

Hugh

[RFC][PATCH] memcg: change shmem handler.

[KAMEZAWA Hiroyuki]

On Sun, 29 Jun 2008 01:22:22 +0100 (BST)
Hugh Dickins <hugh@veritas.com> wrote:

> add_to_page_cache_lru puts PageSwapBacked pages on the active_anon lru,
> so shouldn't mem_cgroup_charge_common mirror that by setting FLAG_ACTIVE?
> 
> Signed-off-by: Hugh Dickins <hugh@veritas.com>

How about a patch like this ?
==
a RFC patch: memcg-change-shmem-handler. 

Should be divided into 2-4 patches, and may have some problem.
(I did many careless misses today.....)

But please see idea/concepts. Maybe right way.

shmem has following characteristics.
In general.
  - seems like file cache
  - swap-backed
  - when it's swapped out, it's removed from radix-tree and added to swap.
  - when it's swapped in, it's removed from swap and added to radix-tree.

With memcg.
  - shmem is treted just as a file-cache. So, started from inactive list.
  - shmem's page fault routine is sensitive to GFP_xxx in which used.
    (GFP_NOWAIT is used) and pre-charge is done before add_to_page_cache.
  - shmem's page is removed by mem_cgroup_uncharge_cache_page(), So,
    shmem's swapcache is not charged.

This patch fixes some mess by
  - PAGE_CGROUP_FLAG_CACHE is deleted (and replaced by FLAG_FILE)
  - PAGE_CGROUP_FLAG_SHMEM is added.
  - add_to_page_cache_nocharge() is added.
    This avoids mem_cgroup_charge_cache_page(). This is useful when page is
    pre-charged.
  - uses add_to_page_cache_nocharge() also in hugemem.
    (I think hugemem controller should be independent from memcg.
     Balbir, how do you think ?)
  - PageSwapBacked() is checked.
    (A imported patch from Hugh Dickins)

As result.
  - shmem will be in SwapBacked/Active list at first.
  - memcg has "shmem/tmpfs" counter.

Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>

--
 include/linux/pagemap.h |   16 +++++++++
 mm/filemap.c            |   49 ++++++++++++++++++++++--------
 mm/hugetlb.c            |    3 +
 mm/memcontrol.c         |   78 +++++++++++++++++++++++++-----------------------
 mm/shmem.c              |   17 ++++++----
 5 files changed, 107 insertions(+), 56 deletions(-)

Index: linux-2.6.26-rc5-mm3-kosaki/mm/memcontrol.c
===================================================================
--- linux-2.6.26-rc5-mm3-kosaki.orig/mm/memcontrol.c	2008-06-30 15:02:52.000000000 +0900
+++ linux-2.6.26-rc5-mm3-kosaki/mm/memcontrol.c	2008-06-30 16:26:34.000000000 +0900
@@ -49,6 +49,7 @@
 	 */
 	MEM_CGROUP_STAT_CACHE, 	   /* # of pages charged as cache */
 	MEM_CGROUP_STAT_RSS,	   /* # of pages charged as anon/swapcache */
+	MEM_CGROUP_STAT_SHMEM,     /* # of pages charges as shmem/tmpfs */
 	MEM_CGROUP_STAT_PGPGIN_COUNT,	/* # of pages paged in */
 	MEM_CGROUP_STAT_PGPGOUT_COUNT,	/* # of pages paged out */
 
@@ -160,10 +161,10 @@
 	struct mem_cgroup *mem_cgroup;
 	int flags;
 };
-#define PAGE_CGROUP_FLAG_CACHE	   (0x1)	/* charged as cache */
-#define PAGE_CGROUP_FLAG_ACTIVE    (0x2)	/* page is active in this cgroup */
-#define PAGE_CGROUP_FLAG_FILE	   (0x4)	/* page is file system backed */
-#define PAGE_CGROUP_FLAG_UNEVICTABLE (0x8)	/* page is unevictableable */
+#define PAGE_CGROUP_FLAG_ACTIVE    (0x1)	/* page is active in this cgroup */
+#define PAGE_CGROUP_FLAG_FILE	   (0x2)	/* page is file system backed */
+#define PAGE_CGROUP_FLAG_UNEVICTABLE (0x4)	/* page is unevictableable */
+#define PAGE_CGROUP_FLAG_SHMEM	   (0x8)       /* page is shmem/tmpfs */
 
 static int page_cgroup_nid(struct page_cgroup *pc)
 {
@@ -178,6 +179,7 @@
 enum charge_type {
 	MEM_CGROUP_CHARGE_TYPE_CACHE = 0,
 	MEM_CGROUP_CHARGE_TYPE_MAPPED,
+	MEM_CGROUP_CHARGE_TYPE_SHMEM,
 	MEM_CGROUP_CHARGE_TYPE_FORCE,	/* used by force_empty */
 };
 
@@ -191,8 +193,10 @@
 	struct mem_cgroup_stat *stat = &mem->stat;
 
 	VM_BUG_ON(!irqs_disabled());
-	if (flags & PAGE_CGROUP_FLAG_CACHE)
+	if (flags & PAGE_CGROUP_FLAG_FILE)
 		__mem_cgroup_stat_add_safe(stat, MEM_CGROUP_STAT_CACHE, val);
+	else if (flags & PAGE_CGROUP_FLAG_SHMEM)
+		__mem_cgroup_stat_add_safe(stat, MEM_CGROUP_STAT_SHMEM, val);
 	else
 		__mem_cgroup_stat_add_safe(stat, MEM_CGROUP_STAT_RSS, val);
 
@@ -573,12 +577,19 @@
 	 * If a page is accounted as a page cache, insert to inactive list.
 	 * If anon, insert to active list.
 	 */
-	if (ctype == MEM_CGROUP_CHARGE_TYPE_CACHE) {
-		pc->flags = PAGE_CGROUP_FLAG_CACHE;
-		if (page_is_file_cache(page))
-			pc->flags |= PAGE_CGROUP_FLAG_FILE;
-	} else
+	switch (ctype) {
+	case MEM_CGROUP_CHARGE_TYPE_CACHE:
+		pc->flags = PAGE_CGROUP_FLAG_FILE;
+		break;
+	case MEM_CGROUP_CHARGE_TYPE_SHMEM:
+		pc->flags = PAGE_CGROUP_FLAG_SHMEM | PAGE_CGROUP_FLAG_ACTIVE;
+		break;
+	case MEM_CGROUP_CHARGE_TYPE_MAPPED:
 		pc->flags = PAGE_CGROUP_FLAG_ACTIVE;
+		break;
+	default:
+		BUG();
+	}
 
 	lock_page_cgroup(page);
 	if (unlikely(page_get_page_cgroup(page))) {
@@ -625,28 +636,10 @@
 int mem_cgroup_cache_charge(struct page *page, struct mm_struct *mm,
 				gfp_t gfp_mask)
 {
-	/*
-	 * Corner case handling. This is called from add_to_page_cache()
-	 * in usual. But some FS (shmem) precharges this page before calling it
-	 * and call add_to_page_cache() with GFP_NOWAIT.
-	 *
-	 * For GFP_NOWAIT case, the page may be pre-charged before calling
-	 * add_to_page_cache(). (See shmem.c) check it here and avoid to call
-	 * charge twice. (It works but has to pay a bit larger cost.)
-	 */
-	if (!(gfp_mask & __GFP_WAIT)) {
-		struct page_cgroup *pc;
+	enum charge_type ctype = MEM_CGROUP_CHARGE_TYPE_CACHE;
 
-		lock_page_cgroup(page);
-		pc = page_get_page_cgroup(page);
-		if (pc) {
-			VM_BUG_ON(pc->page != page);
-			VM_BUG_ON(!pc->mem_cgroup);
-			unlock_page_cgroup(page);
-			return 0;
-		}
-		unlock_page_cgroup(page);
-	}
+	if (PageSwapBacked(page))
+		ctype = MEM_CGROUP_CHARGE_TYPE_SHMEM;
 
 	if (unlikely(!mm))
 		mm = &init_mm;
@@ -678,11 +671,19 @@
 		goto unlock;
 
 	VM_BUG_ON(pc->page != page);
-
+	/*
+	 * There are 2 cases.
+	 * 1. anon pages are swapped out.
+	 * 2. shmem pages are swapped out.
+	 * In both case, PageSwapCache() returns 1 and we don't want to
+	 * uncharge it.
+	 */
+	if (PageSwapCache(page))
+		goto unlock;
+	/* When the page is unmapped, file-cache and shmem memory is alive */
 	if ((ctype == MEM_CGROUP_CHARGE_TYPE_MAPPED)
-	    && ((pc->flags & PAGE_CGROUP_FLAG_CACHE)
-		|| page_mapped(page)
-		|| PageSwapCache(page)))
+	    && ((pc->flags & (PAGE_CGROUP_FLAG_FILE | PAGE_CGROUP_FLAG_SHMEM))
+		|| page_mapped(page)))
 		goto unlock;
 
 	mz = page_cgroup_zoneinfo(pc);
@@ -732,8 +733,10 @@
 	if (pc) {
 		mem = pc->mem_cgroup;
 		css_get(&mem->css);
-		if (pc->flags & PAGE_CGROUP_FLAG_CACHE)
+		if (pc->flags & PAGE_CGROUP_FLAG_FILE)
 			ctype = MEM_CGROUP_CHARGE_TYPE_CACHE;
+		else if (pc->flags & PAGE_CGROUP_FLAG_SHMEM)
+			ctype = MEM_CGROUP_CHARGE_TYPE_SHMEM;
 	}
 	unlock_page_cgroup(page);
 	if (mem) {
@@ -785,6 +788,8 @@
 		progress = try_to_free_mem_cgroup_pages(mem, gfp_mask);
 	} while (!progress && --retry);
 
+	css_put(&mem->css);
+
 	if (!retry)
 		return -ENOMEM;
 	return 0;
@@ -920,6 +925,7 @@
 } mem_cgroup_stat_desc[] = {
 	[MEM_CGROUP_STAT_CACHE] = { "cache", PAGE_SIZE, },
 	[MEM_CGROUP_STAT_RSS] = { "anon/swapcache", PAGE_SIZE, },
+	[MEM_CGROUP_STAT_SHMEM] = { "shmem/tmpfs", PAGE_SIZE, },
 	[MEM_CGROUP_STAT_PGPGIN_COUNT] = {"pgpgin", 1, },
 	[MEM_CGROUP_STAT_PGPGOUT_COUNT] = {"pgpgout", 1, },
 };
Index: linux-2.6.26-rc5-mm3-kosaki/include/linux/pagemap.h
===================================================================
--- linux-2.6.26-rc5-mm3-kosaki.orig/include/linux/pagemap.h	2008-06-30 15:00:04.000000000 +0900
+++ linux-2.6.26-rc5-mm3-kosaki/include/linux/pagemap.h	2008-06-30 15:03:02.000000000 +0900
@@ -258,6 +258,22 @@
 				pgoff_t index, gfp_t gfp_mask);
 int add_to_page_cache_lru(struct page *page, struct address_space *mapping,
 				pgoff_t index, gfp_t gfp_mask);
+
+#ifdef CONFIG_CGROUP_MEM_RES_CTLR
+/*
+ * If the page is pre-charged before add_to_page_cache() this routine is
+ * light-weight.
+ */
+int add_to_page_cache_nocharge(struct page *page, struct address_space *mapping,
+				pgoff_t index, gfp_t gfp_mask);
+#else
+static inline int add_to_page_cache_nocharge(struct page *page,
+				struct address_space *mapping, pgoff_t index,
+				gfp_t mask)
+{
+	return add_to_page_cache(page, mapping, index, gfp_mask);
+}
+#endif
 extern void remove_from_page_cache(struct page *page);
 extern void __remove_from_page_cache(struct page *page);
 
Index: linux-2.6.26-rc5-mm3-kosaki/mm/filemap.c
===================================================================
--- linux-2.6.26-rc5-mm3-kosaki.orig/mm/filemap.c	2008-06-30 15:00:04.000000000 +0900
+++ linux-2.6.26-rc5-mm3-kosaki/mm/filemap.c	2008-06-30 15:23:36.000000000 +0900
@@ -442,8 +442,9 @@
 	return err;
 }
 
+
 /**
- * add_to_page_cache - add newly allocated pagecache pages
+ * add_to_page_cache_nocharge() - add newly allocated pagecache pages
  * @page:	page to add
  * @mapping:	the page's address_space
  * @offset:	page index
@@ -454,22 +455,20 @@
  * The other page state flags were set by rmqueue().
  *
  * This function does not add the page to the LRU.  The caller must do that.
+ * This doesn't call memory resource control routine.
  */
-int add_to_page_cache(struct page *page, struct address_space *mapping,
-		pgoff_t offset, gfp_t gfp_mask)
+
+int add_to_page_cache_nocharge(struct page *page,
+			       struct address_space *mapping,
+			       pgoff_t offset, gfp_t gfp_mask)
 {
-	int error = mem_cgroup_cache_charge(page, current->mm,
-					gfp_mask & ~__GFP_HIGHMEM);
-	if (error)
-		goto out;
 
-	error = radix_tree_preload(gfp_mask & ~__GFP_HIGHMEM);
+	int error = radix_tree_preload(gfp_mask & ~__GFP_HIGHMEM);
 	if (error == 0) {
 		page_cache_get(page);
 		SetPageLocked(page);
 		page->mapping = mapping;
 		page->index = offset;
-
 		spin_lock_irq(&mapping->tree_lock);
 		error = radix_tree_insert(&mapping->page_tree, offset, page);
 		if (likely(!error)) {
@@ -478,15 +477,39 @@
 		} else {
 			page->mapping = NULL;
 			ClearPageLocked(page);
-			mem_cgroup_uncharge_cache_page(page);
 			page_cache_release(page);
 		}
 
 		spin_unlock_irq(&mapping->tree_lock);
 		radix_tree_preload_end();
-	} else
-		mem_cgroup_uncharge_cache_page(page);
-out:
+	}
+	return error;
+}
+
+/**
+ * add_to_page_cache - add newly allocated pagecache pages
+ * @page:	page to add
+ * @mapping:	the page's address_space
+ * @offset:	page index
+ * @gfp_mask:	page allocation mode
+ *
+ * This function is used to add newly allocated pagecache pages;
+ * the page is new, so we can just run SetPageLocked() against it.
+ * The other page state flags were set by rmqueue().
+ *
+ * This function does not add the page to the LRU.  The caller must do that.
+ */
+int add_to_page_cache(struct page *page, struct address_space *mapping,
+		pgoff_t offset, gfp_t gfp_mask)
+{
+	int error = mem_cgroup_cache_charge(page, current->mm,
+					gfp_mask & ~__GFP_HIGHMEM);
+	if (!error) {
+		error = add_to_page_cache_nocharge(page, mapping, offset,
+						  gfp_mask);
+		if (error)
+			mem_cgroup_uncharge_cache_page(page);
+	}
 	return error;
 }
 EXPORT_SYMBOL(add_to_page_cache);
Index: linux-2.6.26-rc5-mm3-kosaki/mm/shmem.c
===================================================================
--- linux-2.6.26-rc5-mm3-kosaki.orig/mm/shmem.c	2008-06-30 15:00:04.000000000 +0900
+++ linux-2.6.26-rc5-mm3-kosaki/mm/shmem.c	2008-06-30 15:37:11.000000000 +0900
@@ -940,10 +940,8 @@
 	spin_lock(&info->lock);
 	ptr = shmem_swp_entry(info, idx, NULL);
 	if (ptr && ptr->val == entry.val)
-		error = add_to_page_cache(page, inode->i_mapping, idx,
+		error = add_to_page_cache_nocharge(page, inode->i_mapping, idx,
 					GFP_NOWAIT);
-	else /* we don't have to account this page. */
-		mem_cgroup_uncharge_cache_page(page);
 
 	if (error == -EEXIST) {
 		struct page *filepage = find_get_page(inode->i_mapping, idx);
@@ -955,6 +953,7 @@
 			 */
 			if (PageUptodate(filepage))
 				error = 0;
+			mem_cgroup_uncharge_cache_page(page);
 			page_cache_release(filepage);
 		}
 	}
@@ -965,7 +964,9 @@
 		shmem_swp_set(info, ptr, 0);
 		swap_free(entry);
 		error = 1;	/* not an error, but entry was found */
-	}
+	} else
+		mem_cgroup_uncharge_cache_page(page);
+
 	if (ptr)
 		shmem_swp_unmap(ptr);
 	spin_unlock(&info->lock);
@@ -1375,6 +1376,7 @@
 				error = -ENOMEM;
 				goto failed;
 			}
+			SetPageSwapBacked(filepage);
 
 			/* Precharge page while we can wait, compensate after */
 			error = mem_cgroup_cache_charge(filepage, current->mm,
@@ -1387,7 +1389,6 @@
 				goto failed;
 			}
 
-			SetPageSwapBacked(filepage);
 			spin_lock(&info->lock);
 			entry = shmem_swp_alloc(info, idx, sgp);
 			if (IS_ERR(entry))
@@ -1400,7 +1401,8 @@
 			if (ret)
 				mem_cgroup_uncharge_cache_page(filepage);
 			else
-				ret = add_to_page_cache_lru(filepage, mapping,
+				ret = add_to_page_cache_nocharge(filepage,
+						mapping,
 						idx, GFP_NOWAIT);
 			/*
 			 * At add_to_page_cache_lru() failure, uncharge will
@@ -1408,6 +1410,7 @@
 			 */
 			if (ret) {
 				spin_unlock(&info->lock);
+				mem_cgroup_uncharge_cache_page(filepage);
 				page_cache_release(filepage);
 				shmem_unacct_blocks(info->flags, 1);
 				shmem_free_blocks(inode, 1);
@@ -1415,6 +1418,8 @@
 				if (error)
 					goto failed;
 				goto repeat;
+			} else {
+				lru_cache_add_active_anon(filepage);
 			}
 			info->flags |= SHMEM_PAGEIN;
 		}
Index: linux-2.6.26-rc5-mm3-kosaki/mm/hugetlb.c
===================================================================
--- linux-2.6.26-rc5-mm3-kosaki.orig/mm/hugetlb.c	2008-06-25 18:07:09.000000000 +0900
+++ linux-2.6.26-rc5-mm3-kosaki/mm/hugetlb.c	2008-06-30 15:08:16.000000000 +0900
@@ -1813,7 +1813,8 @@
 			int err;
 			struct inode *inode = mapping->host;
 
-			err = add_to_page_cache(page, mapping, idx, GFP_KERNEL);
+			err = add_to_page_cache_nocharge(page, mapping,
+						idx, GFP_KERNEL);
 			if (err) {
 				put_page(page);
 				if (err == -EEXIST)

Re: [RFC][PATCH] memcg: change shmem handler.

[Hugh Dickins]

On Mon, 30 Jun 2008, KAMEZAWA Hiroyuki wrote:
> 
> How about a patch like this ?
> ==
> a RFC patch: memcg-change-shmem-handler. 
> 
> Should be divided into 2-4 patches, and may have some problem.
> (I did many careless misses today.....)
> 
> But please see idea/concepts. Maybe right way.

As you say, should be divided.  In particular, some changes are
assuming the splitlru patches (e.g. SwapBacked), and other changes
are independent of them.  Those very much have to be separated.

> 
> shmem has following characteristics.
> In general.
>   - seems like file cache
>   - swap-backed
>   - when it's swapped out, it's removed from radix-tree and added to swap.
>   - when it's swapped in, it's removed from swap and added to radix-tree.

True (aside from swap pages being in a radix-tree of their own).
Though splitlru is muddying the "seems like file cache".

> 
> With memcg.
>   - shmem is treted just as a file-cache. So, started from inactive list.

Depends on what set of patches we're talking about.

>   - shmem's page fault routine is sensitive to GFP_xxx in which used.
>     (GFP_NOWAIT is used) and pre-charge is done before add_to_page_cache.

Nothing particular to shmem or page fault routine, I think; but
shmem implementation is peculiar in calling add_to_page_cache etc.
while holding a spinlock, so needs to precharge, yes.

>   - shmem's page is removed by mem_cgroup_uncharge_cache_page(), So,
>     shmem's swapcache is not charged.

Ah, that's interesting: I'd assumed you'd changed that in your
no-refcount patches, and had been surprised not to notice a slowdown
(waiting for swap to be written and freed before coming under limit).
Now you want to make them wait: not entirely an improvement,
but I see your point.

> 
> This patch fixes some mess by
>   - PAGE_CGROUP_FLAG_CACHE is deleted (and replaced by FLAG_FILE)

That's good.

>   - PAGE_CGROUP_FLAG_SHMEM is added.

That's not good.

>   - add_to_page_cache_nocharge() is added.
>     This avoids mem_cgroup_charge_cache_page(). This is useful when page is
>     pre-charged.

Do you have to?  I get so sick of such variants.  I agree the GFP_NOWAIT
test looked rather a hack, but it's really quite appropriate.  Fragile
in that it relies on the right thing having been done; but there's a
lot of fragility in the way the memcg microcosm is hoping to mimic
the global macrocosm.  (Sorry if I'm being pretentiously obscure!)

>   - uses add_to_page_cache_nocharge() also in hugemem.
>     (I think hugemem controller should be independent from memcg.
>      Balbir, how do you think ?)
>   - PageSwapBacked() is checked.
>     (A imported patch from Hugh Dickins)

Nothing to do with the rest of it?

> 
> As result.
>   - shmem will be in SwapBacked/Active list at first.

Assuming splitlru.  Didn't my two-liner deal with that?

>   - memcg has "shmem/tmpfs" counter.

Is that a good thing?  If we really decide that globally we
need such a counter, then fine for memcg to follow; but I've
not yet heard it asked for.

> 
> Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> 
> --
>  include/linux/pagemap.h |   16 +++++++++
>  mm/filemap.c            |   49 ++++++++++++++++++++++--------
>  mm/hugetlb.c            |    3 +
>  mm/memcontrol.c         |   78 +++++++++++++++++++++++++-----------------------
>  mm/shmem.c              |   17 ++++++----
>  5 files changed, 107 insertions(+), 56 deletions(-)

Not so good (though hardly the end of the world).

Hugh

Re: Re: [RFC][PATCH] memcg: change shmem handler.

[kamezawa.hiroyu]

----- Original Message -----
>> 
>> With memcg.
>>   - shmem is treted just as a file-cache. So, started from inactive list.
>
>Depends on what set of patches we're talking about.
>
just means it's charged by mem_cgroup_cache_charge()

>>   - shmem's page fault routine is sensitive to GFP_xxx in which used.
>>     (GFP_NOWAIT is used) and pre-charge is done before add_to_page_cache.
>
>Nothing particular to shmem or page fault routine, I think; but
>shmem implementation is peculiar in calling add_to_page_cache etc.
>while holding a spinlock, so needs to precharge, yes.
>
>>   - shmem's page is removed by mem_cgroup_uncharge_cache_page(), So,
>>     shmem's swapcache is not charged.
>
>Ah, that's interesting: I'd assumed you'd changed that in your
>no-refcount patches, and had been surprised not to notice a slowdown
>(waiting for swap to be written and freed before coming under limit).
>Now you want to make them wait: not entirely an improvement,
>but I see your point.
>
To do this is (maybe) a few line patch. I'll CC you if I wrote some.

>> 
>> This patch fixes some mess by
>>   - PAGE_CGROUP_FLAG_CACHE is deleted (and replaced by FLAG_FILE)
>
>That's good.
>
will do as an independent patch.

>>   - PAGE_CGROUP_FLAG_SHMEM is added.
>
>That's not good.
>
Hmm

>>   - add_to_page_cache_nocharge() is added.
>>     This avoids mem_cgroup_charge_cache_page(). This is useful when page is
>>     pre-charged.
>
>Do you have to?  I get so sick of such variants.  I agree the GFP_NOWAIT
>test looked rather a hack, but it's really quite appropriate.  Fragile
>in that it relies on the right thing having been done; but there's a
>lot of fragility in the way the memcg microcosm is hoping to mimic
>the global macrocosm.  (Sorry if I'm being pretentiously obscure!)
>
Hmm..okay, find a way to detect precharged case without adding anything.

>>   - uses add_to_page_cache_nocharge() also in hugemem.
>>     (I think hugemem controller should be independent from memcg.
>>      Balbir, how do you think ?)
>>   - PageSwapBacked() is checked.
>>     (A imported patch from Hugh Dickins)
>
>Nothing to do with the rest of it?
>
Just imported I need it to this. I'm sorry if I don't catch what you mean.


>> 
>> As result.
>>   - shmem will be in SwapBacked/Active list at first.
>
>Assuming splitlru.  Didn't my two-liner deal with that?
>
yes. yours do. just want to use switch-case rather than unclear "if" s.

>>   - memcg has "shmem/tmpfs" counter.
>
>Is that a good thing?  If we really decide that globally we
>need such a counter, then fine for memcg to follow; but I've
>not yet heard it asked for.
After swap-controller is introduced, I can imagine there will be a
swap-full/swap-less cgroup. And shmem will be able to be swapped out.
memcg handles limit of memory usage and Admin/Middleware will want to
know current limit is good or bad. So, showing amount of tmpfs 
will be good (It's now shown as Cache...a pages easily kicked out ;)

>
>> 
>> Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
>> 
>> --
>>  include/linux/pagemap.h |   16 +++++++++
>>  mm/filemap.c            |   49 ++++++++++++++++++++++--------
>>  mm/hugetlb.c            |    3 +
>>  mm/memcontrol.c         |   78 +++++++++++++++++++++++++------------------
-----
>>  mm/shmem.c              |   17 ++++++----
>>  5 files changed, 107 insertions(+), 56 deletions(-)
>
>Not so good (though hardly the end of the world).
>
I'll divide and make this clearer.
Anyway I want to wait until -mm's VMM seems stable.

Thanks,
-Kame

[RFC][PATCH] memcg: shmem swap cache

[KAMEZAWA Hiroyuki]

On Tue, 1 Jul 2008 09:56:45 +0900 (JST)
kamezawa.hiroyu@jp.fujitsu.com wrote:

> >>   - shmem's page is removed by mem_cgroup_uncharge_cache_page(), So,
> >>     shmem's swapcache is not charged.
> >
> >Ah, that's interesting: I'd assumed you'd changed that in your
> >no-refcount patches, and had been surprised not to notice a slowdown
> >(waiting for swap to be written and freed before coming under limit).
> >Now you want to make them wait: not entirely an improvement,
> >but I see your point.
> >
> To do this is (maybe) a few line patch. I'll CC you if I wrote some.
> 
maybe like this. how do you feel ?
I'll stack this on my queue and test for a while.

==
SwapCache handling fix.

memcg failed to handle shmem's swapcache. This patch tries to fix it.

After this:

Any page marked as SwapCache is not uncharged.
(until delelte_from_swap_cache() delete the flag.)

To check a page is alive shmem-page-cache or not we use
 page->mapping && !PageAnon(page) instead of pc->flags & PAGE_CGROUP_FLAG_CACHE.

Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>

Index: test-2.6.26-rc5-mm3++/mm/memcontrol.c
===================================================================
--- test-2.6.26-rc5-mm3++.orig/mm/memcontrol.c	2008-07-02 09:29:52.000000000 +0900
+++ test-2.6.26-rc5-mm3++/mm/memcontrol.c	2008-07-02 10:58:15.000000000 +0900
@@ -685,11 +685,45 @@
 
 	VM_BUG_ON(pc->page != page);
 
-	if ((ctype == MEM_CGROUP_CHARGE_TYPE_MAPPED)
-	    && ((pc->flags & PAGE_CGROUP_FLAG_CACHE)
-		|| page_mapped(page)
-		|| PageSwapCache(page)))
+	/*
+	 * File Cache
+	 * If called with MEM_CGROUP_CHARGE_TYPE_MAPPED, check page->mapping.
+	 * add_to_page_cache() .... charged before inserting radix-tree.
+	 * remove_from_page_cache() .... uncharged at removing from radix-tree.
+	 * page->mapping && !PageAnon(page) catches file cache.
+	 *
+	 * Anon/Shmem.....We check PageSwapCache(page).
+	 * Anon .... charged before mapped.
+	 * Shmem .... charged at add_to_page_cache() as usual File Cache.
+	 *
+	 * This page will be finally uncharged when removed from swap-cache
+	 *
+	 * we treat 2 cases here.
+	 * A. anonymous page  B. shmem.
+	 * We never uncharge if page is marked as SwapCache.
+	 * add_to_swap_cache() have nothing to do with charge/uncharge.
+	 * SwapCache flag is deleted before delete_from_swap_cache() calls this
+	 *
+	 * shmem's behavior is following. (see shmem.c/swap_state.c also)
+	 * at swap-out:
+	 * 	0. add_to_page_cache()//charged at page creation.
+	 * 	1. add_to_swap_cache() (marked as SwapCache)
+	 *	2. remove_from_page_cache().  (calls this.)
+	 *	(finally) delete_from_swap_cache(). (calls this.)
+	 * at swap-in:
+	 * 	3. add_to_swap_cache() (no charge here.)
+	 * 	4. add_to_page_cache() (charged here.)
+	 * 	5. delete_from_swap_cache() (calls this.)
+	 * PageSwapCache(page) catches "2".
+	 * page->mapping && !PageAnon() catches "5" and avoid uncharging.
+	 */
+	if (PageSwapCache(page))
 		goto unlock;
+	/* called from unmap or delete_from_swap_cache() */
+	if ((ctype == MEM_CGROUP_CHARGE_TYPE_MAPPED)
+		&& (page_mapped(page)
+		    || (page->mapping && !PageAnon(page))))/* alive cache ? */
+			goto unlock;
 
 	mz = page_cgroup_zoneinfo(pc);
 	spin_lock_irqsave(&mz->lru_lock, flags);

Re: Re: Re: [RFC][PATCH] memcg: change shmem handler.

[kamezawa.hiroyu]

----- Original Message -----
Sorry..a bit more words.
>>Is that a good thing?  If we really decide that globally we
>>need such a counter, then fine for memcg to follow; but I've
>>not yet heard it asked for.
>After swap-controller is introduced, I can imagine there will be a
>swap-full/swap-less cgroup. And shmem will be able to be swapped out.
                                          not
>memcg handles limit of memory usage and Admin/Middleware will want to
>know current limit is good or bad. So, showing amount of tmpfs 
>will be good (It's now shown as Cache...a pages easily kicked out ;)
>
I'm not sure but how this seems under memcg in which DataBase runs.

Now.
   - anon
   - cache (including DataBase's shmem)
After
   - anon
   - cache (file cache)
   - shmem (DataBase's working area)

not good ?

Thanks,
-Kame

Re: [RFC][PATCH] memcg: change shmem handler.

[Balbir Singh]

KAMEZAWA Hiroyuki wrote:
> On Sun, 29 Jun 2008 01:22:22 +0100 (BST)
> Hugh Dickins <hugh@veritas.com> wrote:
> 
>> add_to_page_cache_lru puts PageSwapBacked pages on the active_anon lru,
>> so shouldn't mem_cgroup_charge_common mirror that by setting FLAG_ACTIVE?
>>
>> Signed-off-by: Hugh Dickins <hugh@veritas.com>
> 
> How about a patch like this ?
> ==
> a RFC patch: memcg-change-shmem-handler. 
> 
> Should be divided into 2-4 patches, and may have some problem.
> (I did many careless misses today.....)
> 
> But please see idea/concepts. Maybe right way.
> 
> shmem has following characteristics.
> In general.
>   - seems like file cache
>   - swap-backed
>   - when it's swapped out, it's removed from radix-tree and added to swap.
>   - when it's swapped in, it's removed from swap and added to radix-tree.
> 
> With memcg.
>   - shmem is treted just as a file-cache. So, started from inactive list.
>   - shmem's page fault routine is sensitive to GFP_xxx in which used.
>     (GFP_NOWAIT is used) and pre-charge is done before add_to_page_cache.
>   - shmem's page is removed by mem_cgroup_uncharge_cache_page(), So,
>     shmem's swapcache is not charged.
> 
> This patch fixes some mess by
>   - PAGE_CGROUP_FLAG_CACHE is deleted (and replaced by FLAG_FILE)
>   - PAGE_CGROUP_FLAG_SHMEM is added.
>   - add_to_page_cache_nocharge() is added.
>     This avoids mem_cgroup_charge_cache_page(). This is useful when page is
>     pre-charged.
>   - uses add_to_page_cache_nocharge() also in hugemem.
>     (I think hugemem controller should be independent from memcg.
>      Balbir, how do you think ?)

I am not 100% sure of that right now. I definitely want different control
parameters (not included as a part of memory.limit_in_bytes). If there is
leverage from memory controller, we could consider adding that to it or a
separate controller if that makes more sense.

>   - PageSwapBacked() is checked.
>     (A imported patch from Hugh Dickins)
> 
> As result.
>   - shmem will be in SwapBacked/Active list at first.

Good

>   - memcg has "shmem/tmpfs" counter.
> 

I think this will be useful too (but I need to play with it)

> Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> 
> --
>  include/linux/pagemap.h |   16 +++++++++
>  mm/filemap.c            |   49 ++++++++++++++++++++++--------
>  mm/hugetlb.c            |    3 +
>  mm/memcontrol.c         |   78 +++++++++++++++++++++++++-----------------------
>  mm/shmem.c              |   17 ++++++----
>  5 files changed, 107 insertions(+), 56 deletions(-)
> 
> Index: linux-2.6.26-rc5-mm3-kosaki/mm/memcontrol.c
> ===================================================================
> --- linux-2.6.26-rc5-mm3-kosaki.orig/mm/memcontrol.c	2008-06-30 15:02:52.000000000 +0900
> +++ linux-2.6.26-rc5-mm3-kosaki/mm/memcontrol.c	2008-06-30 16:26:34.000000000 +0900
> @@ -49,6 +49,7 @@
>  	 */
>  	MEM_CGROUP_STAT_CACHE, 	   /* # of pages charged as cache */
>  	MEM_CGROUP_STAT_RSS,	   /* # of pages charged as anon/swapcache */
> +	MEM_CGROUP_STAT_SHMEM,     /* # of pages charges as shmem/tmpfs */
>  	MEM_CGROUP_STAT_PGPGIN_COUNT,	/* # of pages paged in */
>  	MEM_CGROUP_STAT_PGPGOUT_COUNT,	/* # of pages paged out */
> 
> @@ -160,10 +161,10 @@
>  	struct mem_cgroup *mem_cgroup;
>  	int flags;
>  };
> -#define PAGE_CGROUP_FLAG_CACHE	   (0x1)	/* charged as cache */
> -#define PAGE_CGROUP_FLAG_ACTIVE    (0x2)	/* page is active in this cgroup */
> -#define PAGE_CGROUP_FLAG_FILE	   (0x4)	/* page is file system backed */
> -#define PAGE_CGROUP_FLAG_UNEVICTABLE (0x8)	/* page is unevictableable */
> +#define PAGE_CGROUP_FLAG_ACTIVE    (0x1)	/* page is active in this cgroup */
> +#define PAGE_CGROUP_FLAG_FILE	   (0x2)	/* page is file system backed */
> +#define PAGE_CGROUP_FLAG_UNEVICTABLE (0x4)	/* page is unevictableable */
> +#define PAGE_CGROUP_FLAG_SHMEM	   (0x8)       /* page is shmem/tmpfs */
> 
>  static int page_cgroup_nid(struct page_cgroup *pc)
>  {
> @@ -178,6 +179,7 @@
>  enum charge_type {
>  	MEM_CGROUP_CHARGE_TYPE_CACHE = 0,
>  	MEM_CGROUP_CHARGE_TYPE_MAPPED,
> +	MEM_CGROUP_CHARGE_TYPE_SHMEM,
>  	MEM_CGROUP_CHARGE_TYPE_FORCE,	/* used by force_empty */
>  };
> 
> @@ -191,8 +193,10 @@
>  	struct mem_cgroup_stat *stat = &mem->stat;
> 
>  	VM_BUG_ON(!irqs_disabled());
> -	if (flags & PAGE_CGROUP_FLAG_CACHE)
> +	if (flags & PAGE_CGROUP_FLAG_FILE)
>  		__mem_cgroup_stat_add_safe(stat, MEM_CGROUP_STAT_CACHE, val);
> +	else if (flags & PAGE_CGROUP_FLAG_SHMEM)
> +		__mem_cgroup_stat_add_safe(stat, MEM_CGROUP_STAT_SHMEM, val);
>  	else
>  		__mem_cgroup_stat_add_safe(stat, MEM_CGROUP_STAT_RSS, val);
> 
> @@ -573,12 +577,19 @@
>  	 * If a page is accounted as a page cache, insert to inactive list.
>  	 * If anon, insert to active list.
>  	 */
> -	if (ctype == MEM_CGROUP_CHARGE_TYPE_CACHE) {
> -		pc->flags = PAGE_CGROUP_FLAG_CACHE;
> -		if (page_is_file_cache(page))
> -			pc->flags |= PAGE_CGROUP_FLAG_FILE;
> -	} else
> +	switch (ctype) {
> +	case MEM_CGROUP_CHARGE_TYPE_CACHE:
> +		pc->flags = PAGE_CGROUP_FLAG_FILE;
> +		break;
> +	case MEM_CGROUP_CHARGE_TYPE_SHMEM:
> +		pc->flags = PAGE_CGROUP_FLAG_SHMEM | PAGE_CGROUP_FLAG_ACTIVE;
> +		break;
> +	case MEM_CGROUP_CHARGE_TYPE_MAPPED:
>  		pc->flags = PAGE_CGROUP_FLAG_ACTIVE;
> +		break;
> +	default:
> +		BUG();
> +	}
> 
>  	lock_page_cgroup(page);
>  	if (unlikely(page_get_page_cgroup(page))) {
> @@ -625,28 +636,10 @@
>  int mem_cgroup_cache_charge(struct page *page, struct mm_struct *mm,
>  				gfp_t gfp_mask)
>  {
> -	/*
> -	 * Corner case handling. This is called from add_to_page_cache()
> -	 * in usual. But some FS (shmem) precharges this page before calling it
> -	 * and call add_to_page_cache() with GFP_NOWAIT.
> -	 *
> -	 * For GFP_NOWAIT case, the page may be pre-charged before calling
> -	 * add_to_page_cache(). (See shmem.c) check it here and avoid to call
> -	 * charge twice. (It works but has to pay a bit larger cost.)
> -	 */
> -	if (!(gfp_mask & __GFP_WAIT)) {
> -		struct page_cgroup *pc;
> +	enum charge_type ctype = MEM_CGROUP_CHARGE_TYPE_CACHE;
> 
> -		lock_page_cgroup(page);
> -		pc = page_get_page_cgroup(page);
> -		if (pc) {
> -			VM_BUG_ON(pc->page != page);
> -			VM_BUG_ON(!pc->mem_cgroup);
> -			unlock_page_cgroup(page);
> -			return 0;
> -		}
> -		unlock_page_cgroup(page);
> -	}
> +	if (PageSwapBacked(page))
> +		ctype = MEM_CGROUP_CHARGE_TYPE_SHMEM;
> 
>  	if (unlikely(!mm))
>  		mm = &init_mm;
> @@ -678,11 +671,19 @@
>  		goto unlock;
> 
>  	VM_BUG_ON(pc->page != page);
> -
> +	/*
> +	 * There are 2 cases.
> +	 * 1. anon pages are swapped out.
> +	 * 2. shmem pages are swapped out.
> +	 * In both case, PageSwapCache() returns 1 and we don't want to
> +	 * uncharge it.
> +	 */
> +	if (PageSwapCache(page))
> +		goto unlock;
> +	/* When the page is unmapped, file-cache and shmem memory is alive */
>  	if ((ctype == MEM_CGROUP_CHARGE_TYPE_MAPPED)
> -	    && ((pc->flags & PAGE_CGROUP_FLAG_CACHE)
> -		|| page_mapped(page)
> -		|| PageSwapCache(page)))
> +	    && ((pc->flags & (PAGE_CGROUP_FLAG_FILE | PAGE_CGROUP_FLAG_SHMEM))
> +		|| page_mapped(page)))
>  		goto unlock;
> 
>  	mz = page_cgroup_zoneinfo(pc);
> @@ -732,8 +733,10 @@
>  	if (pc) {
>  		mem = pc->mem_cgroup;
>  		css_get(&mem->css);
> -		if (pc->flags & PAGE_CGROUP_FLAG_CACHE)
> +		if (pc->flags & PAGE_CGROUP_FLAG_FILE)
>  			ctype = MEM_CGROUP_CHARGE_TYPE_CACHE;
> +		else if (pc->flags & PAGE_CGROUP_FLAG_SHMEM)
> +			ctype = MEM_CGROUP_CHARGE_TYPE_SHMEM;
>  	}
>  	unlock_page_cgroup(page);
>  	if (mem) {
> @@ -785,6 +788,8 @@
>  		progress = try_to_free_mem_cgroup_pages(mem, gfp_mask);
>  	} while (!progress && --retry);
> 
> +	css_put(&mem->css);
> +

Didn't Hugh fix this as a part of his patchset.

>  	if (!retry)
>  		return -ENOMEM;
>  	return 0;
> @@ -920,6 +925,7 @@
>  } mem_cgroup_stat_desc[] = {
>  	[MEM_CGROUP_STAT_CACHE] = { "cache", PAGE_SIZE, },
>  	[MEM_CGROUP_STAT_RSS] = { "anon/swapcache", PAGE_SIZE, },
> +	[MEM_CGROUP_STAT_SHMEM] = { "shmem/tmpfs", PAGE_SIZE, },
>  	[MEM_CGROUP_STAT_PGPGIN_COUNT] = {"pgpgin", 1, },
>  	[MEM_CGROUP_STAT_PGPGOUT_COUNT] = {"pgpgout", 1, },
>  };
> Index: linux-2.6.26-rc5-mm3-kosaki/include/linux/pagemap.h
> ===================================================================
> --- linux-2.6.26-rc5-mm3-kosaki.orig/include/linux/pagemap.h	2008-06-30 15:00:04.000000000 +0900
> +++ linux-2.6.26-rc5-mm3-kosaki/include/linux/pagemap.h	2008-06-30 15:03:02.000000000 +0900
> @@ -258,6 +258,22 @@
>  				pgoff_t index, gfp_t gfp_mask);
>  int add_to_page_cache_lru(struct page *page, struct address_space *mapping,
>  				pgoff_t index, gfp_t gfp_mask);
> +
> +#ifdef CONFIG_CGROUP_MEM_RES_CTLR
> +/*
> + * If the page is pre-charged before add_to_page_cache() this routine is
> + * light-weight.
> + */
> +int add_to_page_cache_nocharge(struct page *page, struct address_space *mapping,
> +				pgoff_t index, gfp_t gfp_mask);
> +#else
> +static inline int add_to_page_cache_nocharge(struct page *page,
> +				struct address_space *mapping, pgoff_t index,
> +				gfp_t mask)
> +{
> +	return add_to_page_cache(page, mapping, index, gfp_mask);
> +}
> +#endif
>  extern void remove_from_page_cache(struct page *page);
>  extern void __remove_from_page_cache(struct page *page);
> 
> Index: linux-2.6.26-rc5-mm3-kosaki/mm/filemap.c
> ===================================================================
> --- linux-2.6.26-rc5-mm3-kosaki.orig/mm/filemap.c	2008-06-30 15:00:04.000000000 +0900
> +++ linux-2.6.26-rc5-mm3-kosaki/mm/filemap.c	2008-06-30 15:23:36.000000000 +0900
> @@ -442,8 +442,9 @@
>  	return err;
>  }
> 
> +
>  /**
> - * add_to_page_cache - add newly allocated pagecache pages
> + * add_to_page_cache_nocharge() - add newly allocated pagecache pages
>   * @page:	page to add
>   * @mapping:	the page's address_space
>   * @offset:	page index
> @@ -454,22 +455,20 @@
>   * The other page state flags were set by rmqueue().
>   *
>   * This function does not add the page to the LRU.  The caller must do that.
> + * This doesn't call memory resource control routine.
>   */
> -int add_to_page_cache(struct page *page, struct address_space *mapping,
> -		pgoff_t offset, gfp_t gfp_mask)
> +
> +int add_to_page_cache_nocharge(struct page *page,
> +			       struct address_space *mapping,
> +			       pgoff_t offset, gfp_t gfp_mask)
>  {

I wonder if we should modify a generic routine and add charge/no charge variants
to it. Charging is very memory controller specific operation. Can't we infer the
charging/no charging from gfp_mask or another flag?

> -	int error = mem_cgroup_cache_charge(page, current->mm,
> -					gfp_mask & ~__GFP_HIGHMEM);
> -	if (error)
> -		goto out;
> 
> -	error = radix_tree_preload(gfp_mask & ~__GFP_HIGHMEM);
> +	int error = radix_tree_preload(gfp_mask & ~__GFP_HIGHMEM);
>  	if (error == 0) {
>  		page_cache_get(page);
>  		SetPageLocked(page);
>  		page->mapping = mapping;
>  		page->index = offset;
> -
>  		spin_lock_irq(&mapping->tree_lock);
>  		error = radix_tree_insert(&mapping->page_tree, offset, page);
>  		if (likely(!error)) {
> @@ -478,15 +477,39 @@
>  		} else {
>  			page->mapping = NULL;
>  			ClearPageLocked(page);
> -			mem_cgroup_uncharge_cache_page(page);
>  			page_cache_release(page);
>  		}
> 
>  		spin_unlock_irq(&mapping->tree_lock);
>  		radix_tree_preload_end();
> -	} else
> -		mem_cgroup_uncharge_cache_page(page);
> -out:
> +	}
> +	return error;
> +}
> +
> +/**
> + * add_to_page_cache - add newly allocated pagecache pages
> + * @page:	page to add
> + * @mapping:	the page's address_space
> + * @offset:	page index
> + * @gfp_mask:	page allocation mode
> + *
> + * This function is used to add newly allocated pagecache pages;
> + * the page is new, so we can just run SetPageLocked() against it.
> + * The other page state flags were set by rmqueue().
> + *
> + * This function does not add the page to the LRU.  The caller must do that.
> + */
> +int add_to_page_cache(struct page *page, struct address_space *mapping,
> +		pgoff_t offset, gfp_t gfp_mask)
> +{
> +	int error = mem_cgroup_cache_charge(page, current->mm,
> +					gfp_mask & ~__GFP_HIGHMEM);
> +	if (!error) {
> +		error = add_to_page_cache_nocharge(page, mapping, offset,
> +						  gfp_mask);
> +		if (error)
> +			mem_cgroup_uncharge_cache_page(page);
> +	}
>  	return error;
>  }
>  EXPORT_SYMBOL(add_to_page_cache);
> Index: linux-2.6.26-rc5-mm3-kosaki/mm/shmem.c
> ===================================================================
> --- linux-2.6.26-rc5-mm3-kosaki.orig/mm/shmem.c	2008-06-30 15:00:04.000000000 +0900
> +++ linux-2.6.26-rc5-mm3-kosaki/mm/shmem.c	2008-06-30 15:37:11.000000000 +0900
> @@ -940,10 +940,8 @@
>  	spin_lock(&info->lock);
>  	ptr = shmem_swp_entry(info, idx, NULL);
>  	if (ptr && ptr->val == entry.val)
> -		error = add_to_page_cache(page, inode->i_mapping, idx,
> +		error = add_to_page_cache_nocharge(page, inode->i_mapping, idx,
>  					GFP_NOWAIT);
> -	else /* we don't have to account this page. */
> -		mem_cgroup_uncharge_cache_page(page);
> 
>  	if (error == -EEXIST) {
>  		struct page *filepage = find_get_page(inode->i_mapping, idx);
> @@ -955,6 +953,7 @@
>  			 */
>  			if (PageUptodate(filepage))
>  				error = 0;
> +			mem_cgroup_uncharge_cache_page(page);
>  			page_cache_release(filepage);
>  		}
>  	}
> @@ -965,7 +964,9 @@
>  		shmem_swp_set(info, ptr, 0);
>  		swap_free(entry);
>  		error = 1;	/* not an error, but entry was found */
> -	}
> +	} else
> +		mem_cgroup_uncharge_cache_page(page);
> +
>  	if (ptr)
>  		shmem_swp_unmap(ptr);
>  	spin_unlock(&info->lock);
> @@ -1375,6 +1376,7 @@
>  				error = -ENOMEM;
>  				goto failed;
>  			}
> +			SetPageSwapBacked(filepage);
> 
>  			/* Precharge page while we can wait, compensate after */
>  			error = mem_cgroup_cache_charge(filepage, current->mm,
> @@ -1387,7 +1389,6 @@
>  				goto failed;
>  			}
> 
> -			SetPageSwapBacked(filepage);

I thought I saw a patch from Hugh for this one too.. but I might be missing the
chronological ordering of which came first.

>  			spin_lock(&info->lock);
>  			entry = shmem_swp_alloc(info, idx, sgp);
>  			if (IS_ERR(entry))
> @@ -1400,7 +1401,8 @@
>  			if (ret)
>  				mem_cgroup_uncharge_cache_page(filepage);
>  			else
> -				ret = add_to_page_cache_lru(filepage, mapping,
> +				ret = add_to_page_cache_nocharge(filepage,
> +						mapping,
>  						idx, GFP_NOWAIT);
>  			/*
>  			 * At add_to_page_cache_lru() failure, uncharge will
> @@ -1408,6 +1410,7 @@
>  			 */
>  			if (ret) {
>  				spin_unlock(&info->lock);
> +				mem_cgroup_uncharge_cache_page(filepage);
>  				page_cache_release(filepage);
>  				shmem_unacct_blocks(info->flags, 1);
>  				shmem_free_blocks(inode, 1);
> @@ -1415,6 +1418,8 @@
>  				if (error)
>  					goto failed;
>  				goto repeat;
> +			} else {
> +				lru_cache_add_active_anon(filepage);
>  			}
>  			info->flags |= SHMEM_PAGEIN;
>  		}
> Index: linux-2.6.26-rc5-mm3-kosaki/mm/hugetlb.c
> ===================================================================
> --- linux-2.6.26-rc5-mm3-kosaki.orig/mm/hugetlb.c	2008-06-25 18:07:09.000000000 +0900
> +++ linux-2.6.26-rc5-mm3-kosaki/mm/hugetlb.c	2008-06-30 15:08:16.000000000 +0900
> @@ -1813,7 +1813,8 @@
>  			int err;
>  			struct inode *inode = mapping->host;
> 
> -			err = add_to_page_cache(page, mapping, idx, GFP_KERNEL);
> +			err = add_to_page_cache_nocharge(page, mapping,
> +						idx, GFP_KERNEL);
>  			if (err) {
>  				put_page(page);
>  				if (err == -EEXIST)
> 


-- 
	Warm Regards,
	Balbir Singh
	Linux Technology Center
	IBM, ISTL

Re: [RFC][PATCH] memcg: change shmem handler.

[KAMEZAWA Hiroyuki]

On Thu, 03 Jul 2008 12:45:14 +0530
Balbir Singh <balbir@linux.vnet.ibm.com> wrote:

> > This patch fixes some mess by
> >   - PAGE_CGROUP_FLAG_CACHE is deleted (and replaced by FLAG_FILE)
> >   - PAGE_CGROUP_FLAG_SHMEM is added.
> >   - add_to_page_cache_nocharge() is added.
> >     This avoids mem_cgroup_charge_cache_page(). This is useful when page is
> >     pre-charged.
> >   - uses add_to_page_cache_nocharge() also in hugemem.
> >     (I think hugemem controller should be independent from memcg.
> >      Balbir, how do you think ?)
> 
> I am not 100% sure of that right now. I definitely want different control
> parameters (not included as a part of memory.limit_in_bytes). If there is
> leverage from memory controller, we could consider adding that to it or a
> separate controller if that makes more sense.
> 
Checking PG_compound in mem_cgroup_charge_cache_page() is enough, maybe.
(If multi-page-file-cache is added, need rework ;)

> >   - PageSwapBacked() is checked.
> >     (A imported patch from Hugh Dickins)
> > 
> > As result.
> >   - shmem will be in SwapBacked/Active list at first.
> 
> Good
> 
> >   - memcg has "shmem/tmpfs" counter.
> > 
> 
> I think this will be useful too (but I need to play with it)
> 
I've been argued that "if you want to add such a stupid counter, add the
same parameter to global lru without any overhead."

I'll postpone this ;) I have many other thing to do.

But the total number of pages used for shmem is already counted by
address_space->nr_pages. So, not very difficult for global lru if we can
walk all inodes of shmem. The problem will be "whether it's worth to do or not"

Thanks,
-Kame

[PATCH] splitlru: BDI_CAP_SWAP_BACKED

[Hugh Dickins]

The split-lru patches put file and swap-backed pages on different lrus.
shmem/tmpfs pages are awkward because they are swap-backed file pages.
Since it's difficult to change lru midstream, they are treated as swap-
backed throughout, with SetPageSwapBacked on allocation in shmem_getpage.

However, splice read (used by loop and sendfile) and readahead* allocate
pages first, add_to_page_cache_lru, and then call into the filesystem
through ->readpage.  Under memory pressure, the shmem pages arrive at
add_to_swap_cache and hit its BUG_ON(!PageSwapBacked(page)).

I've not yet found a better way to handle this than a "capability"
flag in shmem_backing_dev_info, tested by add_to_page_cache_lru.
And solely because it would look suspicious without it, set that
BDI_CAP_SWAP_BACKED in swap_backing_dev_info also.

* readahead on shmem/tmpfs?  I'd always thought ra_pages 0 prevented
that; but in fact readahead(2), fadvise(POSIX_FADV_WILLNEED) and
madvise(MADV_WILLNEED) all force_page_cache_readahead and get there.

Signed-off-by: Hugh Dickins <hugh@veritas.com>
---
Should follow mmotm's vmscan-split-lru-lists-into-anon-file-sets.patch

 include/linux/backing-dev.h |   13 +++++++++++++
 mm/filemap.c                |   13 ++++++++++++-
 mm/shmem.c                  |    2 +-
 mm/swap_state.c             |    2 +-
 4 files changed, 27 insertions(+), 3 deletions(-)

--- mmotm/include/linux/backing-dev.h	2008-05-03 21:55:10.000000000 +0100
+++ linux/include/linux/backing-dev.h	2008-06-27 17:02:45.000000000 +0100
@@ -175,6 +175,8 @@ int bdi_set_max_ratio(struct backing_dev
  * BDI_CAP_READ_MAP:       Can be mapped for reading
  * BDI_CAP_WRITE_MAP:      Can be mapped for writing
  * BDI_CAP_EXEC_MAP:       Can be mapped for execution
+ *
+ * BDI_CAP_SWAP_BACKED:    Count shmem/tmpfs objects as swap-backed.
  */
 #define BDI_CAP_NO_ACCT_DIRTY	0x00000001
 #define BDI_CAP_NO_WRITEBACK	0x00000002
@@ -184,6 +186,7 @@ int bdi_set_max_ratio(struct backing_dev
 #define BDI_CAP_WRITE_MAP	0x00000020
 #define BDI_CAP_EXEC_MAP	0x00000040
 #define BDI_CAP_NO_ACCT_WB	0x00000080
+#define BDI_CAP_SWAP_BACKED	0x00000100
 
 #define BDI_CAP_VMFLAGS \
 	(BDI_CAP_READ_MAP | BDI_CAP_WRITE_MAP | BDI_CAP_EXEC_MAP)
@@ -248,6 +251,11 @@ static inline bool bdi_cap_account_write
 				      BDI_CAP_NO_WRITEBACK));
 }
 
+static inline bool bdi_cap_swap_backed(struct backing_dev_info *bdi)
+{
+	return bdi->capabilities & BDI_CAP_SWAP_BACKED;
+}
+
 static inline bool mapping_cap_writeback_dirty(struct address_space *mapping)
 {
 	return bdi_cap_writeback_dirty(mapping->backing_dev_info);
@@ -258,4 +266,9 @@ static inline bool mapping_cap_account_d
 	return bdi_cap_account_dirty(mapping->backing_dev_info);
 }
 
+static inline bool mapping_cap_swap_backed(struct address_space *mapping)
+{
+	return bdi_cap_swap_backed(mapping->backing_dev_info);
+}
+
 #endif		/* _LINUX_BACKING_DEV_H */
--- mmotm/mm/filemap.c	2008-06-27 13:39:20.000000000 +0100
+++ linux/mm/filemap.c	2008-06-27 18:16:07.000000000 +0100
@@ -493,7 +493,18 @@ EXPORT_SYMBOL(add_to_page_cache);
 int add_to_page_cache_lru(struct page *page, struct address_space *mapping,
 				pgoff_t offset, gfp_t gfp_mask)
 {
-	int ret = add_to_page_cache(page, mapping, offset, gfp_mask);
+	int ret;
+
+	/*
+	 * Splice_read and readahead add shmem/tmpfs pages into the page cache
+	 * before shmem_readpage has a chance to mark them as SwapBacked: they
+	 * need to go on the active_anon lru below, and mem_cgroup_cache_charge
+	 * (called in add_to_page_cache) needs to know where they're going too.
+	 */
+	if (mapping_cap_swap_backed(mapping))
+		SetPageSwapBacked(page);
+
+	ret = add_to_page_cache(page, mapping, offset, gfp_mask);
 	if (ret == 0) {
 		if (page_is_file_cache(page))
 			lru_cache_add_file(page);
--- mmotm/mm/shmem.c	2008-06-27 13:39:20.000000000 +0100
+++ linux/mm/shmem.c	2008-06-27 17:25:41.000000000 +0100
@@ -201,7 +201,7 @@ static struct vm_operations_struct shmem
 
 static struct backing_dev_info shmem_backing_dev_info  __read_mostly = {
 	.ra_pages	= 0,	/* No readahead */
-	.capabilities	= BDI_CAP_NO_ACCT_AND_WRITEBACK,
+	.capabilities	= BDI_CAP_NO_ACCT_AND_WRITEBACK | BDI_CAP_SWAP_BACKED,
 	.unplug_io_fn	= default_unplug_io_fn,
 };
 
--- mmotm/mm/swap_state.c	2008-06-27 13:39:20.000000000 +0100
+++ linux/mm/swap_state.c	2008-06-27 17:26:49.000000000 +0100
@@ -33,7 +33,7 @@ static const struct address_space_operat
 };
 
 static struct backing_dev_info swap_backing_dev_info = {
-	.capabilities	= BDI_CAP_NO_ACCT_AND_WRITEBACK,
+	.capabilities	= BDI_CAP_NO_ACCT_AND_WRITEBACK | BDI_CAP_SWAP_BACKED,
 	.unplug_io_fn	= swap_unplug_io_fn,
 };
 

Re: [PATCH] splitlru: BDI_CAP_SWAP_BACKED

[KOSAKI Motohiro]

> The split-lru patches put file and swap-backed pages on different lrus.
> shmem/tmpfs pages are awkward because they are swap-backed file pages.
> Since it's difficult to change lru midstream, they are treated as swap-
> backed throughout, with SetPageSwapBacked on allocation in shmem_getpage.
> 
> However, splice read (used by loop and sendfile) and readahead* allocate
> pages first, add_to_page_cache_lru, and then call into the filesystem
> through ->readpage.  Under memory pressure, the shmem pages arrive at
> add_to_swap_cache and hit its BUG_ON(!PageSwapBacked(page)).
> 
> I've not yet found a better way to handle this than a "capability"
> flag in shmem_backing_dev_info, tested by add_to_page_cache_lru.
> And solely because it would look suspicious without it, set that
> BDI_CAP_SWAP_BACKED in swap_backing_dev_info also.
> 
> * readahead on shmem/tmpfs?  I'd always thought ra_pages 0 prevented
> that; but in fact readahead(2), fadvise(POSIX_FADV_WILLNEED) and
> madvise(MADV_WILLNEED) all force_page_cache_readahead and get there.
> 
> Signed-off-by: Hugh Dickins <hugh@veritas.com>

great.

I'm not sure about this patch is perfect.
but it seems makes sense and very good code.

So, I'll testing this patch for a while.

Rik, What do you think about it?

Re: [PATCH] splitlru: BDI_CAP_SWAP_BACKED

[Rik van Riel]

On Sun, 29 Jun 2008 17:48:59 +0900
KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com> wrote:

> I'm not sure about this patch is perfect.
> but it seems makes sense and very good code.
> 
> So, I'll testing this patch for a while.
> 
> Rik, What do you think about it?

I have been running with a similar patch for a while, and
got my system to OOM :)

However, the patch is correct and the OOM is caused by
a different issue.  When stress-testing with a loopback
device on a tmpfs file, the VM thinks that we succeed
in evicting pages from the loopback device (file LRUs)
and continues scanning those LRUs.

Unfortunately for the VM, the loopback writepage function,
in combination with splice, end up moving the pages to the
anon LRUs, instead of freeing any pages.  

This confuses the balancing code, which thinks the pages
got freed.  As a result, the VM keeps scanning the file
LRUs and will eventually realize it failed to free any
memory.

One of Kosaki-san's recent patches may fix this, though.

-- 
All rights reversed.

Re: [PATCH] splitlru: BDI_CAP_SWAP_BACKED

[KAMEZAWA Hiroyuki]

On Sun, 29 Jun 2008 17:48:59 +0900
KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com> wrote:

> > The split-lru patches put file and swap-backed pages on different lrus.
> > shmem/tmpfs pages are awkward because they are swap-backed file pages.
> > Since it's difficult to change lru midstream, they are treated as swap-
> > backed throughout, with SetPageSwapBacked on allocation in shmem_getpage.
> > 
> > However, splice read (used by loop and sendfile) and readahead* allocate
> > pages first, add_to_page_cache_lru, and then call into the filesystem
> > through ->readpage.  Under memory pressure, the shmem pages arrive at
> > add_to_swap_cache and hit its BUG_ON(!PageSwapBacked(page)).
> > 
> > I've not yet found a better way to handle this than a "capability"
> > flag in shmem_backing_dev_info, tested by add_to_page_cache_lru.
> > And solely because it would look suspicious without it, set that
> > BDI_CAP_SWAP_BACKED in swap_backing_dev_info also.
> > 
> > * readahead on shmem/tmpfs?  I'd always thought ra_pages 0 prevented
> > that; but in fact readahead(2), fadvise(POSIX_FADV_WILLNEED) and
> > madvise(MADV_WILLNEED) all force_page_cache_readahead and get there.
> > 
> > Signed-off-by: Hugh Dickins <hugh@veritas.com>
> 
> great.
> 
> I'm not sure about this patch is perfect.
> but it seems makes sense and very good code.
> 
> So, I'll testing this patch for a while.
> 

Hmm, how about adding LRU only for shmem/tmpfs ? nonsense ? ;)
It's just a file if not mapped but it's swap backed.


Thanks,
-Kame

Re: [PATCH] splitlru: BDI_CAP_SWAP_BACKED

[Hugh Dickins]

On Mon, 30 Jun 2008, KAMEZAWA Hiroyuki wrote:
> 
> Hmm, how about adding LRU only for shmem/tmpfs ? nonsense ? ;)

I'd much prefer not: the more LRUs, the more balancing problems;
And I don't think shmem/tmpfs _deserves_ its own LRUs.
But it cannot be ruled out.

Hugh

Re: [PATCH] splitlru: BDI_CAP_SWAP_BACKED

[Rik van Riel]

On Mon, 30 Jun 2008 20:23:54 +0100 (BST)
Hugh Dickins <hugh@veritas.com> wrote:

> On Mon, 30 Jun 2008, KAMEZAWA Hiroyuki wrote:
> > 
> > Hmm, how about adding LRU only for shmem/tmpfs ? nonsense ? ;)
> 
> I'd much prefer not: the more LRUs, the more balancing problems;
> And I don't think shmem/tmpfs _deserves_ its own LRUs.
> But it cannot be ruled out.

Tmpfs is often in the same boat as anonymous memory.
Used for shared memory segments, or for files that
are temporary and will be gone soon.

If swap space runs out, tmpfs pages should not be
scanned.

To me, this suggests they should probably continue
to live on the *_ANON LRUs.  Worst case we make
tmpfs pages in files that are not mmaped (/tmp use)
start out on the inactive list, so they get evicted
first.

-- 
All rights reversed.

Re: [PATCH] splitlru: BDI_CAP_SWAP_BACKED

[Hugh Dickins]

On Mon, 30 Jun 2008, Rik van Riel wrote:
> 
> Tmpfs is often in the same boat as anonymous memory.
> Used for shared memory segments, or for files that
> are temporary and will be gone soon.

Anonymous memory, and temporary files, are often soon gone,
okay.  But I don't find that generalization compelling; and
if they're soon gone, does it matter which lru they go on?

> If swap space runs out, tmpfs pages should not be
> scanned.

That point I like.  But I hope they'd go to the Unevictable
on systems with no swap at all (of course, as with mlocking,
that can change soon after).

> To me, this suggests they should probably continue
> to live on the *_ANON LRUs.  Worst case we make
> tmpfs pages in files that are not mmaped (/tmp use)
> start out on the inactive list, so they get evicted
> first.

Tweaking in/active I'll gladly leave to you!  Whatever
proves best.  What's worrying me is that we have always treated
shmem/tmpfs pages as file pages (e.g. in /proc/meminfo as Cached
not as SwapCached), up until the point that we retire them to
swap; but in splitlru you're sending them down another path;
then mem cgroups seem to want them as something else again.

Your SwapBacked may indeed turn out to be the only implementable
distinction, but it does worry me.  A more useful distinction,
my gut tells me, would be separate LRUs for page_mapped() and
!page_mapped(), which reflects the existing swappiness notion.

But that immediately hits the difficulty we have in switching LRU
midstream, which your SwapBacked-throughout tmpfs neatly sidesteps.

I'd really like to be able to try page_mapped/!page_mapped versus
swap-backed/file-backed, but it would need some LRU-switching
infrastructure (which might come at a prohibitive performance
cost, since it's the batching that poses the problem).

Hugh

Re: [PATCH] splitlru: BDI_CAP_SWAP_BACKED

[Rik van Riel]

On Mon, 30 Jun 2008 21:55:13 +0100 (BST)
Hugh Dickins <hugh@veritas.com> wrote:
> On Mon, 30 Jun 2008, Rik van Riel wrote:
> > 
> > Tmpfs is often in the same boat as anonymous memory.
> > Used for shared memory segments, or for files that
> > are temporary and will be gone soon.
> 
> Anonymous memory, and temporary files, are often soon gone,
> okay.  But I don't find that generalization compelling; and
> if they're soon gone, does it matter which lru they go on?

Temporary files are often soon gone.

Anonymous memory and shmem segments tend to stick around
for longer.
 
> > If swap space runs out, tmpfs pages should not be
> > scanned.
> 
> That point I like.  But I hope they'd go to the Unevictable
> on systems with no swap at all (of course, as with mlocking,
> that can change soon after).

If we have them on the *_ANON LRUs, we will automatically
not scan them when swap space runs out.  Just like we do
not scan anonymous pages when there is no swap space left.

> > To me, this suggests they should probably continue
> > to live on the *_ANON LRUs.  Worst case we make
> > tmpfs pages in files that are not mmaped (/tmp use)
> > start out on the inactive list, so they get evicted
> > first.
> 
> Tweaking in/active I'll gladly leave to you!  Whatever
> proves best.  What's worrying me is that we have always treated
> shmem/tmpfs pages as file pages

This is a performance problem for database systems,
where the system ends up swapping out the shared
memory segment.

> (e.g. in /proc/meminfo as Cached
> not as SwapCached), up until the point that we retire them to
> swap; but in splitlru you're sending them down another path;
> then mem cgroups seem to want them as something else again.

Having the mem cgroups consistent with the global LRU
implementation would be good, indeed.  That will make
balancing a bit easier.
 
> Your SwapBacked may indeed turn out to be the only implementable
> distinction, but it does worry me.  A more useful distinction,
> my gut tells me, would be separate LRUs for page_mapped() and
> !page_mapped(), which reflects the existing swappiness notion.
> 
> But that immediately hits the difficulty we have in switching LRU
> midstream, which your SwapBacked-throughout tmpfs neatly sidesteps.
>
> I'd really like to be able to try page_mapped/!page_mapped versus
> swap-backed/file-backed, but it would need some LRU-switching
> infrastructure (which might come at a prohibitive performance
> cost, since it's the batching that poses the problem).

Agreed, we understand the page_mapped/!page_mapped distinction
quite well.  On the other hand, we do not understand LRU
acrobatics (moving pages between lists on mmap/munmap) and the
consequences of that...

I suspect we'll just have to tweak the swap-backed/file-backed
code until it works right.  Once Andrew comes out with a new
-mm (with all the stability fixes), I will create a kernel RPM
with the latest split LRU code for Fedora 9, so we can get some
wider testing.

I have some performance tweaks in mind already, but not enough
data yet to justify them.  I will continue working on that.

-- 
All rights reversed.

Re: [PATCH] splitlru: shmem_getpage SetPageSwapBacked sooner

[KOSAKI Motohiro]

> shmem_getpage must SetPageSwapBacked before the mem_cgroup_cache_charge,
> otherwise try_to_free_mem_cgroup_pages cannot find the shmem/tmpfs pages
> it needs to free, and OOMs result.
> 
> Signed-off-by: Hugh Dickins <hugh@veritas.com>

it seems makes sense.

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