| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Fri, 9 Jan 2009 12:09:50 +0900
From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@...fujitsu.com>
To: Daisuke Nishimura <nishimura@....nes.nec.co.jp>
Cc: linux-mm@...ck.org, linux-kernel@...r.kernel.org,
balbir@...ux.vnet.ibm.com, lizf@...fujitsu.com, menage@...gle.com
Subject: Re: [RFC][PATCH 3/4] memcg: fix for mem_cgroup_hierarchical_reclaim
On Fri, 9 Jan 2009 11:51:03 +0900
Daisuke Nishimura <nishimura@....nes.nec.co.jp> wrote:
> On Fri, 9 Jan 2009 10:08:30 +0900, KAMEZAWA Hiroyuki <kamezawa.hiroyu@...fujitsu.com> wrote:
> > On Thu, 8 Jan 2009 20:08:01 +0900 (JST)
> > "KAMEZAWA Hiroyuki" <kamezawa.hiroyu@...fujitsu.com> wrote:
> >
> > > Daisuke Nishimura said:
> > > > If root_mem has no children, last_scaned_child is set to root_mem itself.
> > > > But after some children added to root_mem, mem_cgroup_get_next_node can
> > > > mem_cgroup_put the root_mem although root_mem has not been mem_cgroup_get.
> > > >
> > > > This patch fixes this behavior by:
> > > > - Set last_scanned_child to NULL if root_mem has no children or DFS search
> > > > has returned to root_mem itself(root_mem is not a "child" of root_mem).
> > > > Make mem_cgroup_get_first_node return root_mem in this case.
> > > > There are no mem_cgroup_get/put for root_mem.
> > > > - Rename mem_cgroup_get_next_node to __mem_cgroup_get_next_node, and
> > > > mem_cgroup_get_first_node to mem_cgroup_get_next_node.
> > > > Make mem_cgroup_hierarchical_reclaim call only new
> > > > mem_cgroup_get_next_node.
> > > >
> > > >
> > > > Signed-off-by: Daisuke Nishimura <nishimura@....nes.nec.co.jp>
> > >
> > > Hmm, seems necessary fix. Then, it's better to rebase my patch on to this.
> > >
> > > Reviewed-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@...fujitsu.com>
> > >
> > > Maybe simpler one can be written but my patch remove all this out later....
> > >
> > How about this ? (just an exmaple and not tested, sorry)
> >
> >
> hmm, I don't think it's much simpler than this one(I don't want last_scanned_child
> to point to the mem itself, because it's not a "child").
>
> This part will be re-written by your patch, but this patch is needed
> to fix a bug(I saw general protection fault), so let's fix one by one.
of course.
> I'll post my original version. It's well tested :)
>
Ok, please go ahead.
Maybe create a patch agains "rc1" is better for all your fixes.
And please ask Andrew to "This is a bugfix and please put into fast-path"
-Kame
> Thanks,
> Daisuke Nishimura.
>
> >
> > ---
> > mm/memcontrol.c | 52 ++++++++++++++++++++++------------------------------
> > 1 file changed, 22 insertions(+), 30 deletions(-)
> >
> > Index: mmotm-2.6.28-Jan7/mm/memcontrol.c
> > ===================================================================
> > --- mmotm-2.6.28-Jan7.orig/mm/memcontrol.c
> > +++ mmotm-2.6.28-Jan7/mm/memcontrol.c
> > @@ -621,6 +621,7 @@ static struct mem_cgroup *
> > mem_cgroup_get_next_node(struct mem_cgroup *curr, struct mem_cgroup *root_mem)
> > {
> > struct cgroup *cgroup, *curr_cgroup, *root_cgroup;
> > + struct mem_cgroup *orig = root_mem->last_scanned_child;
> >
> > curr_cgroup = curr->css.cgroup;
> > root_cgroup = root_mem->css.cgroup;
> > @@ -629,19 +630,15 @@ mem_cgroup_get_next_node(struct mem_cgro
> > /*
> > * Walk down to children
> > */
> > - mem_cgroup_put(curr);
> > cgroup = list_entry(curr_cgroup->children.next,
> > struct cgroup, sibling);
> > curr = mem_cgroup_from_cont(cgroup);
> > - mem_cgroup_get(curr);
> > goto done;
> > }
> >
> > visit_parent:
> > if (curr_cgroup == root_cgroup) {
> > - mem_cgroup_put(curr);
> > curr = root_mem;
> > - mem_cgroup_get(curr);
> > goto done;
> > }
> >
> > @@ -649,11 +646,9 @@ visit_parent:
> > * Goto next sibling
> > */
> > if (curr_cgroup->sibling.next != &curr_cgroup->parent->children) {
> > - mem_cgroup_put(curr);
> > cgroup = list_entry(curr_cgroup->sibling.next, struct cgroup,
> > sibling);
> > curr = mem_cgroup_from_cont(cgroup);
> > - mem_cgroup_get(curr);
> > goto done;
> > }
> >
> > @@ -664,7 +659,10 @@ visit_parent:
> > goto visit_parent;
> >
> > done:
> > + if (orig)
> > + mem_cgroup_put(orig);
> > root_mem->last_scanned_child = curr;
> > + mem_cgroup_get(curr);
> > return curr;
> > }
> >
> > @@ -677,35 +675,25 @@ static struct mem_cgroup *
> > mem_cgroup_get_first_node(struct mem_cgroup *root_mem)
> > {
> > struct cgroup *cgroup;
> > - struct mem_cgroup *ret;
> > - bool obsolete;
> > + struct mem_cgroup *ret, *orig;
> >
> > - obsolete = mem_cgroup_is_obsolete(root_mem->last_scanned_child);
> > -
> > - /*
> > - * Scan all children under the mem_cgroup mem
> > - */
> > mutex_lock(&mem_cgroup_subsys.hierarchy_mutex);
> > - if (list_empty(&root_mem->css.cgroup->children)) {
> > - ret = root_mem;
> > - goto done;
> > - }
> > -
> > - if (!root_mem->last_scanned_child || obsolete) {
> > -
> > - if (obsolete && root_mem->last_scanned_child)
> > - mem_cgroup_put(root_mem->last_scanned_child);
> > + orig = root_mem->last_scanned_child;
> >
> > - cgroup = list_first_entry(&root_mem->css.cgroup->children,
> > - struct cgroup, sibling);
> > - ret = mem_cgroup_from_cont(cgroup);
> > + if (!orig) {
> > + if (list_empty(&root_mem->css.cgroup->children)) {
> > + ret = root_mem;
> > + } else {
> > + cgroup =
> > + list_first_entry(&root_mem->css.cgroup->children,
> > + struct cgroup, sibling);
> > + ret = mem_cgroup_from_cont(cgroup);
> > + }
> > + root_mem->last_scanned_child = ret;
> > mem_cgroup_get(ret);
> > - } else
> > + } else /* get_next_node will manage refcnt */
> > ret = mem_cgroup_get_next_node(root_mem->last_scanned_child,
> > root_mem);
> > -
> > -done:
> > - root_mem->last_scanned_child = ret;
> > mutex_unlock(&mem_cgroup_subsys.hierarchy_mutex);
> > return ret;
> > }
> > @@ -2232,7 +2220,11 @@ static void mem_cgroup_pre_destroy(struc
> > static void mem_cgroup_destroy(struct cgroup_subsys *ss,
> > struct cgroup *cont)
> > {
> > - mem_cgroup_put(mem_cgroup_from_cont(cont));
> > + struct mem_cgroup *mem = mem_cgroup_from_cont(cont);
> > +
> > + if (mem->last_scanned_child == mem)
> > + mem_cgroup_put(mem);
> > + mem_cgroup_put(mem);
> > }
> >
> > static int mem_cgroup_populate(struct cgroup_subsys *ss,
> >
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists