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

Groups > linux.kernel > #1620050 > unrolled thread

[PATCH 3.12 127/142] cgroup: use an ordered workqueue for cgroup destruction

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 3.12 127/142] cgroup: use an ordered workqueue for cgroup destruction Jiri Slaby <jslaby@suse.cz> - 2017-04-10 17:40 +0200
    Re: [PATCH 3.12 127/142] cgroup: use an ordered workqueue for cgroup  destruction Hugh Dickins <hughd@google.com> - 2017-04-10 21:40 +0200
      Re: [PATCH 3.12 127/142] cgroup: use an ordered workqueue for cgroup  destruction Jiri Slaby <jslaby@suse.cz> - 2017-04-11 08:10 +0200

#1620050 — [PATCH 3.12 127/142] cgroup: use an ordered workqueue for cgroup destruction

From: Hugh Dickins <hughd@google.com>

3.12-stable review patch.  If anyone has any objections, please let me know.

===============

commit ab3f5faa6255a0eb4f832675507d9e295ca7e9ba upstream.

Sometimes the cleanup after memcg hierarchy testing gets stuck in
mem_cgroup_reparent_charges(), unable to bring non-kmem usage down to 0.

There may turn out to be several causes, but a major cause is this: the
workitem to offline parent can get run before workitem to offline child;
parent's mem_cgroup_reparent_charges() circles around waiting for the
child's pages to be reparented to its lrus, but it's holding cgroup_mutex
which prevents the child from reaching its mem_cgroup_reparent_charges().

Just use an ordered workqueue for cgroup_destroy_wq.

tj: Committing as the temporary fix until the reverse dependency can
    be removed from memcg.  Comment updated accordingly.

Fixes: e5fca243abae ("cgroup: use a dedicated workqueue for cgroup destruction")
Suggested-by: Filipe Brandenburger <filbranden@google.com>
Signed-off-by: Hugh Dickins <hughd@google.com>
Signed-off-by: Tejun Heo <tj@kernel.org>
Signed-off-by: Jiri Slaby <jslaby@suse.cz>
---
 kernel/cgroup.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index 5d9d542c0bb5..e89f6cec01c9 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -5168,12 +5168,16 @@ static int __init cgroup_wq_init(void)
 	/*
 	 * There isn't much point in executing destruction path in
 	 * parallel.  Good chunk is serialized with cgroup_mutex anyway.
-	 * Use 1 for @max_active.
+	 *
+	 * XXX: Must be ordered to make sure parent is offlined after
+	 * children.  The ordering requirement is for memcg where a
+	 * parent's offline may wait for a child's leading to deadlock.  In
+	 * the long term, this should be fixed from memcg side.
 	 *
 	 * We would prefer to do this in cgroup_init() above, but that
 	 * is called before init_workqueues(): so leave this until after.
 	 */
-	cgroup_destroy_wq = alloc_workqueue("cgroup_destroy", 0, 1);
+	cgroup_destroy_wq = alloc_ordered_workqueue("cgroup_destroy", 0);
 	BUG_ON(!cgroup_destroy_wq);
 	return 0;
 }
-- 
2.12.2

[toc] | [next] | [standalone]

#1620645 — Re: [PATCH 3.12 127/142] cgroup: use an ordered workqueue for cgroup destruction

On Mon, 10 Apr 2017, Jiri Slaby wrote:

> From: Hugh Dickins <hughd@google.com>
> 
> 3.12-stable review patch.  If anyone has any objections, please let me know.
> 
> ===============
> 
> commit ab3f5faa6255a0eb4f832675507d9e295ca7e9ba upstream.
> 
> Sometimes the cleanup after memcg hierarchy testing gets stuck in
> mem_cgroup_reparent_charges(), unable to bring non-kmem usage down to 0.
> 
> There may turn out to be several causes, but a major cause is this: the
> workitem to offline parent can get run before workitem to offline child;
> parent's mem_cgroup_reparent_charges() circles around waiting for the
> child's pages to be reparented to its lrus, but it's holding cgroup_mutex
> which prevents the child from reaching its mem_cgroup_reparent_charges().
> 
> Just use an ordered workqueue for cgroup_destroy_wq.
> 
> tj: Committing as the temporary fix until the reverse dependency can
>     be removed from memcg.  Comment updated accordingly.
> 
> Fixes: e5fca243abae ("cgroup: use a dedicated workqueue for cgroup destruction")
> Suggested-by: Filipe Brandenburger <filbranden@google.com>
> Signed-off-by: Hugh Dickins <hughd@google.com>
> Signed-off-by: Tejun Heo <tj@kernel.org>
> Signed-off-by: Jiri Slaby <jslaby@suse.cz>

Deja vu, it won't lie down!  See your mailbox of 2016-11-25..28:

No, please drop this one.  It was indeed marked for stable at the time,
but then reverted by 1a11533fbd71792e8c5d36f6763fbce8df0d231d; and you
already have in 3.12-stable the commit which in the end we used to fix
the issue, 4fb1a86fb5e4209a7d4426d4e586c58e9edc74ac
"memcg: reparent charges of children before processing parent".

Hugh

> ---
>  kernel/cgroup.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/cgroup.c b/kernel/cgroup.c
> index 5d9d542c0bb5..e89f6cec01c9 100644
> --- a/kernel/cgroup.c
> +++ b/kernel/cgroup.c
> @@ -5168,12 +5168,16 @@ static int __init cgroup_wq_init(void)
>  	/*
>  	 * There isn't much point in executing destruction path in
>  	 * parallel.  Good chunk is serialized with cgroup_mutex anyway.
> -	 * Use 1 for @max_active.
> +	 *
> +	 * XXX: Must be ordered to make sure parent is offlined after
> +	 * children.  The ordering requirement is for memcg where a
> +	 * parent's offline may wait for a child's leading to deadlock.  In
> +	 * the long term, this should be fixed from memcg side.
>  	 *
>  	 * We would prefer to do this in cgroup_init() above, but that
>  	 * is called before init_workqueues(): so leave this until after.
>  	 */
> -	cgroup_destroy_wq = alloc_workqueue("cgroup_destroy", 0, 1);
> +	cgroup_destroy_wq = alloc_ordered_workqueue("cgroup_destroy", 0);
>  	BUG_ON(!cgroup_destroy_wq);
>  	return 0;
>  }
> -- 
> 2.12.2
> 
> 

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

#1620937 — Re: [PATCH 3.12 127/142] cgroup: use an ordered workqueue for cgroup destruction

On 04/10/2017, 09:30 PM, Hugh Dickins wrote:
> On Mon, 10 Apr 2017, Jiri Slaby wrote:
> 
>> From: Hugh Dickins <hughd@google.com>
>>
>> 3.12-stable review patch.  If anyone has any objections, please let me know.
>>
>> ===============
>>
>> commit ab3f5faa6255a0eb4f832675507d9e295ca7e9ba upstream.
>>
>> Sometimes the cleanup after memcg hierarchy testing gets stuck in
>> mem_cgroup_reparent_charges(), unable to bring non-kmem usage down to 0.
>>
>> There may turn out to be several causes, but a major cause is this: the
>> workitem to offline parent can get run before workitem to offline child;
>> parent's mem_cgroup_reparent_charges() circles around waiting for the
>> child's pages to be reparented to its lrus, but it's holding cgroup_mutex
>> which prevents the child from reaching its mem_cgroup_reparent_charges().
>>
>> Just use an ordered workqueue for cgroup_destroy_wq.
>>
>> tj: Committing as the temporary fix until the reverse dependency can
>>     be removed from memcg.  Comment updated accordingly.
>>
>> Fixes: e5fca243abae ("cgroup: use a dedicated workqueue for cgroup destruction")
>> Suggested-by: Filipe Brandenburger <filbranden@google.com>
>> Signed-off-by: Hugh Dickins <hughd@google.com>
>> Signed-off-by: Tejun Heo <tj@kernel.org>
>> Signed-off-by: Jiri Slaby <jslaby@suse.cz>
> 
> Deja vu, it won't lie down!  See your mailbox of 2016-11-25..28:
> 
> No, please drop this one.  It was indeed marked for stable at the time,
> but then reverted by 1a11533fbd71792e8c5d36f6763fbce8df0d231d; and you
> already have in 3.12-stable the commit which in the end we used to fix
> the issue, 4fb1a86fb5e4209a7d4426d4e586c58e9edc74ac
> "memcg: reparent charges of children before processing parent".

Hmm, right. If only people stopped asking me to pick it. Dropped and
blacklisted. Thanks!

-- 
js
suse labs

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web