Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1453100 > unrolled thread

[PATCH] memcg: put soft limit reclaim out of way if the excess tree is empty

Started byMichal Hocko <mhocko@kernel.org>
First post2016-08-01 12:10 +0200
Last post2016-08-01 16:40 +0200
Articles 7 — 3 participants

Back to article view | Back to linux.kernel

Contents

  [PATCH] memcg: put soft limit reclaim out of way if the excess tree is empty Michal Hocko <mhocko@kernel.org> - 2016-08-01 12:10 +0200
    Re: [PATCH] memcg: put soft limit reclaim out of way if the excess  tree is empty Michal Hocko <mhocko@kernel.org> - 2016-08-01 16:20 +0200
      Re: [PATCH] memcg: put soft limit reclaim out of way if the excess  tree is empty Johannes Weiner <hannes@cmpxchg.org> - 2016-08-01 17:10 +0200
        Re: [PATCH] memcg: put soft limit reclaim out of way if the excess  tree is empty Michal Hocko <mhocko@kernel.org> - 2016-08-01 17:40 +0200
          Re: [PATCH] memcg: put soft limit reclaim out of way if the excess  tree is empty Johannes Weiner <hannes@cmpxchg.org> - 2016-08-01 19:20 +0200
            Re: [PATCH] memcg: put soft limit reclaim out of way if the excess  tree is empty Michal Hocko <mhocko@kernel.org> - 2016-08-01 19:50 +0200
    Re: [PATCH] memcg: put soft limit reclaim out of way if the excess  tree is empty Vladimir Davydov <vdavydov@virtuozzo.com> - 2016-08-01 16:40 +0200

#1453100 — [PATCH] memcg: put soft limit reclaim out of way if the excess tree is empty

FromMichal Hocko <mhocko@kernel.org>
Date2016-08-01 12:10 +0200
Subject[PATCH] memcg: put soft limit reclaim out of way if the excess tree is empty
Message-ID<s1ikx-4Uz-1@gated-at.bofh.it>
From: Michal Hocko <mhocko@suse.com>

We've had a report about soft lockups caused by lock bouncing in the
soft reclaim path:

[331404.849734] BUG: soft lockup - CPU#0 stuck for 22s! [kav4proxy-kavic:3128]
[331404.849920] RIP: 0010:[<ffffffff81469798>]  [<ffffffff81469798>] _raw_spin_lock+0x18/0x20
[331404.849997] Call Trace:
[331404.850010]  [<ffffffff811557ea>] mem_cgroup_soft_limit_reclaim+0x25a/0x280
[331404.850020]  [<ffffffff8111041d>] shrink_zones+0xed/0x200
[331404.850027]  [<ffffffff81111a94>] do_try_to_free_pages+0x74/0x320
[331404.850034]  [<ffffffff81112072>] try_to_free_pages+0x112/0x180
[331404.850042]  [<ffffffff81104a6f>] __alloc_pages_slowpath+0x3ff/0x820
[331404.850049]  [<ffffffff81105079>] __alloc_pages_nodemask+0x1e9/0x200
[331404.850056]  [<ffffffff81141e01>] alloc_pages_vma+0xe1/0x290
[331404.850064]  [<ffffffff8112402f>] do_wp_page+0x19f/0x840
[331404.850071]  [<ffffffff811257cd>] handle_pte_fault+0x1cd/0x230
[331404.850079]  [<ffffffff8146d3ed>] do_page_fault+0x1fd/0x4c0
[331404.850087]  [<ffffffff81469ec5>] page_fault+0x25/0x30

There are no memcgs created so there cannot be any in the soft limit
excess obviously:
[...]
memory  0       1       1

so all this just seems to be mem_cgroup_largest_soft_limit_node
trying to get spin_lock_irq(&mctz->lock) just to find out that the soft
limit excess tree is empty. This is just pointless waisting of cycles
and cache line bouncing during heavy parallel reclaim on large machines.
The particular machine wasn't very healthy and most probably suffering
from a memory leak which just caused the memory reclaim to trash
heavily. But bouncing on the lock certainly didn't help...

Introduce soft_limit_tree_empty which does the optimistic lockless check
and bail out early if the tree is empty. This is theoretically racy but
that shouldn't matter all that much. First of all soft limit is a best
effort feature and it is slowly getting deprecated and its usage should
be really scarce. Bouncing on a lock without a good reason is surely
much bigger problem, especially on large CPU machines.

Signed-off-by: Michal Hocko <mhocko@suse.com>
---
 mm/memcontrol.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index c265212bec8c..eb7e39c2d948 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -2543,6 +2543,11 @@ static int mem_cgroup_resize_memsw_limit(struct mem_cgroup *memcg,
 	return ret;
 }
 
+static inline bool soft_limit_tree_empty(struct mem_cgroup_tree_per_node *mctz)
+{
+	return rb_last(&mctz->rb_root) == NULL;
+}
+
 unsigned long mem_cgroup_soft_limit_reclaim(pg_data_t *pgdat, int order,
 					    gfp_t gfp_mask,
 					    unsigned long *total_scanned)
@@ -2559,6 +2564,9 @@ unsigned long mem_cgroup_soft_limit_reclaim(pg_data_t *pgdat, int order,
 		return 0;
 
 	mctz = soft_limit_tree_node(pgdat->node_id);
+	if (soft_limit_tree_empty(mctz))
+		return 0;
+
 	/*
 	 * This loop can run a while, specially if mem_cgroup's continuously
 	 * keep exceeding their soft limit and putting the system under
-- 
2.8.1

[toc] | [next] | [standalone]

#1453239 — Re: [PATCH] memcg: put soft limit reclaim out of way if the excess tree is empty

FromMichal Hocko <mhocko@kernel.org>
Date2016-08-01 16:20 +0200
SubjectRe: [PATCH] memcg: put soft limit reclaim out of way if the excess tree is empty
Message-ID<s1met-7pd-21@gated-at.bofh.it>
In reply to#1453100
On Mon 01-08-16 16:57:57, Vladimir Davydov wrote:
> On Mon, Aug 01, 2016 at 12:00:21PM +0200, Michal Hocko wrote:
> ...
> > diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> > index c265212bec8c..eb7e39c2d948 100644
> > --- a/mm/memcontrol.c
> > +++ b/mm/memcontrol.c
> > @@ -2543,6 +2543,11 @@ static int mem_cgroup_resize_memsw_limit(struct mem_cgroup *memcg,
> >  	return ret;
> >  }
> >  
> > +static inline bool soft_limit_tree_empty(struct mem_cgroup_tree_per_node *mctz)
> > +{
> > +	return rb_last(&mctz->rb_root) == NULL;
> > +}
> > +
> 
> I don't think traversing rb tree as rb_last() does w/o holding the lock
> is a good idea. Why is RB_EMPTY_ROOT() insufficient here?

Of course it is not. Dohh, forgot to refresh the patch! Sorry about
that.

Updated patch.
---
From 9076cc87cbc49d8c16cee4120c7f5e518511b953 Mon Sep 17 00:00:00 2001
From: Michal Hocko <mhocko@suse.com>
Date: Mon, 1 Aug 2016 10:42:06 +0200
Subject: [PATCH] memcg: put soft limit reclaim out of way if the excess tree
 is empty

We've had a report about soft lockups caused by lock bouncing in the
soft reclaim path:

[331404.849734] BUG: soft lockup - CPU#0 stuck for 22s! [kav4proxy-kavic:3128]
[331404.849920] RIP: 0010:[<ffffffff81469798>]  [<ffffffff81469798>] _raw_spin_lock+0x18/0x20
[331404.849997] Call Trace:
[331404.850010]  [<ffffffff811557ea>] mem_cgroup_soft_limit_reclaim+0x25a/0x280
[331404.850020]  [<ffffffff8111041d>] shrink_zones+0xed/0x200
[331404.850027]  [<ffffffff81111a94>] do_try_to_free_pages+0x74/0x320
[331404.850034]  [<ffffffff81112072>] try_to_free_pages+0x112/0x180
[331404.850042]  [<ffffffff81104a6f>] __alloc_pages_slowpath+0x3ff/0x820
[331404.850049]  [<ffffffff81105079>] __alloc_pages_nodemask+0x1e9/0x200
[331404.850056]  [<ffffffff81141e01>] alloc_pages_vma+0xe1/0x290
[331404.850064]  [<ffffffff8112402f>] do_wp_page+0x19f/0x840
[331404.850071]  [<ffffffff811257cd>] handle_pte_fault+0x1cd/0x230
[331404.850079]  [<ffffffff8146d3ed>] do_page_fault+0x1fd/0x4c0
[331404.850087]  [<ffffffff81469ec5>] page_fault+0x25/0x30

There are no memcgs created so there cannot be any in the soft limit
excess obviously:
[...]
memory  0       1       1

so all this just seems to be mem_cgroup_largest_soft_limit_node
trying to get spin_lock_irq(&mctz->lock) just to find out that the soft
limit excess tree is empty. This is just pointless waisting of cycles
and cache line bouncing during heavy parallel reclaim on large machines.
The particular machine wasn't very healthy and most probably suffering
from a memory leak which just caused the memory reclaim to trash
heavily. But bouncing on the lock certainly didn't help...

Introduce soft_limit_tree_empty which does the optimistic lockless check
and bail out early if the tree is empty. This is theoretically racy but
that shouldn't matter all that much. First of all soft limit is a best
effort feature and it is slowly getting deprecated and its usage should
be really scarce. Bouncing on a lock without a good reason is surely
much bigger problem, especially on large CPU machines.

Signed-off-by: Michal Hocko <mhocko@suse.com>
---
 mm/memcontrol.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index c265212bec8c..c0b57b6a194e 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -2543,6 +2543,11 @@ static int mem_cgroup_resize_memsw_limit(struct mem_cgroup *memcg,
 	return ret;
 }
 
+static inline bool soft_limit_tree_empty(struct mem_cgroup_tree_per_node *mctz)
+{
+	return RB_EMPTY_ROOT(&mctz->rb_root);
+}
+
 unsigned long mem_cgroup_soft_limit_reclaim(pg_data_t *pgdat, int order,
 					    gfp_t gfp_mask,
 					    unsigned long *total_scanned)
@@ -2559,6 +2564,9 @@ unsigned long mem_cgroup_soft_limit_reclaim(pg_data_t *pgdat, int order,
 		return 0;
 
 	mctz = soft_limit_tree_node(pgdat->node_id);
+	if (soft_limit_tree_empty(mctz))
+		return 0;
+
 	/*
 	 * This loop can run a while, specially if mem_cgroup's continuously
 	 * keep exceeding their soft limit and putting the system under
-- 
2.8.1

-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1453269 — Re: [PATCH] memcg: put soft limit reclaim out of way if the excess tree is empty

FromJohannes Weiner <hannes@cmpxchg.org>
Date2016-08-01 17:10 +0200
SubjectRe: [PATCH] memcg: put soft limit reclaim out of way if the excess tree is empty
Message-ID<s1n0S-7W5-19@gated-at.bofh.it>
In reply to#1453239
On Mon, Aug 01, 2016 at 04:12:28PM +0200, Michal Hocko wrote:
> From: Michal Hocko <mhocko@suse.com>
> Date: Mon, 1 Aug 2016 10:42:06 +0200
> Subject: [PATCH] memcg: put soft limit reclaim out of way if the excess tree
>  is empty
> 
> We've had a report about soft lockups caused by lock bouncing in the
> soft reclaim path:
> 
> [331404.849734] BUG: soft lockup - CPU#0 stuck for 22s! [kav4proxy-kavic:3128]
> [331404.849920] RIP: 0010:[<ffffffff81469798>]  [<ffffffff81469798>] _raw_spin_lock+0x18/0x20
> [331404.849997] Call Trace:
> [331404.850010]  [<ffffffff811557ea>] mem_cgroup_soft_limit_reclaim+0x25a/0x280
> [331404.850020]  [<ffffffff8111041d>] shrink_zones+0xed/0x200
> [331404.850027]  [<ffffffff81111a94>] do_try_to_free_pages+0x74/0x320
> [331404.850034]  [<ffffffff81112072>] try_to_free_pages+0x112/0x180
> [331404.850042]  [<ffffffff81104a6f>] __alloc_pages_slowpath+0x3ff/0x820
> [331404.850049]  [<ffffffff81105079>] __alloc_pages_nodemask+0x1e9/0x200
> [331404.850056]  [<ffffffff81141e01>] alloc_pages_vma+0xe1/0x290
> [331404.850064]  [<ffffffff8112402f>] do_wp_page+0x19f/0x840
> [331404.850071]  [<ffffffff811257cd>] handle_pte_fault+0x1cd/0x230
> [331404.850079]  [<ffffffff8146d3ed>] do_page_fault+0x1fd/0x4c0
> [331404.850087]  [<ffffffff81469ec5>] page_fault+0x25/0x30
> 
> There are no memcgs created so there cannot be any in the soft limit
> excess obviously:
> [...]
> memory  0       1       1
> 
> so all this just seems to be mem_cgroup_largest_soft_limit_node
> trying to get spin_lock_irq(&mctz->lock) just to find out that the soft
> limit excess tree is empty. This is just pointless waisting of cycles
> and cache line bouncing during heavy parallel reclaim on large machines.
> The particular machine wasn't very healthy and most probably suffering
> from a memory leak which just caused the memory reclaim to trash
> heavily. But bouncing on the lock certainly didn't help...
> 
> Introduce soft_limit_tree_empty which does the optimistic lockless check
> and bail out early if the tree is empty. This is theoretically racy but
> that shouldn't matter all that much. First of all soft limit is a best
> effort feature and it is slowly getting deprecated and its usage should
> be really scarce. Bouncing on a lock without a good reason is surely
> much bigger problem, especially on large CPU machines.
> 
> Signed-off-by: Michal Hocko <mhocko@suse.com>
> ---
>  mm/memcontrol.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index c265212bec8c..c0b57b6a194e 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -2543,6 +2543,11 @@ static int mem_cgroup_resize_memsw_limit(struct mem_cgroup *memcg,
>  	return ret;
>  }
>  
> +static inline bool soft_limit_tree_empty(struct mem_cgroup_tree_per_node *mctz)
> +{
> +	return RB_EMPTY_ROOT(&mctz->rb_root);
> +}

Can you please fold this into the caller? It should be obvious enough.

Other than that, this patch makes sense to me.

Acked-by: Johannes Weiner <hannes@cmpxchg.org>

[toc] | [prev] | [next] | [standalone]

#1453281 — Re: [PATCH] memcg: put soft limit reclaim out of way if the excess tree is empty

FromMichal Hocko <mhocko@kernel.org>
Date2016-08-01 17:40 +0200
SubjectRe: [PATCH] memcg: put soft limit reclaim out of way if the excess tree is empty
Message-ID<s1ntT-88a-7@gated-at.bofh.it>
In reply to#1453269
On Mon 01-08-16 11:03:43, Johannes Weiner wrote:
> On Mon, Aug 01, 2016 at 04:12:28PM +0200, Michal Hocko wrote:
[...]
> > diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> > index c265212bec8c..c0b57b6a194e 100644
> > --- a/mm/memcontrol.c
> > +++ b/mm/memcontrol.c
> > @@ -2543,6 +2543,11 @@ static int mem_cgroup_resize_memsw_limit(struct mem_cgroup *memcg,
> >  	return ret;
> >  }
> >  
> > +static inline bool soft_limit_tree_empty(struct mem_cgroup_tree_per_node *mctz)
> > +{
> > +	return RB_EMPTY_ROOT(&mctz->rb_root);
> > +}
> 
> Can you please fold this into the caller? It should be obvious enough.

OK, fair enough. There will probably be no other callers. I've added
comment as well

> Other than that, this patch makes sense to me.
> 
> Acked-by: Johannes Weiner <hannes@cmpxchg.org>

Thanks!

If the following sounds good I will resend v2.
---
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index c0b57b6a194e..e56d6a0f92ac 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -2543,11 +2543,6 @@ static int mem_cgroup_resize_memsw_limit(struct mem_cgroup *memcg,
 	return ret;
 }
 
-static inline bool soft_limit_tree_empty(struct mem_cgroup_tree_per_node *mctz)
-{
-	return RB_EMPTY_ROOT(&mctz->rb_root);
-}
-
 unsigned long mem_cgroup_soft_limit_reclaim(pg_data_t *pgdat, int order,
 					    gfp_t gfp_mask,
 					    unsigned long *total_scanned)
@@ -2564,7 +2559,13 @@ unsigned long mem_cgroup_soft_limit_reclaim(pg_data_t *pgdat, int order,
 		return 0;
 
 	mctz = soft_limit_tree_node(pgdat->node_id);
-	if (soft_limit_tree_empty(mctz))
+
+	/*
+	 * Do not even bother to check the largest node if the node
+	 * is empty. Do it lockless to prevent lock bouncing. Races
+	 * are acceptable as soft limit is best effort anyway.
+	 */
+	if (RB_EMPTY_ROOT(&mctz->rb_root))
 		return 0;
 
 	/*

-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1453345 — Re: [PATCH] memcg: put soft limit reclaim out of way if the excess tree is empty

FromJohannes Weiner <hannes@cmpxchg.org>
Date2016-08-01 19:20 +0200
SubjectRe: [PATCH] memcg: put soft limit reclaim out of way if the excess tree is empty
Message-ID<s1p2G-MQ-31@gated-at.bofh.it>
In reply to#1453281
On Mon, Aug 01, 2016 at 05:24:54PM +0200, Michal Hocko wrote:
> @@ -2564,7 +2559,13 @@ unsigned long mem_cgroup_soft_limit_reclaim(pg_data_t *pgdat, int order,
>  		return 0;
>  
>  	mctz = soft_limit_tree_node(pgdat->node_id);
> -	if (soft_limit_tree_empty(mctz))
> +
> +	/*
> +	 * Do not even bother to check the largest node if the node

                                                               root

> +	 * is empty. Do it lockless to prevent lock bouncing. Races
> +	 * are acceptable as soft limit is best effort anyway.
> +	 */
> +	if (RB_EMPTY_ROOT(&mctz->rb_root))
>  		return 0;

Other than that, looks good. Please retain my

Acked-by: Johannes Weiner <hannes@cmpxchg.org>

in version 2.

[toc] | [prev] | [next] | [standalone]

#1453373 — Re: [PATCH] memcg: put soft limit reclaim out of way if the excess tree is empty

FromMichal Hocko <mhocko@kernel.org>
Date2016-08-01 19:50 +0200
SubjectRe: [PATCH] memcg: put soft limit reclaim out of way if the excess tree is empty
Message-ID<s1pvI-YH-27@gated-at.bofh.it>
In reply to#1453345
On Mon 01-08-16 13:17:17, Johannes Weiner wrote:
> On Mon, Aug 01, 2016 at 05:24:54PM +0200, Michal Hocko wrote:
> > @@ -2564,7 +2559,13 @@ unsigned long mem_cgroup_soft_limit_reclaim(pg_data_t *pgdat, int order,
> >  		return 0;
> >  
> >  	mctz = soft_limit_tree_node(pgdat->node_id);
> > -	if (soft_limit_tree_empty(mctz))
> > +
> > +	/*
> > +	 * Do not even bother to check the largest node if the node
> 
>                                                                root

Fixed

> 
> > +	 * is empty. Do it lockless to prevent lock bouncing. Races
> > +	 * are acceptable as soft limit is best effort anyway.
> > +	 */
> > +	if (RB_EMPTY_ROOT(&mctz->rb_root))
> >  		return 0;
> 
> Other than that, looks good. Please retain my
> 
> Acked-by: Johannes Weiner <hannes@cmpxchg.org>

Thanks.

-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1453253 — Re: [PATCH] memcg: put soft limit reclaim out of way if the excess tree is empty

FromVladimir Davydov <vdavydov@virtuozzo.com>
Date2016-08-01 16:40 +0200
SubjectRe: [PATCH] memcg: put soft limit reclaim out of way if the excess tree is empty
Message-ID<s1met-7pd-23@gated-at.bofh.it>
In reply to#1453100
On Mon, Aug 01, 2016 at 12:00:21PM +0200, Michal Hocko wrote:
...
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index c265212bec8c..eb7e39c2d948 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -2543,6 +2543,11 @@ static int mem_cgroup_resize_memsw_limit(struct mem_cgroup *memcg,
>  	return ret;
>  }
>  
> +static inline bool soft_limit_tree_empty(struct mem_cgroup_tree_per_node *mctz)
> +{
> +	return rb_last(&mctz->rb_root) == NULL;
> +}
> +

I don't think traversing rb tree as rb_last() does w/o holding the lock
is a good idea. Why is RB_EMPTY_ROOT() insufficient here?

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web