From: Li Zefan on
> +#ifdef CONFIG_CGROUP_MEM_RES_CTLR_SWAP
> +u64 hwpoison_filter_memcg;
> +EXPORT_SYMBOL_GPL(hwpoison_filter_memcg);
> +static int hwpoison_filter_task(struct page *p)
> +{
> + struct mem_cgroup *mem;
> + struct cgroup_subsys_state *css;
> + unsigned long ino;
> +
> + if (!hwpoison_filter_memcg)
> + return 0;
> +
> + mem = try_get_mem_cgroup_from_page(p);
> + if (!mem)
> + return -EINVAL;
> +
> + css = mem_cgroup_css(mem);
> + ino = css->cgroup->dentry->d_inode->i_ino;

I have a question, can try_get_mem_cgroup_from_page() return
root_mem_cgroup?

if it can, then css->cgroup->dentry is NULL, if memcg is
not mounted and there is no subdir in memcg. Because the root
cgroup of an inactive subsystem has no dentry.

> + css_put(css);
> +
> + if (ino != hwpoison_filter_memcg)
> + return -EINVAL;
> +
> + return 0;
> +}
> +#else
> +static int hwpoison_filter_task(struct page *p) { return 0; }
> +#endif
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: KAMEZAWA Hiroyuki on
On Wed, 09 Dec 2009 13:04:06 +0800
Li Zefan <lizf(a)cn.fujitsu.com> wrote:

> > +#ifdef CONFIG_CGROUP_MEM_RES_CTLR_SWAP
> > +u64 hwpoison_filter_memcg;
> > +EXPORT_SYMBOL_GPL(hwpoison_filter_memcg);
> > +static int hwpoison_filter_task(struct page *p)
> > +{
> > + struct mem_cgroup *mem;
> > + struct cgroup_subsys_state *css;
> > + unsigned long ino;
> > +
> > + if (!hwpoison_filter_memcg)
> > + return 0;
> > +
> > + mem = try_get_mem_cgroup_from_page(p);
> > + if (!mem)
> > + return -EINVAL;
> > +
> > + css = mem_cgroup_css(mem);
> > + ino = css->cgroup->dentry->d_inode->i_ino;
>
> I have a question, can try_get_mem_cgroup_from_page() return
> root_mem_cgroup?
>
yes.

> if it can, then css->cgroup->dentry is NULL, if memcg is
> not mounted and there is no subdir in memcg. Because the root
> cgroup of an inactive subsystem has no dentry.
>

Nice catch. It sounds possible. That should be handled.

Regards,
-Kame

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Balbir Singh on
* KAMEZAWA Hiroyuki <kamezawa.hiroyu(a)jp.fujitsu.com> [2009-12-09 14:06:20]:

> On Wed, 09 Dec 2009 13:04:06 +0800
> Li Zefan <lizf(a)cn.fujitsu.com> wrote:
>
> > > +#ifdef CONFIG_CGROUP_MEM_RES_CTLR_SWAP
> > > +u64 hwpoison_filter_memcg;
> > > +EXPORT_SYMBOL_GPL(hwpoison_filter_memcg);
> > > +static int hwpoison_filter_task(struct page *p)
> > > +{
> > > + struct mem_cgroup *mem;
> > > + struct cgroup_subsys_state *css;
> > > + unsigned long ino;
> > > +
> > > + if (!hwpoison_filter_memcg)
> > > + return 0;
> > > +
> > > + mem = try_get_mem_cgroup_from_page(p);
> > > + if (!mem)
> > > + return -EINVAL;
> > > +
> > > + css = mem_cgroup_css(mem);
> > > + ino = css->cgroup->dentry->d_inode->i_ino;
> >
> > I have a question, can try_get_mem_cgroup_from_page() return
> > root_mem_cgroup?
> >
> yes.
>
> > if it can, then css->cgroup->dentry is NULL, if memcg is
> > not mounted and there is no subdir in memcg. Because the root
> > cgroup of an inactive subsystem has no dentry.
> >
>
> Nice catch. It sounds possible. That should be handled.
>
>

Yes, agreed, good catch!

--
Balbir
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Andi Kleen on
> I have a question, can try_get_mem_cgroup_from_page() return
> root_mem_cgroup?

It could be called for any page.


> if it can, then css->cgroup->dentry is NULL, if memcg is
> not mounted and there is no subdir in memcg. Because the root
> cgroup of an inactive subsystem has no dentry.

Thanks. I'll just add an return -EINVAL for this case, sounds good?

-Andi

--
ak(a)linux.intel.com -- Speaking for myself only.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Paul Menage on
On Tue, Dec 8, 2009 at 1:16 PM, Andi Kleen <andi(a)firstfloor.org> wrote:
>
> 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.)

While the functionality sounds useful, the interface (passing an inode
number) feels a bit ugly to me. Also, if that group is deleted and a
new cgroup created, you could end up reusing the inode number.

How about an approach where you write either the cgroup path (relative
to the memcg mount) or an fd open on the desired cgroup? Then you
could store a (counted) css reference rather than an inode number,
which would make the filter function cleaner too, since it would just
need to compare css objects.

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