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

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

[Nai Xia]

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-----
/*
 * This test program triggers frequent faults on merged ksm pages but 
 * still keeps the faulted pages mergeable.
 */

#include <stdio.h>
#include <stdlib.h>
#include <errno.h>
#include <string.h>
#include <unistd.h>
#include <sys/mman.h>

#define MADV_MERGEABLE   12
#define MADV_UNMERGEABLE 13


#define SIZE (1000*1024*1024)
#define SEED	1
#define PAGE_SIZE 4096

int main(int argc, char **argv)
{
	char *p;
	int j;
	long feed = 1, new_feed, tmp;
	unsigned int offset;
	int status;
	int ret;

	pid_t child;

	child = fork();

	p = mmap(NULL, SIZE, PROT_WRITE|PROT_READ, 
		 MAP_PRIVATE|MAP_ANONYMOUS, 0, 0);
    	if (p == MAP_FAILED) {
    		printf("mmap error\n");
    		return 0;
    	}

	ret = madvise(p, SIZE, MADV_MERGEABLE);

	if (ret==-1) {
		printf("madvise failed \n");
		return 0;
	}

	memset(p, feed, SIZE);

	while (1) {
		for (j=0; j<SIZE; j+= PAGE_SIZE) {
			    p[j] *= p[j]*1;
		}
	}

	return 0;
}
----test.c ends-------

Some of the patch lines removes trailing spaces in related files.

Signed-off-by: Nai Xia <nai.xia@gmail.com>
---
diff --git a/fs/proc/meminfo.c b/fs/proc/meminfo.c
index ed257d1..dd0ff82 100644
--- a/fs/proc/meminfo.c
+++ b/fs/proc/meminfo.c
@@ -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
@@ -145,6 +148,9 @@ static int meminfo_proc_show(struct seq_file *m, void *v)
 		K(global_page_state(NR_SLAB_UNRECLAIMABLE)),
 		global_page_state(NR_KERNEL_STACK) * THREAD_SIZE / 1024,
 		K(global_page_state(NR_PAGETABLE)),
+#ifdef CONFIG_KSM
+		K(global_page_state(NR_KSM_PAGES_SHARING)),
+#endif
 #ifdef CONFIG_QUICKLIST
 		K(quicklist_total_size()),
 #endif
diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
index 02ecb01..01450e3 100644
--- a/include/linux/mmzone.h
+++ b/include/linux/mmzone.h
@@ -115,6 +115,9 @@ enum zone_stat_item {
 	NUMA_OTHER,		/* allocation from other node */
 #endif
 	NR_ANON_TRANSPARENT_HUGEPAGES,
+#ifdef CONFIG_KSM
+	NR_KSM_PAGES_SHARING,
+#endif
 	NR_VM_ZONE_STAT_ITEMS };
 
 /*
@@ -344,7 +347,7 @@ struct zone {
 	ZONE_PADDING(_pad1_)
 
 	/* Fields commonly accessed by the page reclaim scanner */
-	spinlock_t		lru_lock;	
+	spinlock_t		lru_lock;
 	struct zone_lru {
 		struct list_head list;
 	} lru[NR_LRU_LISTS];
@@ -722,7 +725,7 @@ static inline int is_normal_idx(enum zone_type idx)
 }
 
 /**
- * is_highmem - helper function to quickly check if a struct zone is a 
+ * is_highmem - helper function to quickly check if a struct zone is a
  *              highmem zone or not.  This is an attempt to keep references
  *              to ZONE_{DMA/NORMAL/HIGHMEM/etc} in general code to a 
minimum.
  * @zone - pointer to struct zone variable
diff --git a/mm/ksm.c b/mm/ksm.c
index c2b2a94..3c22d30 100644
--- a/mm/ksm.c
+++ b/mm/ksm.c
@@ -897,6 +897,7 @@ static int try_to_merge_one_page(struct vm_area_struct 
*vma,
 	 */
 	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:
@@ -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);
diff --git a/mm/memory.c b/mm/memory.c
index 8e8c183..d86abe9 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -719,6 +719,8 @@ copy_one_pte(struct mm_struct *dst_mm, struct mm_struct 
*src_mm,
 			rss[MM_ANONPAGES]++;
 		else
 			rss[MM_FILEPAGES]++;
+		if (PageKsm(page)) /* follows page_dup_rmap() */
+			inc_zone_page_state(page, NR_KSM_PAGES_SHARING);
 	}
 
 out_set_pte:
@@ -1423,7 +1425,7 @@ int __get_user_pages(struct task_struct *tsk, struct 
mm_struct *mm,
 
 	VM_BUG_ON(!!pages != !!(gup_flags & FOLL_GET));
 
-	/* 
+	/*
 	 * Require read or write permissions.
 	 * If FOLL_FORCE is set, we only require the "MAY" flags.
 	 */
diff --git a/mm/rmap.c b/mm/rmap.c
index f21f4a1..0d7ab31 100644
--- a/mm/rmap.c
+++ b/mm/rmap.c
@@ -801,9 +801,9 @@ void page_move_anon_rmap(struct page *page,
 
 /**
  * __page_set_anon_rmap - set up new anonymous rmap
- * @page:	Page to add to rmap	
+ * @page:	Page to add to rmap
  * @vma:	VM area to add page to.
- * @address:	User virtual address of the mapping	
+ * @address:	User virtual address of the mapping
  * @exclusive:	the page is exclusively owned by the current process
  */
 static void __page_set_anon_rmap(struct page *page,
@@ -889,8 +889,10 @@ void do_page_add_anon_rmap(struct page *page,
 			__inc_zone_page_state(page,
 					      NR_ANON_TRANSPARENT_HUGEPAGES);
 	}
-	if (unlikely(PageKsm(page)))
+	if (unlikely(PageKsm(page))) {
+		__inc_zone_page_state(page, NR_KSM_PAGES_SHARING);
 		return;
+	}
 
 	VM_BUG_ON(!PageLocked(page));
 	VM_BUG_ON(address < vma->vm_start || address >= vma->vm_end);
@@ -949,6 +951,9 @@ void page_add_file_rmap(struct page *page)
  */
 void page_remove_rmap(struct page *page)
 {
+	if (PageKsm(page))
+		__dec_zone_page_state(page, NR_KSM_PAGES_SHARING);
+
 	/* page still mapped by someone else? */
 	if (!atomic_add_negative(-1, &page->_mapcount))
 		return;

---

  

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

[Nai Xia]

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-----
/*
 * This test program triggers frequent faults on merged ksm pages but 
 * still keeps the faulted pages mergeable.
 */

#include <stdio.h>
#include <stdlib.h>
#include <errno.h>
#include <string.h>
#include <unistd.h>
#include <sys/mman.h>

#define MADV_MERGEABLE   12
#define MADV_UNMERGEABLE 13


#define SIZE (1000*1024*1024)
#define SEED	1
#define PAGE_SIZE 4096

int main(int argc, char **argv)
{
	char *p;
	int j;
	long feed = 1, new_feed, tmp;
	unsigned int offset;
	int status;
	int ret;

	pid_t child;

	child = fork();

	p = mmap(NULL, SIZE, PROT_WRITE|PROT_READ, 
		 MAP_PRIVATE|MAP_ANONYMOUS, 0, 0);
    	if (p = MAP_FAILED) {
    		printf("mmap error\n");
    		return 0;
    	}

	ret = madvise(p, SIZE, MADV_MERGEABLE);

	if (ret=-1) {
		printf("madvise failed \n");
		return 0;
	}

	memset(p, feed, SIZE);

	while (1) {
		for (j=0; j<SIZE; j+= PAGE_SIZE) {
			    p[j] *= p[j]*1;
		}
	}

	return 0;
}
----test.c ends-------

Some of the patch lines removes trailing spaces in related files.

Signed-off-by: Nai Xia <nai.xia@gmail.com>
---
diff --git a/fs/proc/meminfo.c b/fs/proc/meminfo.c
index ed257d1..dd0ff82 100644
--- a/fs/proc/meminfo.c
+++ b/fs/proc/meminfo.c
@@ -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
@@ -145,6 +148,9 @@ static int meminfo_proc_show(struct seq_file *m, void *v)
 		K(global_page_state(NR_SLAB_UNRECLAIMABLE)),
 		global_page_state(NR_KERNEL_STACK) * THREAD_SIZE / 1024,
 		K(global_page_state(NR_PAGETABLE)),
+#ifdef CONFIG_KSM
+		K(global_page_state(NR_KSM_PAGES_SHARING)),
+#endif
 #ifdef CONFIG_QUICKLIST
 		K(quicklist_total_size()),
 #endif
diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
index 02ecb01..01450e3 100644
--- a/include/linux/mmzone.h
+++ b/include/linux/mmzone.h
@@ -115,6 +115,9 @@ enum zone_stat_item {
 	NUMA_OTHER,		/* allocation from other node */
 #endif
 	NR_ANON_TRANSPARENT_HUGEPAGES,
+#ifdef CONFIG_KSM
+	NR_KSM_PAGES_SHARING,
+#endif
 	NR_VM_ZONE_STAT_ITEMS };
 
 /*
@@ -344,7 +347,7 @@ struct zone {
 	ZONE_PADDING(_pad1_)
 
 	/* Fields commonly accessed by the page reclaim scanner */
-	spinlock_t		lru_lock;	
+	spinlock_t		lru_lock;
 	struct zone_lru {
 		struct list_head list;
 	} lru[NR_LRU_LISTS];
@@ -722,7 +725,7 @@ static inline int is_normal_idx(enum zone_type idx)
 }
 
 /**
- * is_highmem - helper function to quickly check if a struct zone is a 
+ * is_highmem - helper function to quickly check if a struct zone is a
  *              highmem zone or not.  This is an attempt to keep references
  *              to ZONE_{DMA/NORMAL/HIGHMEM/etc} in general code to a 
minimum.
  * @zone - pointer to struct zone variable
diff --git a/mm/ksm.c b/mm/ksm.c
index c2b2a94..3c22d30 100644
--- a/mm/ksm.c
+++ b/mm/ksm.c
@@ -897,6 +897,7 @@ static int try_to_merge_one_page(struct vm_area_struct 
*vma,
 	 */
 	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:
@@ -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);
diff --git a/mm/memory.c b/mm/memory.c
index 8e8c183..d86abe9 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -719,6 +719,8 @@ copy_one_pte(struct mm_struct *dst_mm, struct mm_struct 
*src_mm,
 			rss[MM_ANONPAGES]++;
 		else
 			rss[MM_FILEPAGES]++;
+		if (PageKsm(page)) /* follows page_dup_rmap() */
+			inc_zone_page_state(page, NR_KSM_PAGES_SHARING);
 	}
 
 out_set_pte:
@@ -1423,7 +1425,7 @@ int __get_user_pages(struct task_struct *tsk, struct 
mm_struct *mm,
 
 	VM_BUG_ON(!!pages != !!(gup_flags & FOLL_GET));
 
-	/* 
+	/*
 	 * Require read or write permissions.
 	 * If FOLL_FORCE is set, we only require the "MAY" flags.
 	 */
diff --git a/mm/rmap.c b/mm/rmap.c
index f21f4a1..0d7ab31 100644
--- a/mm/rmap.c
+++ b/mm/rmap.c
@@ -801,9 +801,9 @@ void page_move_anon_rmap(struct page *page,
 
 /**
  * __page_set_anon_rmap - set up new anonymous rmap
- * @page:	Page to add to rmap	
+ * @page:	Page to add to rmap
  * @vma:	VM area to add page to.
- * @address:	User virtual address of the mapping	
+ * @address:	User virtual address of the mapping
  * @exclusive:	the page is exclusively owned by the current process
  */
 static void __page_set_anon_rmap(struct page *page,
@@ -889,8 +889,10 @@ void do_page_add_anon_rmap(struct page *page,
 			__inc_zone_page_state(page,
 					      NR_ANON_TRANSPARENT_HUGEPAGES);
 	}
-	if (unlikely(PageKsm(page)))
+	if (unlikely(PageKsm(page))) {
+		__inc_zone_page_state(page, NR_KSM_PAGES_SHARING);
 		return;
+	}
 
 	VM_BUG_ON(!PageLocked(page));
 	VM_BUG_ON(address < vma->vm_start || address >= vma->vm_end);
@@ -949,6 +951,9 @@ void page_add_file_rmap(struct page *page)
  */
 void page_remove_rmap(struct page *page)
 {
+	if (PageKsm(page))
+		__dec_zone_page_state(page, NR_KSM_PAGES_SHARING);
+
 	/* page still mapped by someone else? */
 	if (!atomic_add_negative(-1, &page->_mapcount))
 		return;

---

  

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

[Nai Xia]

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-----
/*
 * This test program triggers frequent faults on merged ksm pages but 
 * still keeps the faulted pages mergeable.
 */

#include <stdio.h>
#include <stdlib.h>
#include <errno.h>
#include <string.h>
#include <unistd.h>
#include <sys/mman.h>

#define MADV_MERGEABLE   12
#define MADV_UNMERGEABLE 13


#define SIZE (1000*1024*1024)
#define SEED	1
#define PAGE_SIZE 4096

int main(int argc, char **argv)
{
	char *p;
	int j;
	long feed = 1, new_feed, tmp;
	unsigned int offset;
	int status;
	int ret;

	pid_t child;

	child = fork();

	p = mmap(NULL, SIZE, PROT_WRITE|PROT_READ, 
		 MAP_PRIVATE|MAP_ANONYMOUS, 0, 0);
    	if (p == MAP_FAILED) {
    		printf("mmap error\n");
    		return 0;
    	}

	ret = madvise(p, SIZE, MADV_MERGEABLE);

	if (ret==-1) {
		printf("madvise failed \n");
		return 0;
	}

	memset(p, feed, SIZE);

	while (1) {
		for (j=0; j<SIZE; j+= PAGE_SIZE) {
			    p[j] *= p[j]*1;
		}
	}

	return 0;
}
----test.c ends-------

Some of the patch lines removes trailing spaces in related files.

Signed-off-by: Nai Xia <nai.xia@gmail.com>
---
diff --git a/fs/proc/meminfo.c b/fs/proc/meminfo.c
index ed257d1..dd0ff82 100644
--- a/fs/proc/meminfo.c
+++ b/fs/proc/meminfo.c
@@ -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
@@ -145,6 +148,9 @@ static int meminfo_proc_show(struct seq_file *m, void *v)
 		K(global_page_state(NR_SLAB_UNRECLAIMABLE)),
 		global_page_state(NR_KERNEL_STACK) * THREAD_SIZE / 1024,
 		K(global_page_state(NR_PAGETABLE)),
+#ifdef CONFIG_KSM
+		K(global_page_state(NR_KSM_PAGES_SHARING)),
+#endif
 #ifdef CONFIG_QUICKLIST
 		K(quicklist_total_size()),
 #endif
diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
index 02ecb01..01450e3 100644
--- a/include/linux/mmzone.h
+++ b/include/linux/mmzone.h
@@ -115,6 +115,9 @@ enum zone_stat_item {
 	NUMA_OTHER,		/* allocation from other node */
 #endif
 	NR_ANON_TRANSPARENT_HUGEPAGES,
+#ifdef CONFIG_KSM
+	NR_KSM_PAGES_SHARING,
+#endif
 	NR_VM_ZONE_STAT_ITEMS };
 
 /*
@@ -344,7 +347,7 @@ struct zone {
 	ZONE_PADDING(_pad1_)
 
 	/* Fields commonly accessed by the page reclaim scanner */
-	spinlock_t		lru_lock;	
+	spinlock_t		lru_lock;
 	struct zone_lru {
 		struct list_head list;
 	} lru[NR_LRU_LISTS];
@@ -722,7 +725,7 @@ static inline int is_normal_idx(enum zone_type idx)
 }
 
 /**
- * is_highmem - helper function to quickly check if a struct zone is a 
+ * is_highmem - helper function to quickly check if a struct zone is a
  *              highmem zone or not.  This is an attempt to keep references
  *              to ZONE_{DMA/NORMAL/HIGHMEM/etc} in general code to a 
minimum.
  * @zone - pointer to struct zone variable
diff --git a/mm/ksm.c b/mm/ksm.c
index c2b2a94..3c22d30 100644
--- a/mm/ksm.c
+++ b/mm/ksm.c
@@ -897,6 +897,7 @@ static int try_to_merge_one_page(struct vm_area_struct 
*vma,
 	 */
 	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:
@@ -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);
diff --git a/mm/memory.c b/mm/memory.c
index 8e8c183..d86abe9 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -719,6 +719,8 @@ copy_one_pte(struct mm_struct *dst_mm, struct mm_struct 
*src_mm,
 			rss[MM_ANONPAGES]++;
 		else
 			rss[MM_FILEPAGES]++;
+		if (PageKsm(page)) /* follows page_dup_rmap() */
+			inc_zone_page_state(page, NR_KSM_PAGES_SHARING);
 	}
 
 out_set_pte:
@@ -1423,7 +1425,7 @@ int __get_user_pages(struct task_struct *tsk, struct 
mm_struct *mm,
 
 	VM_BUG_ON(!!pages != !!(gup_flags & FOLL_GET));
 
-	/* 
+	/*
 	 * Require read or write permissions.
 	 * If FOLL_FORCE is set, we only require the "MAY" flags.
 	 */
diff --git a/mm/rmap.c b/mm/rmap.c
index f21f4a1..0d7ab31 100644
--- a/mm/rmap.c
+++ b/mm/rmap.c
@@ -801,9 +801,9 @@ void page_move_anon_rmap(struct page *page,
 
 /**
  * __page_set_anon_rmap - set up new anonymous rmap
- * @page:	Page to add to rmap	
+ * @page:	Page to add to rmap
  * @vma:	VM area to add page to.
- * @address:	User virtual address of the mapping	
+ * @address:	User virtual address of the mapping
  * @exclusive:	the page is exclusively owned by the current process
  */
 static void __page_set_anon_rmap(struct page *page,
@@ -889,8 +889,10 @@ void do_page_add_anon_rmap(struct page *page,
 			__inc_zone_page_state(page,
 					      NR_ANON_TRANSPARENT_HUGEPAGES);
 	}
-	if (unlikely(PageKsm(page)))
+	if (unlikely(PageKsm(page))) {
+		__inc_zone_page_state(page, NR_KSM_PAGES_SHARING);
 		return;
+	}
 
 	VM_BUG_ON(!PageLocked(page));
 	VM_BUG_ON(address < vma->vm_start || address >= vma->vm_end);
@@ -949,6 +951,9 @@ void page_add_file_rmap(struct page *page)
  */
 void page_remove_rmap(struct page *page)
 {
+	if (PageKsm(page))
+		__dec_zone_page_state(page, NR_KSM_PAGES_SHARING);
+
 	/* page still mapped by someone else? */
 	if (!atomic_add_negative(-1, &page->_mapcount))
 		return;

---

  

--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] ksm: add vm_stat and meminfo entry to reflect pte mapping to ksm pages

[Dave Hansen]

On Sat, 2011-02-26 at 22:56 +0800, Nai Xia wrote
> @@ -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); 

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

Re: [PATCH] ksm: add vm_stat and meminfo entry to reflect pte

[Dave Hansen]

On Sat, 2011-02-26 at 22:56 +0800, Nai Xia wrote
> @@ -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); 

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

Re: [PATCH] ksm: add vm_stat and meminfo entry to reflect pte mapping to ksm pages

[Dave Hansen]

On Sat, 2011-02-26 at 22:56 +0800, Nai Xia wrote
> @@ -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); 

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

--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] ksm: add vm_stat and meminfo entry to reflect pte mapping to ksm pages

[Andrew Morton]

On Sat, 26 Feb 2011 22:56:31 +0800
Nai Xia <nai.xia@gmail.com> 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.

> 
> --- test.c-----
>

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

> +++ b/mm/ksm.c
> @@ -897,6 +897,7 @@ static int try_to_merge_one_page(struct vm_area_struct 
> *vma,

Your email client wordwraps the patches.

Re: [PATCH] ksm: add vm_stat and meminfo entry to reflect pte

[Andrew Morton]

On Sat, 26 Feb 2011 22:56:31 +0800
Nai Xia <nai.xia@gmail.com> 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.

> 
> --- test.c-----
>

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

> +++ b/mm/ksm.c
> @@ -897,6 +897,7 @@ static int try_to_merge_one_page(struct vm_area_struct 
> *vma,

Your email client wordwraps the patches.

Re: [PATCH] ksm: add vm_stat and meminfo entry to reflect pte mapping to ksm pages

[Andrew Morton]

On Sat, 26 Feb 2011 22:56:31 +0800
Nai Xia <nai.xia@gmail.com> 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.

> 
> --- test.c-----
>

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

> +++ b/mm/ksm.c
> @@ -897,6 +897,7 @@ static int try_to_merge_one_page(struct vm_area_struct 
> *vma,

Your email client wordwraps the patches.

--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] ksm: add vm_stat and meminfo entry to reflect pte mapping to ksm pages

[Andrew Morton]

On Sat, 26 Feb 2011 22:56:31 +0800
Nai Xia <nai.xia@gmail.com> 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. 
> 
> ...
>
> --- a/fs/proc/meminfo.c
> +++ b/fs/proc/meminfo.c
> @@ -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);

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.

--- a/mm/ksm.c~ksm-add-vm_stat-and-meminfo-entry-to-reflect-pte-mapping-to-ksm-pages-fix
+++ a/mm/ksm.c
@@ -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);
--- a/mm/memory.c~ksm-add-vm_stat-and-meminfo-entry-to-reflect-pte-mapping-to-ksm-pages-fix
+++ a/mm/memory.c
@@ -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:
--- a/mm/rmap.c~ksm-add-vm_stat-and-meminfo-entry-to-reflect-pte-mapping-to-ksm-pages-fix
+++ a/mm/rmap.c
@@ -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;
_

Re: [PATCH] ksm: add vm_stat and meminfo entry to reflect pte

[Andrew Morton]

On Sat, 26 Feb 2011 22:56:31 +0800
Nai Xia <nai.xia@gmail.com> 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. 
> 
> ...
>
> --- a/fs/proc/meminfo.c
> +++ b/fs/proc/meminfo.c
> @@ -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);

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.

--- a/mm/ksm.c~ksm-add-vm_stat-and-meminfo-entry-to-reflect-pte-mapping-to-ksm-pages-fix
+++ a/mm/ksm.c
@@ -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);
--- a/mm/memory.c~ksm-add-vm_stat-and-meminfo-entry-to-reflect-pte-mapping-to-ksm-pages-fix
+++ a/mm/memory.c
@@ -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:
--- a/mm/rmap.c~ksm-add-vm_stat-and-meminfo-entry-to-reflect-pte-mapping-to-ksm-pages-fix
+++ a/mm/rmap.c
@@ -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;
_

Re: [PATCH] ksm: add vm_stat and meminfo entry to reflect pte mapping to ksm pages

[Andrew Morton]

On Sat, 26 Feb 2011 22:56:31 +0800
Nai Xia <nai.xia@gmail.com> 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. 
> 
> ...
>
> --- a/fs/proc/meminfo.c
> +++ b/fs/proc/meminfo.c
> @@ -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);

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.

--- a/mm/ksm.c~ksm-add-vm_stat-and-meminfo-entry-to-reflect-pte-mapping-to-ksm-pages-fix
+++ a/mm/ksm.c
@@ -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);
--- a/mm/memory.c~ksm-add-vm_stat-and-meminfo-entry-to-reflect-pte-mapping-to-ksm-pages-fix
+++ a/mm/memory.c
@@ -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:
--- a/mm/rmap.c~ksm-add-vm_stat-and-meminfo-entry-to-reflect-pte-mapping-to-ksm-pages-fix
+++ a/mm/rmap.c
@@ -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;
_

--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] ksm: add vm_stat and meminfo entry to reflect pte mapping to ksm pages

[xianai]

>On Tuesday 01 March 2011, at 10:21:48, <Dave Hansen <dave@linux.vnet.ibm.com>> wrote
> On Sat, 2011-02-26 at 22:56 +0800, Nai Xia wrote
> > @@ -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); 
> 
> 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.

Yes, thanks for pointing out, I will fix it soon. And sorry for the late 
response,there was a bug in my mail client which prevents this mail from being 
filtered out.

Nai

> 
> -- Dave
> 
> 

Re: [PATCH] ksm: add vm_stat and meminfo entry to reflect pte mapping to ksm pages

[xianai]

>On Tuesday 01 March 2011, at 10:21:48, <Dave Hansen <dave@linux.vnet.ibm.com>> wrote
> On Sat, 2011-02-26 at 22:56 +0800, Nai Xia wrote
> > @@ -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); 
> 
> 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.

Yes, thanks for pointing out, I will fix it soon. And sorry for the late 
response,there was a bug in my mail client which prevents this mail from being 
filtered out.

Nai

> 
> -- Dave
> 
> 

Re: [PATCH] ksm: add vm_stat and meminfo entry to reflect pte mapping to ksm pages

[xianai]

>On Tuesday 01 March 2011, at 10:21:48, <Dave Hansen <dave@linux.vnet.ibm.com>> wrote
> On Sat, 2011-02-26 at 22:56 +0800, Nai Xia wrote
> > @@ -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); 
> 
> 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.

Yes, thanks for pointing out, I will fix it soon. And sorry for the late 
response,there was a bug in my mail client which prevents this mail from being 
filtered out.

Nai

> 
> -- Dave
> 
> 

--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] ksm: add vm_stat and meminfo entry to reflect pte mapping to ksm pages

[Nai Xia]

>On Wednesday 02 March 2011, at 07:41:00, <Andrew Morton <akpm@linux-foundation.org>> wrote
> On Sat, 26 Feb 2011 22:56:31 +0800
> Nai Xia <nai.xia@gmail.com> 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?


The ksm_pages_sharing is really a count for how many "rmap_item"s is currently
linked in stable_nodes. ksmd updates ksm_pages_sharing whenever it's waken up.
However, just during the time ksmd is sleeping, many shared KSM pages can be 
broken because of page writes. So ksm_pages_sharing means much more an internal
state for ksm than a real count for how much pages is being shared at some time
point. Since the state of the internal data structures of ksm is only updated 
by ksmd privately. I think it's hard to make it correctly reflect the real time
memory saving. So I just added another count and let ksm_pages_sharing still 
focus on it original role.

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

Yes, I will make it to the zoneinfo, soon. 

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

OK, I understand now. :)

> 
> > +++ b/mm/ksm.c
> > @@ -897,6 +897,7 @@ static int try_to_merge_one_page(struct vm_area_struct 
> > *vma,
> 
> Your email client wordwraps the patches.

Oops, sorry for the crappy client it is also responsible for my late reply.
I will fix it soon.

Nai

> 

Re: [PATCH] ksm: add vm_stat and meminfo entry to reflect pte mapping to ksm pages

[Nai Xia]

>On Wednesday 02 March 2011, at 07:41:00, <Andrew Morton <akpm@linux-foundation.org>> wrote
> On Sat, 26 Feb 2011 22:56:31 +0800
> Nai Xia <nai.xia@gmail.com> 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?


The ksm_pages_sharing is really a count for how many "rmap_item"s is currently
linked in stable_nodes. ksmd updates ksm_pages_sharing whenever it's waken up.
However, just during the time ksmd is sleeping, many shared KSM pages can be 
broken because of page writes. So ksm_pages_sharing means much more an internal
state for ksm than a real count for how much pages is being shared at some time
point. Since the state of the internal data structures of ksm is only updated 
by ksmd privately. I think it's hard to make it correctly reflect the real time
memory saving. So I just added another count and let ksm_pages_sharing still 
focus on it original role.

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

Yes, I will make it to the zoneinfo, soon. 

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

OK, I understand now. :)

> 
> > +++ b/mm/ksm.c
> > @@ -897,6 +897,7 @@ static int try_to_merge_one_page(struct vm_area_struct 
> > *vma,
> 
> Your email client wordwraps the patches.

Oops, sorry for the crappy client it is also responsible for my late reply.
I will fix it soon.

Nai

> 

Re: [PATCH] ksm: add vm_stat and meminfo entry to reflect pte mapping to ksm pages

[Nai Xia]

>On Wednesday 02 March 2011, at 07:41:00, <Andrew Morton <akpm@linux-foundation.org>> wrote
> On Sat, 26 Feb 2011 22:56:31 +0800
> Nai Xia <nai.xia@gmail.com> 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?


The ksm_pages_sharing is really a count for how many "rmap_item"s is currently
linked in stable_nodes. ksmd updates ksm_pages_sharing whenever it's waken up.
However, just during the time ksmd is sleeping, many shared KSM pages can be 
broken because of page writes. So ksm_pages_sharing means much more an internal
state for ksm than a real count for how much pages is being shared at some time
point. Since the state of the internal data structures of ksm is only updated 
by ksmd privately. I think it's hard to make it correctly reflect the real time
memory saving. So I just added another count and let ksm_pages_sharing still 
focus on it original role.

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

Yes, I will make it to the zoneinfo, soon. 

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

OK, I understand now. :)

> 
> > +++ b/mm/ksm.c
> > @@ -897,6 +897,7 @@ static int try_to_merge_one_page(struct vm_area_struct 
> > *vma,
> 
> Your email client wordwraps the patches.

Oops, sorry for the crappy client it is also responsible for my late reply.
I will fix it soon.

Nai

> 

--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] ksm: add vm_stat and meminfo entry to reflect pte mapping to ksm pages

[Nai Xia]

>On Thursday 03 March 2011, at 06:31:42, <Andrew Morton <akpm@linux-foundation.org>> 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.

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

Thanks,
-Nai
> 
> --- a/mm/ksm.c~ksm-add-vm_stat-and-meminfo-entry-to-reflect-pte-mapping-to-ksm-pages-fix
> +++ a/mm/ksm.c
> @@ -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);
> --- a/mm/memory.c~ksm-add-vm_stat-and-meminfo-entry-to-reflect-pte-mapping-to-ksm-pages-fix
> +++ a/mm/memory.c
> @@ -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:
> --- a/mm/rmap.c~ksm-add-vm_stat-and-meminfo-entry-to-reflect-pte-mapping-to-ksm-pages-fix
> +++ a/mm/rmap.c
> @@ -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;
> _
> 
> 

Re: [PATCH] ksm: add vm_stat and meminfo entry to reflect pte mapping to ksm pages

[Nai Xia]

>On Thursday 03 March 2011, at 06:31:42, <Andrew Morton <akpm@linux-foundation.org>> 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.

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

Thanks,
-Nai
> 
> --- a/mm/ksm.c~ksm-add-vm_stat-and-meminfo-entry-to-reflect-pte-mapping-to-ksm-pages-fix
> +++ a/mm/ksm.c
> @@ -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);
> --- a/mm/memory.c~ksm-add-vm_stat-and-meminfo-entry-to-reflect-pte-mapping-to-ksm-pages-fix
> +++ a/mm/memory.c
> @@ -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:
> --- a/mm/rmap.c~ksm-add-vm_stat-and-meminfo-entry-to-reflect-pte-mapping-to-ksm-pages-fix
> +++ a/mm/rmap.c
> @@ -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;
> _
> 
> 

Re: [PATCH] ksm: add vm_stat and meminfo entry to reflect pte mapping to ksm pages

[Nai Xia]

>On Thursday 03 March 2011, at 06:31:42, <Andrew Morton <akpm@linux-foundation.org>> 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.

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

Thanks,
-Nai
> 
> --- a/mm/ksm.c~ksm-add-vm_stat-and-meminfo-entry-to-reflect-pte-mapping-to-ksm-pages-fix
> +++ a/mm/ksm.c
> @@ -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);
> --- a/mm/memory.c~ksm-add-vm_stat-and-meminfo-entry-to-reflect-pte-mapping-to-ksm-pages-fix
> +++ a/mm/memory.c
> @@ -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:
> --- a/mm/rmap.c~ksm-add-vm_stat-and-meminfo-entry-to-reflect-pte-mapping-to-ksm-pages-fix
> +++ a/mm/rmap.c
> @@ -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;
> _
> 
> 

--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] ksm: add vm_stat and meminfo entry to reflect pte mapping to ksm pages

[Hugh Dickins]

On Fri, 18 Mar 2011, Nai Xia wrote:
> >On Thursday 03 March 2011, at 06:31:42, <Andrew Morton <akpm@linux-foundation.org>> 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.
> 
> > 
> > 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. 

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

Re: [PATCH] ksm: add vm_stat and meminfo entry to reflect pte mapping

[Hugh Dickins]

On Fri, 18 Mar 2011, Nai Xia wrote:
> >On Thursday 03 March 2011, at 06:31:42, <Andrew Morton <akpm@linux-foundation.org>> 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.
> 
> > 
> > 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. 

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

Re: [PATCH] ksm: add vm_stat and meminfo entry to reflect pte mapping to ksm pages

[Hugh Dickins]

On Fri, 18 Mar 2011, Nai Xia wrote:
> >On Thursday 03 March 2011, at 06:31:42, <Andrew Morton <akpm@linux-foundation.org>> 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.
> 
> > 
> > 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. 

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

--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] ksm: add vm_stat and meminfo entry to reflect pte mapping to ksm pages

[Nai Xia]

On Sat, Mar 19, 2011 at 6:40 AM, Hugh Dickins <hughd@google.com> wrote:
> On Fri, 18 Mar 2011, Nai Xia wrote:
>> >On Thursday 03 March 2011, at 06:31:42, <Andrew Morton <akpm@linux-foundation.org>> 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.
>>
>> >
>> > 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.
>
> 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.

This was lately pointed out by me and canceled by another patch in
mm-commits@vger.kernel.org and CCed to your obsolete email
address: hugh.dickins@tiscali.co.uk I think.

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

This is just what I thought before I introduced NR_KSM_PAGES_SHARING,
which then did break the compiling. My mistake.

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

OK, I have to explain, here.
This patch is actually a tiny part of a bunch of code I wrote to improve ksm
in several aspects(This is somewhat off the topic but if you are interested,
please take at look at  http://code.google.com/p/uksm/, It's still on
very early
stage).

In my code, the inconsistency is amplified by non-uniform
scan speed for different VMAs and significantly improved max scan speed.
Then I think this patch may also be helpful to ksm itself.  Just as you said,
I had thought it at least improves the accuracy.

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

Then how about not touching the sexy meminfo and adding a new
interface file in /sys/kernel/mm/ksm/ ?  OK, on condition that the bug
below can be properly solved.

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

You are right,  copy_one_pte does not take page lock. So it's definitely a
bug in my patch, although it did not appear in my tests.  Actually, there is
another issue in my patch: It tries to count all the ptes, while actually only
those changed by ksmd really matter, those added by fork does not mean
memory savings. I had thought not taking the mapcount
, instead, only increase the count by one each time a pte is changed by ksmd,
but It seems also hard to tell a pte mapped to ksm page was previously
changed by ksmd or by fork when it gets unmapped.

So indeed, I have no idea to fix this bug for the time being.


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

Yes, I agree on your point. Let's hope there is an efficient and
simple solution.
But for now,  please drop this patch, Andrew.

Nai

Re: [PATCH] ksm: add vm_stat and meminfo entry to reflect pte mapping

[Nai Xia]

On Sat, Mar 19, 2011 at 6:40 AM, Hugh Dickins <hughd@google.com> wrote:
> On Fri, 18 Mar 2011, Nai Xia wrote:
>> >On Thursday 03 March 2011, at 06:31:42, <Andrew Morton <akpm@linux-foundation.org>> 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.
>>
>> >
>> > 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.
>
> 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.

This was lately pointed out by me and canceled by another patch in
mm-commits@vger.kernel.org and CCed to your obsolete email
address: hugh.dickins@tiscali.co.uk I think.

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

This is just what I thought before I introduced NR_KSM_PAGES_SHARING,
which then did break the compiling. My mistake.

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

OK, I have to explain, here.
This patch is actually a tiny part of a bunch of code I wrote to improve ksm
in several aspects(This is somewhat off the topic but if you are interested,
please take at look at  http://code.google.com/p/uksm/, It's still on
very early
stage).

In my code, the inconsistency is amplified by non-uniform
scan speed for different VMAs and significantly improved max scan speed.
Then I think this patch may also be helpful to ksm itself.  Just as you said,
I had thought it at least improves the accuracy.

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

Then how about not touching the sexy meminfo and adding a new
interface file in /sys/kernel/mm/ksm/ ?  OK, on condition that the bug
below can be properly solved.

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

You are right,  copy_one_pte does not take page lock. So it's definitely a
bug in my patch, although it did not appear in my tests.  Actually, there is
another issue in my patch: It tries to count all the ptes, while actually only
those changed by ksmd really matter, those added by fork does not mean
memory savings. I had thought not taking the mapcount
, instead, only increase the count by one each time a pte is changed by ksmd,
but It seems also hard to tell a pte mapped to ksm page was previously
changed by ksmd or by fork when it gets unmapped.

So indeed, I have no idea to fix this bug for the time being.


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

Yes, I agree on your point. Let's hope there is an efficient and
simple solution.
But for now,  please drop this patch, Andrew.

Nai
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ksm: add vm_stat and meminfo entry to reflect pte mapping to ksm pages

[Nai Xia]

On Sat, Mar 19, 2011 at 6:40 AM, Hugh Dickins <hughd@google.com> wrote:
> On Fri, 18 Mar 2011, Nai Xia wrote:
>> >On Thursday 03 March 2011, at 06:31:42, <Andrew Morton <akpm@linux-foundation.org>> 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.
>>
>> >
>> > 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.
>
> 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.

This was lately pointed out by me and canceled by another patch in
mm-commits@vger.kernel.org and CCed to your obsolete email
address: hugh.dickins@tiscali.co.uk I think.

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

This is just what I thought before I introduced NR_KSM_PAGES_SHARING,
which then did break the compiling. My mistake.

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

OK, I have to explain, here.
This patch is actually a tiny part of a bunch of code I wrote to improve ksm
in several aspects(This is somewhat off the topic but if you are interested,
please take at look at  http://code.google.com/p/uksm/, It's still on
very early
stage).

In my code, the inconsistency is amplified by non-uniform
scan speed for different VMAs and significantly improved max scan speed.
Then I think this patch may also be helpful to ksm itself.  Just as you said,
I had thought it at least improves the accuracy.

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

Then how about not touching the sexy meminfo and adding a new
interface file in /sys/kernel/mm/ksm/ ?  OK, on condition that the bug
below can be properly solved.

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

You are right,  copy_one_pte does not take page lock. So it's definitely a
bug in my patch, although it did not appear in my tests.  Actually, there is
another issue in my patch: It tries to count all the ptes, while actually only
those changed by ksmd really matter, those added by fork does not mean
memory savings. I had thought not taking the mapcount
, instead, only increase the count by one each time a pte is changed by ksmd,
but It seems also hard to tell a pte mapped to ksm page was previously
changed by ksmd or by fork when it gets unmapped.

So indeed, I have no idea to fix this bug for the time being.


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

Yes, I agree on your point. Let's hope there is an efficient and
simple solution.
But for now,  please drop this patch, Andrew.

Nai

--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>