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

[Wu Fengguang]

On Thu, Dec 03, 2009 at 10:58:40AM +0800, KAMEZAWA Hiroyuki wrote:
> On Thu, 3 Dec 2009 10:47:39 +0800
> Wu Fengguang <fengguang.wu@xxxxxxxxx> wrote:
> 
> > On Thu, Dec 03, 2009 at 10:28:22AM +0800, KAMEZAWA Hiroyuki wrote:
> > Ah please forgive my memcg ignorance..  Then how about bring back the
> > old css_id() based scheme (old patch follows)?
> > 
> maybe enough. but please take care of the fact that css is can be "reused"
> once freed.

OK.

> > > 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
> > 
> Ah, this is nice to be put into changelog or some documentation.

Good idea, I'll add it.

> > > 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@xxxxxxxxx>
> 
> No objections from me. please use "id" check. or adds new flag to
> struct mem_cgroup, as you like.

There's a 3rd option: inode number in cgroupfs.

test case:
                memcg_ino=$(ls -id /cgroup/hwpoison | cut -f1 -d' ')
                echo $memcg_ino > /debug/hwpoison/corrupt-filter-memcg

kernel code:
                hwpoison_filter_memcg ==
                memcg->css->cgroup->dentry->d_inode->i_ino

It's pretty long chain, but performance is not a big concern for test
purpose :) As long as the inode number will be accessible and unique
in long term.

This avoids adding extra interfaces to memcg. What do you think?

> The style I prefer is
> ==
>  struct mem_cgroup {
>   ....
>   bool hwpoison_test_enabled;
>  };
> 
> +#ifdef CONFIG_HWPOISON_INJECT /* for now, only user is hwpoison testing */
> +	{
> +		.name = "hwpoison_test_enable",
> +		.read_u64 = ....
> +	},
> +#endif
> 
> and.
> 	mem = try_get_mem_cgroup_from_page(p);
> 	if (mem_cgroup_is_under_poison_test(mem))
> 		ret = true;
> 	mem_cgroup_put(mem);	/* calls css_put() */

It seems mem_cgroup_put() does atomic_dec_and_test(&mem->refcnt).
Is that changed to css_put() recently?

> Maybe not difficult. and this is an usual way. But it's ok if you don't want to
> scannter HWPOISON things to other function's files. This is test operation.
> 
> So, "including real use case and patches" is only my request, for this time.

OK, thanks for the review!

Regards,
Fengguang
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/