Message ID | 20210413065153.63431-3-songmuchun@bytedance.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | memcontrol code cleanup and simplification | expand |
On Tue, Apr 13, 2021 at 02:51:48PM +0800, Muchun Song wrote: > When mm is NULL, we do not need to hold rcu lock and call css_tryget for > the root memcg. And we also do not need to check !mm in every loop of > while. So bail out early when !mm. > > Signed-off-by: Muchun Song <songmuchun@bytedance.com> > Acked-by: Johannes Weiner <hannes@cmpxchg.org> > Reviewed-by: Shakeel Butt <shakeelb@google.com> Acked-by: Roman Gushchin <guro@fb.com> Nice! > --- > mm/memcontrol.c | 21 ++++++++++----------- > 1 file changed, 10 insertions(+), 11 deletions(-) > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > index f229de925aa5..9cbfff59b171 100644 > --- a/mm/memcontrol.c > +++ b/mm/memcontrol.c > @@ -901,20 +901,19 @@ struct mem_cgroup *get_mem_cgroup_from_mm(struct mm_struct *mm) > if (mem_cgroup_disabled()) > return NULL; > > + /* > + * Page cache insertions can happen without an > + * actual mm context, e.g. during disk probing > + * on boot, loopback IO, acct() writes etc. > + */ > + if (unlikely(!mm)) > + return root_mem_cgroup; > + > rcu_read_lock(); > do { > - /* > - * Page cache insertions can happen without an > - * actual mm context, e.g. during disk probing > - * on boot, loopback IO, acct() writes etc. > - */ > - if (unlikely(!mm)) > + memcg = mem_cgroup_from_task(rcu_dereference(mm->owner)); > + if (unlikely(!memcg)) > memcg = root_mem_cgroup; > - else { > - memcg = mem_cgroup_from_task(rcu_dereference(mm->owner)); > - if (unlikely(!memcg)) > - memcg = root_mem_cgroup; > - } > } while (!css_tryget(&memcg->css)); > rcu_read_unlock(); > return memcg; > -- > 2.11.0 >
On Tue 13-04-21 14:51:48, Muchun Song wrote: > When mm is NULL, we do not need to hold rcu lock and call css_tryget for > the root memcg. And we also do not need to check !mm in every loop of > while. So bail out early when !mm. mem_cgroup_charge and other callers unconditionally drop the reference so how come this does not underflow reference count? > Signed-off-by: Muchun Song <songmuchun@bytedance.com> > Acked-by: Johannes Weiner <hannes@cmpxchg.org> > Reviewed-by: Shakeel Butt <shakeelb@google.com> > --- > mm/memcontrol.c | 21 ++++++++++----------- > 1 file changed, 10 insertions(+), 11 deletions(-) > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > index f229de925aa5..9cbfff59b171 100644 > --- a/mm/memcontrol.c > +++ b/mm/memcontrol.c > @@ -901,20 +901,19 @@ struct mem_cgroup *get_mem_cgroup_from_mm(struct mm_struct *mm) > if (mem_cgroup_disabled()) > return NULL; > > + /* > + * Page cache insertions can happen without an > + * actual mm context, e.g. during disk probing > + * on boot, loopback IO, acct() writes etc. > + */ > + if (unlikely(!mm)) > + return root_mem_cgroup; > + > rcu_read_lock(); > do { > - /* > - * Page cache insertions can happen without an > - * actual mm context, e.g. during disk probing > - * on boot, loopback IO, acct() writes etc. > - */ > - if (unlikely(!mm)) > + memcg = mem_cgroup_from_task(rcu_dereference(mm->owner)); > + if (unlikely(!memcg)) > memcg = root_mem_cgroup; > - else { > - memcg = mem_cgroup_from_task(rcu_dereference(mm->owner)); > - if (unlikely(!memcg)) > - memcg = root_mem_cgroup; > - } > } while (!css_tryget(&memcg->css)); > rcu_read_unlock(); > return memcg; > -- > 2.11.0
On Wed, Apr 14, 2021 at 5:24 PM Michal Hocko <mhocko@suse.com> wrote: > > On Tue 13-04-21 14:51:48, Muchun Song wrote: > > When mm is NULL, we do not need to hold rcu lock and call css_tryget for > > the root memcg. And we also do not need to check !mm in every loop of > > while. So bail out early when !mm. > > mem_cgroup_charge and other callers unconditionally drop the reference > so how come this does not underflow reference count? For the root memcg, the CSS_NO_REF flag is set, so css_get and css_put do not get or put reference. Thanks. > > > Signed-off-by: Muchun Song <songmuchun@bytedance.com> > > Acked-by: Johannes Weiner <hannes@cmpxchg.org> > > Reviewed-by: Shakeel Butt <shakeelb@google.com> > > --- > > mm/memcontrol.c | 21 ++++++++++----------- > > 1 file changed, 10 insertions(+), 11 deletions(-) > > > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > > index f229de925aa5..9cbfff59b171 100644 > > --- a/mm/memcontrol.c > > +++ b/mm/memcontrol.c > > @@ -901,20 +901,19 @@ struct mem_cgroup *get_mem_cgroup_from_mm(struct mm_struct *mm) > > if (mem_cgroup_disabled()) > > return NULL; > > > > + /* > > + * Page cache insertions can happen without an > > + * actual mm context, e.g. during disk probing > > + * on boot, loopback IO, acct() writes etc. > > + */ > > + if (unlikely(!mm)) > > + return root_mem_cgroup; > > + > > rcu_read_lock(); > > do { > > - /* > > - * Page cache insertions can happen without an > > - * actual mm context, e.g. during disk probing > > - * on boot, loopback IO, acct() writes etc. > > - */ > > - if (unlikely(!mm)) > > + memcg = mem_cgroup_from_task(rcu_dereference(mm->owner)); > > + if (unlikely(!memcg)) > > memcg = root_mem_cgroup; > > - else { > > - memcg = mem_cgroup_from_task(rcu_dereference(mm->owner)); > > - if (unlikely(!memcg)) > > - memcg = root_mem_cgroup; > > - } > > } while (!css_tryget(&memcg->css)); > > rcu_read_unlock(); > > return memcg; > > -- > > 2.11.0 > > -- > Michal Hocko > SUSE Labs
On Wed 14-04-21 18:04:35, Muchun Song wrote: > On Wed, Apr 14, 2021 at 5:24 PM Michal Hocko <mhocko@suse.com> wrote: > > > > On Tue 13-04-21 14:51:48, Muchun Song wrote: > > > When mm is NULL, we do not need to hold rcu lock and call css_tryget for > > > the root memcg. And we also do not need to check !mm in every loop of > > > while. So bail out early when !mm. > > > > mem_cgroup_charge and other callers unconditionally drop the reference > > so how come this does not underflow reference count? > > For the root memcg, the CSS_NO_REF flag is set, so css_get > and css_put do not get or put reference. Ohh, right you are. I must have forgot about that special case. I am pretty sure I (and likely few more) will stumble over that in the future again. A small comment explaining that the reference can be safely ignore would be helpful. Anyway Acked-by: Michal Hocko <mhocko@suse.com> Thanks!
On Wed, Apr 14, 2021 at 6:15 PM Michal Hocko <mhocko@suse.com> wrote: > > On Wed 14-04-21 18:04:35, Muchun Song wrote: > > On Wed, Apr 14, 2021 at 5:24 PM Michal Hocko <mhocko@suse.com> wrote: > > > > > > On Tue 13-04-21 14:51:48, Muchun Song wrote: > > > > When mm is NULL, we do not need to hold rcu lock and call css_tryget for > > > > the root memcg. And we also do not need to check !mm in every loop of > > > > while. So bail out early when !mm. > > > > > > mem_cgroup_charge and other callers unconditionally drop the reference > > > so how come this does not underflow reference count? > > > > For the root memcg, the CSS_NO_REF flag is set, so css_get > > and css_put do not get or put reference. > > Ohh, right you are. I must have forgot about that special case. I am > pretty sure I (and likely few more) will stumble over that in the future > again. A small comment explaining that the reference can be safely > ignore would be helpful. OK. Will do. > > Anyway > Acked-by: Michal Hocko <mhocko@suse.com> Thanks. > > Thanks! > > -- > Michal Hocko > SUSE Labs
On Thu 15-04-21 11:13:16, Muchun Song wrote: > On Wed, Apr 14, 2021 at 6:15 PM Michal Hocko <mhocko@suse.com> wrote: > > > > On Wed 14-04-21 18:04:35, Muchun Song wrote: > > > On Wed, Apr 14, 2021 at 5:24 PM Michal Hocko <mhocko@suse.com> wrote: > > > > > > > > On Tue 13-04-21 14:51:48, Muchun Song wrote: > > > > > When mm is NULL, we do not need to hold rcu lock and call css_tryget for > > > > > the root memcg. And we also do not need to check !mm in every loop of > > > > > while. So bail out early when !mm. > > > > > > > > mem_cgroup_charge and other callers unconditionally drop the reference > > > > so how come this does not underflow reference count? > > > > > > For the root memcg, the CSS_NO_REF flag is set, so css_get > > > and css_put do not get or put reference. > > > > Ohh, right you are. I must have forgot about that special case. I am > > pretty sure I (and likely few more) will stumble over that in the future > > again. A small comment explaining that the reference can be safely > > ignore would be helpful. > > OK. Will do. I would go with the following if that helps /* * No need to css_get on root memcg as the reference counting is * disabled on the root level in the cgroup core. See CSS_NO_REF */ Thanks
diff --git a/mm/memcontrol.c b/mm/memcontrol.c index f229de925aa5..9cbfff59b171 100644 --- a/mm/memcontrol.c +++ b/mm/memcontrol.c @@ -901,20 +901,19 @@ struct mem_cgroup *get_mem_cgroup_from_mm(struct mm_struct *mm) if (mem_cgroup_disabled()) return NULL; + /* + * Page cache insertions can happen without an + * actual mm context, e.g. during disk probing + * on boot, loopback IO, acct() writes etc. + */ + if (unlikely(!mm)) + return root_mem_cgroup; + rcu_read_lock(); do { - /* - * Page cache insertions can happen without an - * actual mm context, e.g. during disk probing - * on boot, loopback IO, acct() writes etc. - */ - if (unlikely(!mm)) + memcg = mem_cgroup_from_task(rcu_dereference(mm->owner)); + if (unlikely(!memcg)) memcg = root_mem_cgroup; - else { - memcg = mem_cgroup_from_task(rcu_dereference(mm->owner)); - if (unlikely(!memcg)) - memcg = root_mem_cgroup; - } } while (!css_tryget(&memcg->css)); rcu_read_unlock(); return memcg;