On Thu 24-05-18 14:16:35, Andrew Morton wrote: > On Thu, 24 May 2018 13:10:02 +0200 Michal Hocko <mho...@kernel.org> wrote: > > > I would really prefer and appreciate a repost with all the fixes folded > > in. > > [1/2]
Thanks Andrew and sorry it took so long! This seems to be missing the fix for the issue I've mentioned in http://lkml.kernel.org/r/20180510121838.ge5...@dhcp22.suse.cz and Eric wanted to handle by http://lkml.kernel.org/r/87wovu889o....@xmission.com. I do not think that this fix is really correct one though. I will comment in a reply to that email. In any case I really think this should better be reposted in one patch series and restart the discussion. I strongly suggest revisiting my previous attempt http://lkml.kernel.org/r/1436358472-29137-8-git-send-email-mho...@kernel.org including the follow up discussion regarding the unfortunate CLONE_VM outside of thread group case and potential solution for that. > -static void mm_init_owner(struct mm_struct *mm, struct task_struct *p) > +static void mm_init_memcg(struct mm_struct *mm) > { > #ifdef CONFIG_MEMCG > - mm->owner = p; > + struct cgroup_subsys_state *css; > + > + /* Ensure mm->memcg is initialized */ > + mm->memcg = NULL; > + > + rcu_read_lock(); > + css = task_css(current, memory_cgrp_id); > + if (css && css_tryget(css)) > + mm_update_memcg(mm, mem_cgroup_from_css(css)); > + rcu_read_unlock(); Is it possible that css_tryget ever fails here? I remember I used to do plain css_get in my patch. > @@ -4850,15 +4836,16 @@ static int mem_cgroup_can_attach(struct > if (!move_flags) > return 0; > > - from = mem_cgroup_from_task(p); > + from = mem_cgroup_from_css(task_css(p, memory_cgrp_id)); > > VM_BUG_ON(from == memcg); > > mm = get_task_mm(p); > if (!mm) > return 0; > - /* We move charges only when we move a owner of the mm */ > - if (mm->owner == p) { > + > + /* We move charges except for creative uses of CLONE_VM */ > + if (mm->memcg == from) { I must be missing something here but how do you prevent those cases? AFAICS processes sharing the mm will simply allow to migrate. > VM_BUG_ON(mc.from); > VM_BUG_ON(mc.to); > VM_BUG_ON(mc.precharge); Other than that the patch makes sense to me. -- Michal Hocko SUSE Labs
