diff mbox series

[2/7] mm: memcontrol: bail out early when !mm in get_mem_cgroup_from_mm

Message ID 20210413065153.63431-3-songmuchun@bytedance.com (mailing list archive)
State New, archived
Headers show
Series memcontrol code cleanup and simplification | expand

Commit Message

Muchun Song April 13, 2021, 6:51 a.m. UTC 
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>
---
 mm/memcontrol.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

Comments

Roman Gushchin April 13, 2021, 5:47 p.m. UTC | #1 
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
>
Michal Hocko April 14, 2021, 9:24 a.m. UTC | #2 
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
Muchun Song April 14, 2021, 10:04 a.m. UTC | #3 
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
Michal Hocko April 14, 2021, 10:15 a.m. UTC | #4 
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!
Muchun Song April 15, 2021, 3:13 a.m. UTC | #5 
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
Michal Hocko April 15, 2021, 7:09 a.m. UTC | #6 
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 mbox series

Patch

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;