Re: [RFC][PATCH] memcg: fix a race when setting memcg.swappiness

[KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>]

On Wed, 14 Jan 2009 14:47:18 +0800
Li Zefan <lizf@cn.fujitsu.com> wrote:

> KAMEZAWA Hiroyuki wrote:
> > On Wed, 14 Jan 2009 11:24:18 +0800
> > Li Zefan <lizf@cn.fujitsu.com> wrote:
> > 
> >> (suppose: memcg->use_hierarchy == 0 and memcg->swappiness == 60)
> >>
> >> echo 10 > /memcg/0/swappiness   |
> >>   mem_cgroup_swappiness_write() |
> >>     ...                         | echo 1 > /memcg/0/use_hierarchy
> >>                                 | mkdir /mnt/0/1
> >>                                 |   sub_memcg->swappiness = 60;
> >>     memcg->swappiness = 10;     |
> >>
> >> In the above scenario, we end up having 2 different swappiness
> >> values in a single hierarchy.
> >>
> >> Note we can't use hierarchy_lock here, because it doesn't protect
> >> the create() method.
> >>
> >> Though IMO use cgroup_lock() in simple write functions is OK,
> >> Paul would like to avoid it. And he sugguested use a counter to
> >> count the number of children instead of check cgrp->children list:
> >>
> >> =================
> >> create() does:
> >>
> >> lock memcg_parent
> >> memcg->swappiness = memcg->parent->swappiness;
> >> memcg_parent->child_count++;
> >> unlock memcg_parent
> >>
> >> and write() does:
> >>
> >> lock memcg
> >> if (!memcg->child_count) {
> >>   memcg->swappiness = swappiness;
> >> } else {
> >>   report error;
> >> }
> >> unlock memcg
> >>
> >> destroy() does:
> >> lock memcg_parent
> >> memcg_parent->child_count--;
> >> unlock memcg_parent
> >>
> >> =================
> >>
> >> And there is a suble differnce with checking cgrp->children,
> >> that a cgroup is removed from parent's list in cgroup_rmdir(),
> >> while memcg->child_count is decremented in cgroup_diput().
> >>
> >>
> >> Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
> > 
> > Seems reasonable, but, hmm...
> > 
> 
> Do you mean you agree to avoid using cgroup_lock()?
> 
> > Why hierarchy_mutex can't be used for create() ?
> > 
> 
> We can make hierarchy_mutex work for this race by:
> 
> @@ -2403,16 +2403,18 @@ static long cgroup_create(struct cgroup *parent, struct
>         if (notify_on_release(parent))
>                 set_bit(CGRP_NOTIFY_ON_RELEASE, &cgrp->flags);
> 
> +       cgroup_lock_hierarchy(root);
> +
>         for_each_subsys(root, ss) {
>                 struct cgroup_subsys_state *css = ss->create(ss, cgrp);
>                 if (IS_ERR(css)) {
> +                       cgroup_unlock_hierarchy(root);
>                         err = PTR_ERR(css);
>                         goto err_destroy;
>                 }
>                 init_cgroup_css(css, ss, cgrp);
>         }
> 
> -       cgroup_lock_hierarchy(root);
>         list_add(&cgrp->sibling, &cgrp->parent->children);
>         cgroup_unlock_hierarchy(root);
>         root->number_of_cgroups++;
> 
> But this may not be what we want, because hierarchy_mutex is meant to be
> lightweight, so it's not held while subsys callbacks are invoked, except
> bind().
> 

Ah, I see your point. But "we can't trust hieararchy_lock for create()"
is a probelm.  How about following ?
==
for_each-subsys(root,ss) {
	if (ss->create) {
		mutex_lock(&ss->hierarchy_mutex);
		css = ss->create(ss, cgroup);
		mutex_unlock(&ss->hierarchy_mutex);
		if (IS_ERR(...)) {
		}
	}
==

-Kame



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