Re: [PATCH v6 23/29] memcg: destroy memcg caches

Michal Hocko
2012-11-02T20:19:36Z

On Fri 02-11-12 11:46:42, Glauber Costa wrote:
> On 11/02/2012 04:05 AM, Andrew Morton wrote:
> > On Thu, 1 Nov 2012 16:07:39 +0400
> > Glauber Costa <glommer@parallels.com> wrote:
> >
> >> This patch implements destruction of memcg caches. Right now,
> >> only caches where our reference counter is the last remaining are
> >> deleted. If there are any other reference counters around, we just
> >> leave the caches lying around until they go away.
> >>
> >> When that happen, a destruction function is called from the cache
> >> code. Caches are only destroyed in process context, so we queue them
> >> up for later processing in the general case.
> >>
> >>
> >> ...
> >>
> >> @@ -5950,6 +6012,7 @@ static int mem_cgroup_pre_destroy(struct cgroup *cont)
> >> {
> >> struct mem_cgroup *memcg = mem_cgroup_from_cont(cont);
> >>
> >> + mem_cgroup_destroy_all_caches(memcg);
> >> return mem_cgroup_force_empty(memcg, false);
> >> }
> >>
> >
> > Conflicts with linux-next cgroup changes. Looks pretty simple:
> >
> >
> > static int mem_cgroup_pre_destroy(struct cgroup *cont)
> > {
> > struct mem_cgroup *memcg = mem_cgroup_from_cont(cont);
> > int ret;
> >
> > css_get(&memcg->css);
> > ret = mem_cgroup_reparent_charges(memcg);
> > mem_cgroup_destroy_all_caches(memcg);
> > css_put(&memcg->css);
> >
> > return ret;
> > }
> >
>
> There is one significant difference between the code I had and the code
> after your fix up.
>
> In my patch, caches were destroyed before the call to
> mem_cgroup_force_empty. In the final, version, they are destroyed after it.
>
> I am here thinking, but I am not sure if this have any significant
> impact... If we run mem_cgroup_destroy_all_caches() before reparenting,
> we'll have shrunk a lot of the pending caches, and we will have less
> pages to reparent. But we only reparent pages in the lru anyway, and
> then expect kmem and remaining umem to match. So *in theory* it should
> be fine.
>
> Where can I grab your final tree so I can test it and make sure it is
> all good ?

Everything is in the -mm git tree (I tend to take mmots trees if they
compile).

--
Michal Hocko
SUSE Labs