ksm: add vm_stat and meminfo entry to reflect pte mapping to ksm pages

(quoted diff) +4 -0

@@ -904,6 +905,10 @@ static int try_to_merge_one_page(struct vm_area_struct 
 *vma,
                         */
                        set_page_stable_node(page, NULL);
                        mark_page_accessed(page);
+                       if (mapcount)
+                               add_zone_page_state(page_zone(page),
+                                                   NR_KSM_PAGES_SHARING,
+                                                   mapcount);
                        err = 0;
                } else if (pages_identical(page, kpage))

                        err = replace_page(vma, page, kpage, orig_pte); 

On Tuesday 01 March 2011, at 10:21:48, <Dave Hansen <[email protected]>> wrote On Sat, 2011-02-26 at 22:56 +0800, Nai Xia wrote

Quoted 11 lines

(quoted diff) +4 -0

@@ -904,6 +905,10 @@ static int try_to_merge_one_page(struct vm_area_struct 
 *vma,
                         */
                        set_page_stable_node(page, NULL);
                        mark_page_accessed(page);
+                       if (mapcount)
+                               add_zone_page_state(page_zone(page),
+                                                   NR_KSM_PAGES_SHARING,
+                                                   mapcount);
                        err = 0;
                } else if (pages_identical(page, kpage))

Quoted 1 line

                        err = replace_page(vma, page, kpage, orig_pte); 

If you're going to store this per-zone, does it make sense to have it show up in /proc/zoneinfo? meminfo's also getting pretty porky these days, so I almost wonder if it should stay in zoneinfo only.

-- Dave

ksm_pages_sharing is updated by ksmd periodically. In some cases, it cannot reflect the actual savings and makes the benchmarks on volatile VMAs very inaccurate.

This patch add a vm_stat entry and let the /proc/meminfo show information about how much virutal address pte is being mapped to ksm pages. With default ksm paramters (pages_to_scan=100 && sleep_millisecs=20), this can result in 50% more accurate averaged savings result for the following test program. Bigger sleep_millisecs values will increase this deviation.

--- test.c-----

On Wednesday 02 March 2011, at 07:41:00, <Andrew Morton <[email protected]>> wrote

Quoted 9 lines · On Sat, 26 Feb 2011 22:56:31 +0800 Nai Xia <[email protected]> wrote:

ksm_pages_sharing is updated by ksmd periodically. In some cases, it cannot reflect the actual savings and makes the benchmarks on volatile VMAs very inaccurate.

This patch add a vm_stat entry and let the /proc/meminfo show information about how much virutal address pte is being mapped to ksm pages. With default ksm paramters (pages_to_scan=100 && sleep_millisecs=20), this can result in 50% more accurate averaged savings result for the following test program. Bigger sleep_millisecs values will increase this deviation.

So I think you're saying that the existing ksm_pages_sharing sysfs file is no good.

You added a new entry to /proc/meminfo and left ksm_pages_sharing as-is. Why not leave /proc/meminfo alone, and fix up the existing ksm_pages_sharing?

Also, the patch accumulates the NR_KSM_PAGES_SHARING counts on a per-zone basis as well as on a global basis, but only provides the global count to userspace. The per-zone counts are potentially interesting? If not, maintaining the per-zone counters is wasted overhead.

Quoted 4 lines

--- test.c-----

The "^---" token conventionally means "end of changelog". Please avoid inserting it into the middle of the changelog.

Review static int try_to_merge_one_page(struct vm_area_struct · line 897

Quoted 4 lines

+++ b/mm/ksm.c

(quoted diff) +0 -0

@@ -897,6 +897,7 @@ static int try_to_merge_one_page(struct vm_area_struct 
 *vma,

Your email client wordwraps the patches.

ksm_pages_sharing is updated by ksmd periodically. In some cases, it cannot reflect the actual savings and makes the benchmarks on volatile VMAs very inaccurate.

This patch add a vm_stat entry and let the /proc/meminfo show information about how much virutal address pte is being mapped to ksm pages. With default ksm paramters (pages_to_scan=100 && sleep_millisecs=20), this can result in 50% more accurate averaged savings result for the following test program. Bigger sleep_millisecs values will increase this deviation.

...

fs/proc/meminfo.c +7 -0

@@ -87,6 +87,9 @@ static int meminfo_proc_show(struct seq_file *m, void *v)
 		"SUnreclaim:     %8lu kB\n"
 		"KernelStack:    %8lu kB\n"
 		"PageTables:     %8lu kB\n"
+#ifdef CONFIG_KSM
+		"KsmSharing:     %8lu kB\n"
+#endif
 #ifdef CONFIG_QUICKLIST
 		"Quicklists:     %8lu kB\n"
 #endif
 
...

@@ -904,6 +905,10 @@ static int try_to_merge_one_page(struct vm_area_struct 
 *vma,
 			 */
 			set_page_stable_node(page, NULL);
 			mark_page_accessed(page);
+			if (mapcount)
+				add_zone_page_state(page_zone(page),
+						    NR_KSM_PAGES_SHARING,
+						    mapcount);
 			err = 0;
 		} else if (pages_identical(page, kpage))

err = replace_page(vma, page, kpage, orig_pte);

On Thursday 03 March 2011, at 06:31:42, <Andrew Morton <[email protected]>> wrote This patch obviously wasn't tested with CONFIG_KSM=n, which was a
pretty basic patch-testing failure :(

I fixed up my tree with the below, but really the amount of ifdeffing is unacceptable - please find a cleaner way to fix up this patch.

mm/ksm.c renamed from mm/ksm.c~ksm-add-vm_stat-and-meminfo-entry-to-reflect-pte-mapping-to-ksm-pages-fix +3 -2

@@ -883,7 +883,6 @@ static int try_to_merge_one_page(struct 
 	 */
 	if (write_protect_page(vma, page, &orig_pte) = 0) {
 		if (!kpage) {
-			long mapcount = page_mapcount(page);
 			/*
 			 * While we hold page lock, upgrade page from
 			 * PageAnon+anon_vma to PageKsm+NULL stable_node:

@@ -891,10 +890,12 @@ static int try_to_merge_one_page(struct 
 			 */
 			set_page_stable_node(page, NULL);
 			mark_page_accessed(page);
-			if (mapcount)
+#ifdef CONFIG_KSM
+			if (page_mapcount(page))
 				add_zone_page_state(page_zone(page),
 						    NR_KSM_PAGES_SHARING,
 						    mapcount);
+#endif
 			err = 0;
 		} else if (pages_identical(page, kpage))
 			err = replace_page(vma, page, kpage, orig_pte);

mm/memory.c renamed from mm/memory.c~ksm-add-vm_stat-and-meminfo-entry-to-reflect-pte-mapping-to-ksm-pages-fix +2 -0

@@ -719,8 +719,10 @@ copy_one_pte(struct mm_struct *dst_mm, s
 			rss[MM_ANONPAGES]++;
 		else
 			rss[MM_FILEPAGES]++;
+#ifdef CONFIG_KSM
 		if (PageKsm(page)) /* follows page_dup_rmap() */
 			inc_zone_page_state(page, NR_KSM_PAGES_SHARING);
+#endif
 	}
 
 out_set_pte:

mm/rmap.c renamed from mm/rmap.c~ksm-add-vm_stat-and-meminfo-entry-to-reflect-pte-mapping-to-ksm-pages-fix +4 -2

@@ -893,11 +893,12 @@ void do_page_add_anon_rmap(struct page *
 			__inc_zone_page_state(page,
 					      NR_ANON_TRANSPARENT_HUGEPAGES);
 	}
+#ifdef CONFIG_KSM
 	if (unlikely(PageKsm(page))) {
 		__inc_zone_page_state(page, NR_KSM_PAGES_SHARING);
 		return;
 	}
-
+#endif
 	VM_BUG_ON(!PageLocked(page));
 	VM_BUG_ON(address < vma->vm_start || address >= vma->vm_end);
 	if (first)

@@ -955,9 +956,10 @@ void page_add_file_rmap(struct page *pag
  */
 void page_remove_rmap(struct page *page)
 {
+#ifdef CONFIG_KSM
 	if (PageKsm(page))
 		__dec_zone_page_state(page, NR_KSM_PAGES_SHARING);
-
+#endif
 	/* page still mapped by someone else? */
 	if (!atomic_add_negative(-1, &page->_mapcount))
 		return;

_

Quoted 3 lines

On Thursday 03 March 2011, at 06:31:42, <Andrew Morton <[email protected]>> wrote This patch obviously wasn't tested with CONFIG_KSM=n, which was a
pretty basic patch-testing failure :(

Oops, I will be careful to avoid similar mistakes next time.

Quoted 3 lines

I fixed up my tree with the below, but really the amount of ifdeffing is unacceptable - please find a cleaner way to fix up this patch.

Ok, I will have a try in my next patch submit.

Quoted 10 lines · Nai Xia

Quoted 3 lines

On Thursday 03 March 2011, at 06:31:42, <Andrew Morton <[email protected]>> wrote This patch obviously wasn't tested with CONFIG_KSM=n, which was a
pretty basic patch-testing failure :(

Oops, I will be careful to avoid similar mistakes next time.

Quoted 3 lines

I fixed up my tree with the below, but really the amount of ifdeffing is unacceptable - please find a cleaner way to fix up this patch.

Quoted 1 line

Ok, I will have a try in my next patch submit.

A couple of notes on that.

akpm's fixup introduced an #ifdef CONFIG_KSM in mm/ksm.c: that should be, er, unnecessary - since ksm.c is only compiled when CONFIG_KSM=y.

And PageKsm(page) evaluates to 0 when CONFIG_KSM is not set, so the optimizer should eliminate code from most places without #ifdef:
though you need to keep the #ifdef around display in /proc/meminfo itself, so as not to annoy non-KSM people with an always 0kB line.

But I am uncomfortable with the whole patch.

Can you make a stronger case for it?  KSM is designed to have its own cycle, and to keep out of the way of the rest of mm as much as possible (not as much as originally hoped, I admit).  Do we really want to show its statistics in /proc/meminfo now?  And do we really care that they don't keep up with exiting processes when the scan rate is low?

I am not asserting that we don't, nor am I nacking your patch:
but I would like to hear more support for it, before it adds yet another line to our user interface in /proc/meminfo.

And there is an awkward little bug in your patch, which amplifies a more significant and shameful pair of bugs of mine in KSM itself - no wonder that I'm anxious about your patch going in!

Your bug is precisely where akpm added the #ifdef in ksm.c.  The problem is that page_mapcount() is maintained atomically, generally without spinlock or pagelock: so the value of mapcount there, unless it is 1, can go up or down racily (as other processes sharing that anonymous page fork or unmap at the same time).

I could hardly complain about that, while suggesting above that more approximate numbers are good enough!  Except that, when KSM is turned off, there's a chance that you'd be left showing a non-0 kB in /proc/meminfo.  Then people will want a fix, and I don't yet know what that fix will be.

My first bug is in the break_cow() technique used to get back to normal, when merging into a KSM page fails for one reason or another:
that technique misses other mappings of the page.  I did have a patch in progress to fix that a few months ago, but it wasn't quite working, and then I realized the second bug: that even when successful, if VM_UNMERGEABLE has been used in forked processes, then we could end up with a KSM page in a VM_UNMERGEABLE area, which is against the spec.

A solution to all three problems would be to revert to allocating a separate KSM page, instead of using one of the pages already there. But that feels like a regression, and I don't think anybody is really hurting from the current situation, so I've not jumped to fix it yet.

Hugh