Re: [External] Re: [PATCH 2/7] mm: memcontrol: bail out early when !mm in get_mem_cgroup_from_mm
On Thu 15-04-21 11:13:16, Muchun Song wrote: > On Wed, Apr 14, 2021 at 6:15 PM Michal Hocko wrote: > > > > On Wed 14-04-21 18:04:35, Muchun Song wrote: > > > On Wed, Apr 14, 2021 at 5:24 PM Michal Hocko 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 -- Michal Hocko SUSE Labs
Re: [External] Re: [PATCH 2/7] mm: memcontrol: bail out early when !mm in get_mem_cgroup_from_mm
On Wed, Apr 14, 2021 at 6:15 PM Michal Hocko wrote: > > On Wed 14-04-21 18:04:35, Muchun Song wrote: > > On Wed, Apr 14, 2021 at 5:24 PM Michal Hocko 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 Thanks. > > Thanks! > > -- > Michal Hocko > SUSE Labs
Re: [External] Re: [PATCH 2/7] mm: memcontrol: bail out early when !mm in get_mem_cgroup_from_mm
On Wed 14-04-21 18:04:35, Muchun Song wrote: > On Wed, Apr 14, 2021 at 5:24 PM Michal Hocko 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 Thanks! -- Michal Hocko SUSE Labs
Re: [External] Re: [PATCH 2/7] mm: memcontrol: bail out early when !mm in get_mem_cgroup_from_mm
On Wed, Apr 14, 2021 at 5:24 PM Michal Hocko 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
> > Acked-by: Johannes Weiner
> > Reviewed-by: Shakeel Butt
> > ---
> > 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(>css));
> > rcu_read_unlock();
> > return memcg;
> > --
> > 2.11.0
>
> --
> Michal Hocko
> SUSE Labs
Re: [PATCH 2/7] mm: memcontrol: bail out early when !mm in get_mem_cgroup_from_mm
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
> Acked-by: Johannes Weiner
> Reviewed-by: Shakeel Butt
> ---
> 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(>css));
> rcu_read_unlock();
> return memcg;
> --
> 2.11.0
--
Michal Hocko
SUSE Labs
Re: [PATCH 2/7] mm: memcontrol: bail out early when !mm in get_mem_cgroup_from_mm
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
> Acked-by: Johannes Weiner
> Reviewed-by: Shakeel Butt
Acked-by: Roman Gushchin
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(>css));
> rcu_read_unlock();
> return memcg;
> --
> 2.11.0
>
