Re: [PATCH 22/24] HWPOISON: add memory cgroup filter

[Wu Fengguang <fengguang.wu@intel.com>]

On Thu, Dec 03, 2009 at 10:28:22AM +0800, KAMEZAWA Hiroyuki wrote:
> On Thu, 3 Dec 2009 10:19:15 +0800
> Wu Fengguang <fengguang.wu@intel.com> wrote:
> 
> > On Thu, Dec 03, 2009 at 09:52:29AM +0800, KAMEZAWA Hiroyuki wrote:
> > > On Wed, 2 Dec 2009 20:58:42 +0800
> > > Wu Fengguang <fengguang.wu@intel.com> wrote:
> > > 
> > > > On Wed, Dec 02, 2009 at 08:44:46PM +0800, Andi Kleen wrote:
> > > > > >  
> > > > > > +static int hwpoison_filter_task(struct page *p)
> > > > > > +{
> > > > > 
> > > > > Can we make that ifdef instead of depends on ?
> > > > 
> > > > Sure. Here is the updated patch.
> > > > 
> > > > ---
> > > > HWPOISON: add memory cgroup filter
> > > > 
> > > > The hwpoison test suite need to inject hwpoison to a collection of
> > > > selected task pages, and must not touch pages not owned by them and
> > > > thus kill important system processes such as init. (But it's OK to
> > > > mis-hwpoison free/unowned pages as well as shared clean pages.
> > > > Mis-hwpoison of shared dirty pages will kill all tasks, so the test
> > > > suite will target all or non of such tasks in the first place.)
> > > > 
> > > > The memory cgroup serves this purpose well. We can put the target
> > > > processes under the control of a memory cgroup, and tell the hwpoison
> > > > injection code to only kill pages associated with some active memory
> > > > cgroup.
> > > > 
> > > > The prerequisite for doing hwpoison stress tests with mem_cgroup is,
> > > > the mem_cgroup code tracks task pages _accurately_ (unless page is
> > > > locked).  Which we believe is/should be true.
> > > > 
> > > > The benifits are simplification of hwpoison injector code. Also the
> > > > mem_cgroup code will automatically be tested by hwpoison test cases.
> > > > 
> > > > The alternative interfaces pin-pfn/unpin-pfn can also delegate the
> > > > (process and page flags) filtering functions reliably to user space.
> > > > However prototype implementation shows that this scheme adds more
> > > > complexity than we wanted.
> > > > 
> > > > CC: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
> > > > CC: Hugh Dickins <hugh.dickins@tiscali.co.uk>
> > > > CC: Daisuke Nishimura <nishimura@mxp.nes.nec.co.jp>
> > > > CC: Balbir Singh <balbir@linux.vnet.ibm.com>
> > > > CC: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> > > > CC: Li Zefan <lizf@cn.fujitsu.com>
> > > > CC: Paul Menage <menage@google.com>
> > > > CC: Nick Piggin <npiggin@suse.de> 
> > > > CC: Andi Kleen <andi@firstfloor.org> 
> > > > Signed-off-by: Wu Fengguang <fengguang.wu@intel.com>
> > > > ---
> > > >  mm/Kconfig           |    2 +-
> > > >  mm/hwpoison-inject.c |    7 +++++++
> > > >  mm/internal.h        |    1 +
> > > >  mm/memory-failure.c  |   28 ++++++++++++++++++++++++++++
> > > >  4 files changed, 37 insertions(+), 1 deletion(-)
> > > > 
> > > > --- linux-mm.orig/mm/memory-failure.c	2009-12-01 09:56:06.000000000 +0800
> > > > +++ linux-mm/mm/memory-failure.c	2009-12-02 20:56:55.000000000 +0800
> > > > @@ -96,6 +96,31 @@ static int hwpoison_filter_flags(struct 
> > > >  		return -EINVAL;
> > > >  }
> > > >  
> > > > +#ifdef	CONFIG_CGROUP_MEM_RES_CTLR_SWAP
> > > > +u32 hwpoison_filter_memcg;
> > > > +static int hwpoison_filter_task(struct page *p)
> > > > +{
> > > > +	struct mem_cgroup *mem;
> > > > +	struct cgroup_subsys_state *css;
> > > > +
> > > > +	if (!hwpoison_filter_memcg)
> > > > +		return 0;
> > > > +
> > > > +	mem = try_get_mem_cgroup_from_page(p);
> > > > +	if (!mem)
> > > > +		return -EINVAL;
> > > > +
> > > > +	css = mem_cgroup_css(mem);
> > > > +	if (!css)
> > > > +		return -EINVAL;
> > > 
> > > > +
> > > > +	css_put(css);
> > > > +	return 0;
> > > > +}
> > > 
> > > 
> > > Hmm..can you adds comment ? What does this function is for ?
> > 
> > Good idea. How about this one?
> > 
> > /*
> >  * This allows stress tests to limit test scope to a collection of tasks
> >  * by putting them under some memcg. This prevents killing unrelated/important
> >  * processes such as /sbin/init. Note that the target task may share clean
> >  * pages with init (eg. libc text), which is harmless. If the target task
> >  * share _dirty_ pages with another task B, the test scheme must make sure B
> >  * is also included in the memcg. At last, due to race conditions this filter
> >  * can only guarantee that the page either belongs to the memcg tasks, or is
> >  * a freed page.
> >  */
> > 
> Hmm. seems good but..by what means "avoiding killing /sbin/init" is done ?
> All process are under some memcg..

Ah please forgive my memcg ignorance..  Then how about bring back the
old css_id() based scheme (old patch follows)?

> If you have more patches to be usable the function above,
> I recommend you to post this with some real-use patches, in step by step.

Do you mean user space test case? Here is a simple one:

        #!/bin/sh

        TEST_PROG=usemem
        TEST_PARM="-m 100 -s 100"

        test -d /cgroup/hwpoison && rmdir /cgroup/hwpoison
        mkdir /cgroup/hwpoison

        $TEST_PROG $TEST_PARM &
        echo `pidof $TEST_PROG` > /cgroup/hwpoison/tasks

        memcg_id=$(</cgroup/hwpoison/memory.id)
        echo $memcg_id > /debug/hwpoison/corrupt-filter-memcg

        ./corrupt-all-pfn

> patch 19,20 is ok for me.

Thanks,
Fengguang
---
memcg: show memory.id in cgroupfs

The hwpoison test suite need to selectively inject hwpoison to some
targeted task pages, and must not kill important system processes
such as init.

The memory cgroup serves this purpose well. We can put the target
processes under the control of a memory cgroup, tell the hwpoison
injection code the id of that memory cgroup so that it will only
poison pages associated with it.

Signed-off-by: Wu Fengguang <fengguang.wu@intel.com>
---
 mm/memcontrol.c |   13 +++++++++++++
 1 file changed, 13 insertions(+)

--- linux-mm.orig/mm/memcontrol.c	2009-09-07 16:01:02.000000000 +0800
+++ linux-mm/mm/memcontrol.c	2009-09-11 18:20:55.000000000 +0800
@@ -2510,6 +2510,13 @@ mem_cgroup_get_recursive_idx_stat(struct
 	*val = d.val;
 }
 
+#ifdef CONFIG_HWPOISON_INJECT
+static u64 mem_cgroup_id_read(struct cgroup *cont, struct cftype *cft)
+{
+	return css_id(cgroup_subsys_state(cont, mem_cgroup_subsys_id));
+}
+#endif
+
 static u64 mem_cgroup_read(struct cgroup *cont, struct cftype *cft)
 {
 	struct mem_cgroup *mem = mem_cgroup_from_cont(cont);
@@ -2841,6 +2848,12 @@ static int mem_cgroup_swappiness_write(s
 
 
 static struct cftype mem_cgroup_files[] = {
+#ifdef CONFIG_HWPOISON_INJECT /* for now, only user is hwpoison testing */
+	{
+		.name = "id",
+		.read_u64 = mem_cgroup_id_read,
+	},
+#endif
 	{
 		.name = "usage_in_bytes",
 		.private = MEMFILE_PRIVATE(_MEM, RES_USAGE),

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