| Message ID | 20201028035013.99711-1-songmuchun@bytedance.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] mm: memcg/slab: Fix return child memcg objcg for root memcg | expand |
On Wed, Oct 28, 2020 at 11:50:10AM +0800, Muchun Song wrote: > Consider the following memcg hierarchy. > > root > / \ > A B > > If we get the objcg of memcg A failed, the get_obj_cgroup_from_current > can return the wrong objcg for the root memcg. > > Fixes: bf4f059954dc ("mm: memcg/slab: obj_cgroup API") > Signed-off-by: Muchun Song <songmuchun@bytedance.com> Acked-by: Roman Gushchin <guro@fb.com> Thanks!
On Tue, Oct 27, 2020 at 8:50 PM Muchun Song <songmuchun@bytedance.com> wrote: > > Consider the following memcg hierarchy. > > root > / \ > A B > > If we get the objcg of memcg A failed, Please fix the above statement. > the get_obj_cgroup_from_current > can return the wrong objcg for the root memcg. > > Fixes: bf4f059954dc ("mm: memcg/slab: obj_cgroup API") > Signed-off-by: Muchun Song <songmuchun@bytedance.com> > --- > changelog in v2: > 1. Do not use a comparison with the root_mem_cgroup > > mm/memcontrol.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > index 1337775b04f3..8c8b4c3ed5a0 100644 > --- a/mm/memcontrol.c > +++ b/mm/memcontrol.c > @@ -2961,6 +2961,7 @@ __always_inline struct obj_cgroup *get_obj_cgroup_from_current(void) > objcg = rcu_dereference(memcg->objcg); > if (objcg && obj_cgroup_tryget(objcg)) > break; > + objcg = NULL; Roman, in your cleanup, are you planning to have objcg for root memcg as well? > } > rcu_read_unlock(); > > -- > 2.20.1 >
On Thu, Oct 29, 2020 at 11:48 PM Shakeel Butt <shakeelb@google.com> wrote: > > On Tue, Oct 27, 2020 at 8:50 PM Muchun Song <songmuchun@bytedance.com> wrote: > > > > Consider the following memcg hierarchy. > > > > root > > / \ > > A B > > > > If we get the objcg of memcg A failed, > > Please fix the above statement. Sorry, could you be more specific, I don't quite understand. Thanks. > > > the get_obj_cgroup_from_current > > can return the wrong objcg for the root memcg. > > > > Fixes: bf4f059954dc ("mm: memcg/slab: obj_cgroup API") > > Signed-off-by: Muchun Song <songmuchun@bytedance.com> > > --- > > changelog in v2: > > 1. Do not use a comparison with the root_mem_cgroup > > > > mm/memcontrol.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > > index 1337775b04f3..8c8b4c3ed5a0 100644 > > --- a/mm/memcontrol.c > > +++ b/mm/memcontrol.c > > @@ -2961,6 +2961,7 @@ __always_inline struct obj_cgroup *get_obj_cgroup_from_current(void) > > objcg = rcu_dereference(memcg->objcg); > > if (objcg && obj_cgroup_tryget(objcg)) > > break; > > + objcg = NULL; > > Roman, in your cleanup, are you planning to have objcg for root memcg as well? > > > } > > rcu_read_unlock(); > > > > -- > > 2.20.1 > > -- Yours, Muchun
On Thu, Oct 29, 2020 at 9:09 AM Muchun Song <songmuchun@bytedance.com> wrote: > > On Thu, Oct 29, 2020 at 11:48 PM Shakeel Butt <shakeelb@google.com> wrote: > > > > On Tue, Oct 27, 2020 at 8:50 PM Muchun Song <songmuchun@bytedance.com> wrote: > > > > > > Consider the following memcg hierarchy. > > > > > > root > > > / \ > > > A B > > > > > > If we get the objcg of memcg A failed, > > > > Please fix the above statement. > > Sorry, could you be more specific, I don't quite understand. Fix the grammar. Something like "If we failed to get the reference on objcg of memcg A..."
On Fri, Oct 30, 2020 at 12:18 AM Shakeel Butt <shakeelb@google.com> wrote: > > On Thu, Oct 29, 2020 at 9:09 AM Muchun Song <songmuchun@bytedance.com> wrote: > > > > On Thu, Oct 29, 2020 at 11:48 PM Shakeel Butt <shakeelb@google.com> wrote: > > > > > > On Tue, Oct 27, 2020 at 8:50 PM Muchun Song <songmuchun@bytedance.com> wrote: > > > > > > > > Consider the following memcg hierarchy. > > > > > > > > root > > > > / \ > > > > A B > > > > > > > > If we get the objcg of memcg A failed, > > > > > > Please fix the above statement. > > > > Sorry, could you be more specific, I don't quite understand. > > Fix the grammar. Something like "If we failed to get the reference on > objcg of memcg A..." Got it. Sorry for my poor English, Thanks.
On Thu, Oct 29, 2020 at 08:48:45AM -0700, Shakeel Butt wrote: > On Tue, Oct 27, 2020 at 8:50 PM Muchun Song <songmuchun@bytedance.com> wrote: > > > > Consider the following memcg hierarchy. > > > > root > > / \ > > A B > > > > If we get the objcg of memcg A failed, > > Please fix the above statement. > > > the get_obj_cgroup_from_current > > can return the wrong objcg for the root memcg. > > > > Fixes: bf4f059954dc ("mm: memcg/slab: obj_cgroup API") > > Signed-off-by: Muchun Song <songmuchun@bytedance.com> > > --- > > changelog in v2: > > 1. Do not use a comparison with the root_mem_cgroup > > > > mm/memcontrol.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > > index 1337775b04f3..8c8b4c3ed5a0 100644 > > --- a/mm/memcontrol.c > > +++ b/mm/memcontrol.c > > @@ -2961,6 +2961,7 @@ __always_inline struct obj_cgroup *get_obj_cgroup_from_current(void) > > objcg = rcu_dereference(memcg->objcg); > > if (objcg && obj_cgroup_tryget(objcg)) > > break; > > + objcg = NULL; > > Roman, in your cleanup, are you planning to have objcg for root memcg as well? Yes. I'll just change the for loop to include the root_mem_cgroup. Thanks!
On Thu, Oct 29, 2020 at 10:10 AM Roman Gushchin <guro@fb.com> wrote: > > On Thu, Oct 29, 2020 at 08:48:45AM -0700, Shakeel Butt wrote: > > On Tue, Oct 27, 2020 at 8:50 PM Muchun Song <songmuchun@bytedance.com> wrote: > > > > > > Consider the following memcg hierarchy. > > > > > > root > > > / \ > > > A B > > > > > > If we get the objcg of memcg A failed, > > > > Please fix the above statement. > > > > > the get_obj_cgroup_from_current > > > can return the wrong objcg for the root memcg. > > > > > > Fixes: bf4f059954dc ("mm: memcg/slab: obj_cgroup API") > > > Signed-off-by: Muchun Song <songmuchun@bytedance.com> > > > --- > > > changelog in v2: > > > 1. Do not use a comparison with the root_mem_cgroup > > > > > > mm/memcontrol.c | 1 + > > > 1 file changed, 1 insertion(+) > > > > > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > > > index 1337775b04f3..8c8b4c3ed5a0 100644 > > > --- a/mm/memcontrol.c > > > +++ b/mm/memcontrol.c > > > @@ -2961,6 +2961,7 @@ __always_inline struct obj_cgroup *get_obj_cgroup_from_current(void) > > > objcg = rcu_dereference(memcg->objcg); > > > if (objcg && obj_cgroup_tryget(objcg)) > > > break; > > > + objcg = NULL; > > > > Roman, in your cleanup, are you planning to have objcg for root memcg as well? > > Yes. I'll just change the for loop to include the root_mem_cgroup. > Then do we really need this patch since it's not tagged for stable?
On Thu, Oct 29, 2020 at 01:34:57PM -0700, Shakeel Butt wrote: > On Thu, Oct 29, 2020 at 10:10 AM Roman Gushchin <guro@fb.com> wrote: > > > > On Thu, Oct 29, 2020 at 08:48:45AM -0700, Shakeel Butt wrote: > > > On Tue, Oct 27, 2020 at 8:50 PM Muchun Song <songmuchun@bytedance.com> wrote: > > > > > > > > Consider the following memcg hierarchy. > > > > > > > > root > > > > / \ > > > > A B > > > > > > > > If we get the objcg of memcg A failed, > > > > > > Please fix the above statement. > > > > > > > the get_obj_cgroup_from_current > > > > can return the wrong objcg for the root memcg. > > > > > > > > Fixes: bf4f059954dc ("mm: memcg/slab: obj_cgroup API") > > > > Signed-off-by: Muchun Song <songmuchun@bytedance.com> > > > > --- > > > > changelog in v2: > > > > 1. Do not use a comparison with the root_mem_cgroup > > > > > > > > mm/memcontrol.c | 1 + > > > > 1 file changed, 1 insertion(+) > > > > > > > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > > > > index 1337775b04f3..8c8b4c3ed5a0 100644 > > > > --- a/mm/memcontrol.c > > > > +++ b/mm/memcontrol.c > > > > @@ -2961,6 +2961,7 @@ __always_inline struct obj_cgroup *get_obj_cgroup_from_current(void) > > > > objcg = rcu_dereference(memcg->objcg); > > > > if (objcg && obj_cgroup_tryget(objcg)) > > > > break; > > > > + objcg = NULL; > > > > > > Roman, in your cleanup, are you planning to have objcg for root memcg as well? > > > > Yes. I'll just change the for loop to include the root_mem_cgroup. > > > > Then do we really need this patch since it's not tagged for stable? A good question. Honestly, I really doubt that obj_cgroup_tryget() can fail even with the current code. But, formally speaking, it's possible. So there is likely no practical difference if we'll apply this patch or not, even without the root handling cleanup. But you're right, with the root handling cleanup, it will make even less of a difference. Thanks!
diff --git a/mm/memcontrol.c b/mm/memcontrol.c index 1337775b04f3..8c8b4c3ed5a0 100644 --- a/mm/memcontrol.c +++ b/mm/memcontrol.c @@ -2961,6 +2961,7 @@ __always_inline struct obj_cgroup *get_obj_cgroup_from_current(void) objcg = rcu_dereference(memcg->objcg); if (objcg && obj_cgroup_tryget(objcg)) break; + objcg = NULL; } rcu_read_unlock();
Consider the following memcg hierarchy. root / \ A B If we get the objcg of memcg A failed, the get_obj_cgroup_from_current can return the wrong objcg for the root memcg. Fixes: bf4f059954dc ("mm: memcg/slab: obj_cgroup API") Signed-off-by: Muchun Song <songmuchun@bytedance.com> --- changelog in v2: 1. Do not use a comparison with the root_mem_cgroup mm/memcontrol.c | 1 + 1 file changed, 1 insertion(+)