bdi: Move cgroup bdi_writeback to a dedicated low concurrency workqueue

PATCH Applied Tejun Heo 5 participants 10 messages May 23, 2018 – May 24, 2018

Subsystems (2)

Memory management MM misc

lore

Share & tools

lore

Tejun Heo

author

2018-05-23 17:56 UTC #0 lore raw

PATCH 1/1 bdi: Move cgroup bdi_writeback to a dedicated low concurrency workqueue

Quoted 1 line

From 0aa2e9b921d6db71150633ff290199554f0842a8 Mon Sep 17 00:00:00 2001

From: Tejun Heo <[email protected]>
Date: Wed, 23 May 2018 10:29:00 -0700

cgwb_release() punts the actual release to cgwb_release_workfn() on system_wq. Depending on the number of cgroups or block devices, there can be a lot of cgwb_release_workfn() in flight at the same time.

We're periodically seeing close to 256 kworkers getting stuck with the following stack trace and overtime the entire system gets stuck.

  [<ffffffff810ee40c>] _synchronize_rcu_expedited.constprop.72+0x2fc/0x330
  [<ffffffff810ee634>] synchronize_rcu_expedited+0x24/0x30
  [<ffffffff811ccf23>] bdi_unregister+0x53/0x290
  [<ffffffff811cd1e9>] release_bdi+0x89/0xc0
  [<ffffffff811cd645>] wb_exit+0x85/0xa0
  [<ffffffff811cdc84>] cgwb_release_workfn+0x54/0xb0
  [<ffffffff810a68d0>] process_one_work+0x150/0x410
  [<ffffffff810a71fd>] worker_thread+0x6d/0x520
  [<ffffffff810ad3dc>] kthread+0x12c/0x160
  [<ffffffff81969019>] ret_from_fork+0x29/0x40
  [<ffffffffffffffff>] 0xffffffffffffffff

The events leading to the lockup are...

1. A lot of cgwb_release_workfn() is queued at the same time and all system_wq kworkers are assigned to execute them.

2. They all end up calling synchronize_rcu_expedited(). One of them wins and tries to perform the expedited synchronization.

3. However, that invovles queueing rcu_exp_work to system_wq and waiting for it. Because #1 is holding all available kworkers on system_wq, rcu_exp_work can't be executed. cgwb_release_workfn() is waiting for synchronize_rcu_expedited() which in turn is waiting for cgwb_release_workfn() to free up some of the kworkers.

We shouldn't be scheduling hundreds of cgwb_release_workfn() at the same time. There's nothing to be gained from that. This patch updates cgwb release path to use a dedicated percpu workqueue with @max_active of 1.

While this resolves the problem at hand, it might be a good idea to isolate rcu_exp_work to its own workqueue too as it can be used from various paths and is prone to this sort of indirect A-A deadlocks.

Signed-off-by Tejun Heo <[email protected]>

Cc "Paul E. McKenney" <[email protected]>

Cc [email protected]

---
 mm/backing-dev.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

mm/backing-dev.c +17 -1

index 7441bd9..8fe3ebd 100644
@@ -412,6 +412,7 @@ static void wb_exit(struct bdi_writeback *wb)
  * protected.
  */
 static DEFINE_SPINLOCK(cgwb_lock);
+static struct workqueue_struct *cgwb_release_wq;
 
 /**
  * wb_congested_get_create - get or create a wb_congested

@@ -522,7 +523,7 @@ static void cgwb_release(struct percpu_ref *refcnt)
 {
 	struct bdi_writeback *wb = container_of(refcnt, struct bdi_writeback,
 						refcnt);
-	schedule_work(&wb->release_work);
+	queue_work(cgwb_release_wq, &wb->release_work);
 }
 
 static void cgwb_kill(struct bdi_writeback *wb)

@@ -784,6 +785,21 @@ static void cgwb_bdi_register(struct backing_dev_info *bdi)
 	spin_unlock_irq(&cgwb_lock);
 }
 
+static int __init cgwb_init(void)
+{
+	/*
+	 * There can be many concurrent release work items overwhelming
+	 * system_wq.  Put them in a separate wq and limit concurrency.
+	 * There's no point in executing many of these in parallel.
+	 */
+	cgwb_release_wq = alloc_workqueue("cgwb_release", 0, 1);
+	if (!cgwb_release_wq)
+		return -ENOMEM;
+
+	return 0;
+}
+subsys_initcall(cgwb_init);
+
 #else	/* CONFIG_CGROUP_WRITEBACK */
 
 static int cgwb_bdi_init(struct backing_dev_info *bdi)

Signature · 1 line

2.9.5

Paul E. McKenney

2018-05-23 18:48 UTC #1 lore raw

Quoted 37 lines · Tejun Heo

Quoted 1 line
From 0aa2e9b921d6db71150633ff290199554f0842a8 Mon Sep 17 00:00:00 2001
From: Tejun Heo <[email protected]>
Date: Wed, 23 May 2018 10:29:00 -0700
cgwb_release() punts the actual release to cgwb_release_workfn() on system_wq. Depending on the number of cgroups or block devices, there can be a lot of cgwb_release_workfn() in flight at the same time.
We're periodically seeing close to 256 kworkers getting stuck with the following stack trace and overtime the entire system gets stuck.
  [<ffffffff810ee40c>] _synchronize_rcu_expedited.constprop.72+0x2fc/0x330
  [<ffffffff810ee634>] synchronize_rcu_expedited+0x24/0x30
  [<ffffffff811ccf23>] bdi_unregister+0x53/0x290
  [<ffffffff811cd1e9>] release_bdi+0x89/0xc0
  [<ffffffff811cd645>] wb_exit+0x85/0xa0
  [<ffffffff811cdc84>] cgwb_release_workfn+0x54/0xb0
  [<ffffffff810a68d0>] process_one_work+0x150/0x410
  [<ffffffff810a71fd>] worker_thread+0x6d/0x520
  [<ffffffff810ad3dc>] kthread+0x12c/0x160
  [<ffffffff81969019>] ret_from_fork+0x29/0x40
  [<ffffffffffffffff>] 0xffffffffffffffff
The events leading to the lockup are...
1. A lot of cgwb_release_workfn() is queued at the same time and all system_wq kworkers are assigned to execute them.
2. They all end up calling synchronize_rcu_expedited(). One of them wins and tries to perform the expedited synchronization.
3. However, that invovles queueing rcu_exp_work to system_wq and waiting for it. Because #1 is holding all available kworkers on system_wq, rcu_exp_work can't be executed. cgwb_release_workfn() is waiting for synchronize_rcu_expedited() which in turn is waiting for cgwb_release_workfn() to free up some of the kworkers.

Quoted 8 lines

We shouldn't be scheduling hundreds of cgwb_release_workfn() at the same time. There's nothing to be gained from that. This patch updates cgwb release path to use a dedicated percpu workqueue with @max_active of 1.
While this resolves the problem at hand, it might be a good idea to isolate rcu_exp_work to its own workqueue too as it can be used from various paths and is prone to this sort of indirect A-A deadlocks.

Commit ad7c946b35ad4 ("rcu: Create RCU-specific workqueues with rescuers") was accepted into mainline this past merge window. Does that do what you want, or are you looking for something else?

Thanx, Paul

Tejun Heo

author

2018-05-23 18:51 UTC #2 lore raw

Quoted 7 lines · Paul E. McKenney

Quoted 3 lines
While this resolves the problem at hand, it might be a good idea to isolate rcu_exp_work to its own workqueue too as it can be used from various paths and is prone to this sort of indirect A-A deadlocks.
Commit ad7c946b35ad4 ("rcu: Create RCU-specific workqueues with rescuers") was accepted into mainline this past merge window. Does that do what you want, or are you looking for something else?

Ah, that does it. Sorry, was looking at an older kernel.

Thanks.

Signature · 1 line

tejun

Paul E. McKenney

2018-05-23 19:20 UTC #3 lore raw

Quoted 10 lines · Tejun Heo

Quoted 7 lines · Paul E. McKenney
Quoted 3 lines
While this resolves the problem at hand, it might be a good idea to isolate rcu_exp_work to its own workqueue too as it can be used from various paths and is prone to this sort of indirect A-A deadlocks.
Commit ad7c946b35ad4 ("rcu: Create RCU-specific workqueues with rescuers") was accepted into mainline this past merge window. Does that do what you want, or are you looking for something else?
Ah, that does it. Sorry, was looking at an older kernel.

No worries -- after all, I must confess that I was a bit slow in getting to this.

Thanx, Paul

Jens Axboe

Applied

2018-05-23 21:29 UTC #4 lore raw

In reply to Tejun Heo

Quoted 37 lines · Tejun Heo

From 0aa2e9b921d6db71150633ff290199554f0842a8 Mon Sep 17 00:00:00 2001
From: Tejun Heo <[email protected]>
Date: Wed, 23 May 2018 10:29:00 -0700
cgwb_release() punts the actual release to cgwb_release_workfn() on system_wq. Depending on the number of cgroups or block devices, there can be a lot of cgwb_release_workfn() in flight at the same time.
We're periodically seeing close to 256 kworkers getting stuck with the following stack trace and overtime the entire system gets stuck.
  [<ffffffff810ee40c>] _synchronize_rcu_expedited.constprop.72+0x2fc/0x330
  [<ffffffff810ee634>] synchronize_rcu_expedited+0x24/0x30
  [<ffffffff811ccf23>] bdi_unregister+0x53/0x290
  [<ffffffff811cd1e9>] release_bdi+0x89/0xc0
  [<ffffffff811cd645>] wb_exit+0x85/0xa0
  [<ffffffff811cdc84>] cgwb_release_workfn+0x54/0xb0
  [<ffffffff810a68d0>] process_one_work+0x150/0x410
  [<ffffffff810a71fd>] worker_thread+0x6d/0x520
  [<ffffffff810ad3dc>] kthread+0x12c/0x160
  [<ffffffff81969019>] ret_from_fork+0x29/0x40
  [<ffffffffffffffff>] 0xffffffffffffffff
The events leading to the lockup are...
1. A lot of cgwb_release_workfn() is queued at the same time and all system_wq kworkers are assigned to execute them.
2. They all end up calling synchronize_rcu_expedited(). One of them wins and tries to perform the expedited synchronization.
3. However, that invovles queueing rcu_exp_work to system_wq and waiting for it. Because #1 is holding all available kworkers on system_wq, rcu_exp_work can't be executed. cgwb_release_workfn() is waiting for synchronize_rcu_expedited() which in turn is waiting for cgwb_release_workfn() to free up some of the kworkers.

Quoted 8 lines

We shouldn't be scheduling hundreds of cgwb_release_workfn() at the same time. There's nothing to be gained from that. This patch updates cgwb release path to use a dedicated percpu workqueue with @max_active of 1.
While this resolves the problem at hand, it might be a good idea to isolate rcu_exp_work to its own workqueue too as it can be used from various paths and is prone to this sort of indirect A-A deadlocks.

Applied, thanks.

Signature · 1 line

Jens Axboe

Rik van Riel

2018-05-23 22:03 UTC #5 lore raw

In reply to Tejun Heo

Quoted 15 lines · Tejun Heo

The events leading to the lockup are...
1. A lot of cgwb_release_workfn() is queued at the same time and all system_wq kworkers are assigned to execute them.
2. They all end up calling synchronize_rcu_expedited(). One of them wins and tries to perform the expedited synchronization.
3. However, that invovles queueing rcu_exp_work to system_wq and waiting for it. Because #1 is holding all available kworkers on system_wq, rcu_exp_work can't be executed. cgwb_release_workfn() is waiting for synchronize_rcu_expedited() which in turn is
waiting
for cgwb_release_workfn() to free up some of the kworkers.

Quoted 4 lines

We shouldn't be scheduling hundreds of cgwb_release_workfn() at the same time. There's nothing to be gained from that. This patch updates cgwb release path to use a dedicated percpu workqueue with @max_active of 1.

Dumb question. Does setting max_active to 1 mean that every cgwb_release_workfn() ends up forcing another RCU grace period on the whole system, while today you might have a bunch of them waiting on the same RCU grace period advance?

Would it be faster to have some number (up to 16?) push RCU once, at the same time, instead of having each of them push RCU into a next grace period one after another?

I may be overlooking something fundamental here, but I thought I'd at least ask the question, just in case :)

Signature · 1 line

All Rights Reversed.

Tejun Heo

author

2018-05-23 23:17 UTC #6 lore raw

Hello, Rik.

Quoted 10 lines · Rik van Riel

Dumb question. Does setting max_active to 1 mean that every cgwb_release_workfn() ends up forcing another RCU grace period on the whole system, while today you might have a bunch of them waiting on the same RCU grace period advance?
Would it be faster to have some number (up to 16?) push RCU once, at the same time, instead of having each of them push RCU into a next grace period one after another?

Oh yeah, you're absolutely right. This would end up doing a lot of back-to-back synchronize_rcu_expedited() calls which can't be good. I'll send a patch to push it upto 16.

Thanks.

Signature · 1 line

tejun

Tejun Heo

author

2018-05-23 23:25 UTC #7 lore raw

PATCH 1/1 bdi: Increase the concurrecy level of cgwb_release_wq

In reply to Tejun Heo

cgwb_release_wq was added to isolate and limit the concurrency level of cgwb release work items. The max concurrency was set to 1 per CPU but we do want some level of concurrency as otherwise it ends up calling, through bdi_unregister(), synchronize_sched_expedited() back-to-back for each release work item.

Let's allow them to bunch up a bit.

Signed-off-by Tejun Heo <[email protected]>

---
 mm/backing-dev.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

mm/backing-dev.c +3 -2

index 8fe3ebd..f9a2268 100644
@@ -790,9 +790,10 @@ static int __init cgwb_init(void)
 	/*
 	 * There can be many concurrent release work items overwhelming
 	 * system_wq.  Put them in a separate wq and limit concurrency.
-	 * There's no point in executing many of these in parallel.
+	 * Allow some level of concurrency so that we can batch on
+	 * synchronize_rcu_expedited() calls in bdi_unregister().
 	 */
-	cgwb_release_wq = alloc_workqueue("cgwb_release", 0, 1);
+	cgwb_release_wq = alloc_workqueue("cgwb_release", 0, 16);
 	if (!cgwb_release_wq)
 		return -ENOMEM;

Jan Kara

Reviewed-by on v1 1/1

2018-05-24 10:19 UTC #8 lore raw

In reply to Tejun Heo

Quoted 10 lines · Tejun Heo

From 0aa2e9b921d6db71150633ff290199554f0842a8 Mon Sep 17 00:00:00 2001
From: Tejun Heo <[email protected]>
Date: Wed, 23 May 2018 10:29:00 -0700
cgwb_release() punts the actual release to cgwb_release_workfn() on system_wq. Depending on the number of cgroups or block devices, there can be a lot of cgwb_release_workfn() in flight at the same time.
We're periodically seeing close to 256 kworkers getting stuck with the following stack trace and overtime the entire system gets stuck.

OK, but that means that you have to have 256 block devices, don't you? As we have a bdi per device and we call synchronize_rcu_expedited() only when unregistering bdi (and the corresponding request queue must be gone at that point as well as that's otherwise holding a reference). Am I understanding the situation correctly?

Quoted 26 lines

  [<ffffffff810ee40c>] _synchronize_rcu_expedited.constprop.72+0x2fc/0x330
  [<ffffffff810ee634>] synchronize_rcu_expedited+0x24/0x30
  [<ffffffff811ccf23>] bdi_unregister+0x53/0x290
  [<ffffffff811cd1e9>] release_bdi+0x89/0xc0
  [<ffffffff811cd645>] wb_exit+0x85/0xa0
  [<ffffffff811cdc84>] cgwb_release_workfn+0x54/0xb0
  [<ffffffff810a68d0>] process_one_work+0x150/0x410
  [<ffffffff810a71fd>] worker_thread+0x6d/0x520
  [<ffffffff810ad3dc>] kthread+0x12c/0x160
  [<ffffffff81969019>] ret_from_fork+0x29/0x40
  [<ffffffffffffffff>] 0xffffffffffffffff
The events leading to the lockup are...
1. A lot of cgwb_release_workfn() is queued at the same time and all system_wq kworkers are assigned to execute them.
2. They all end up calling synchronize_rcu_expedited(). One of them wins and tries to perform the expedited synchronization.
3. However, that invovles queueing rcu_exp_work to system_wq and waiting for it. Because #1 is holding all available kworkers on system_wq, rcu_exp_work can't be executed. cgwb_release_workfn() is waiting for synchronize_rcu_expedited() which in turn is waiting for cgwb_release_workfn() to free up some of the kworkers.

Quoted 4 lines

We shouldn't be scheduling hundreds of cgwb_release_workfn() at the same time. There's nothing to be gained from that. This patch updates cgwb release path to use a dedicated percpu workqueue with @max_active of 1.

As Rik wrote, some paralelism is good to reduce number of forced grace periods so raising this to 16 is good. I was thinking whether we could not batch rcu grace periods in some explicit way but that would be difficult to do.

But thinking a bit more about this, if we made bdi RCU freed, we could just avoid the synchronize_rcu_expedited() in bdi_remove_from_list() altogether. The uses of bdi list are pretty limited and everybody ends up testing WB_registered bit before doing anything anyway... What do you think?

Other than that you can add:

Reviewed-by Jan Kara <[email protected]>

to this patch when updated to increase concurrency as that's a good short term solution (for stable kernels) anyway.

Honza

Quoted 61 lines

Signed-off-by Tejun Heo <[email protected]>

Cc "Paul E. McKenney" <[email protected]>

Cc [email protected]

---
 mm/backing-dev.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

mm/backing-dev.c +17 -1

index 7441bd9..8fe3ebd 100644
@@ -412,6 +412,7 @@ static void wb_exit(struct bdi_writeback *wb)
  * protected.
  */
 static DEFINE_SPINLOCK(cgwb_lock);
+static struct workqueue_struct *cgwb_release_wq;
 
 /**
  * wb_congested_get_create - get or create a wb_congested

@@ -522,7 +523,7 @@ static void cgwb_release(struct percpu_ref *refcnt)
 {
 	struct bdi_writeback *wb = container_of(refcnt, struct bdi_writeback,
 						refcnt);
-	schedule_work(&wb->release_work);
+	queue_work(cgwb_release_wq, &wb->release_work);
 }
 
 static void cgwb_kill(struct bdi_writeback *wb)

@@ -784,6 +785,21 @@ static void cgwb_bdi_register(struct backing_dev_info *bdi)
 	spin_unlock_irq(&cgwb_lock);
 }
 
+static int __init cgwb_init(void)
+{
+	/*
+	 * There can be many concurrent release work items overwhelming
+	 * system_wq.  Put them in a separate wq and limit concurrency.
+	 * There's no point in executing many of these in parallel.
+	 */
+	cgwb_release_wq = alloc_workqueue("cgwb_release", 0, 1);
+	if (!cgwb_release_wq)
+		return -ENOMEM;
+
+	return 0;
+}
+subsys_initcall(cgwb_init);
+
 #else	/* CONFIG_CGROUP_WRITEBACK */
 
 static int cgwb_bdi_init(struct backing_dev_info *bdi)

Signature · 1 line

2.9.5

Signature · 2 lines

Jan Kara <[email protected]>
SUSE Labs, CR

Tejun Heo

author

2018-05-24 14:00 UTC #9 lore raw

Hello, Jan.

Quoted 4 lines · Jan Kara

Quoted 2 lines
We're periodically seeing close to 256 kworkers getting stuck with the following stack trace and overtime the entire system gets stuck.
OK, but that means that you have to have 256 block devices, don't you? As

It'd be a product of the number of cgroups being deleted at the same time and the number of block devices they were accessing. With multiple loop devices and multiple cgroup removals, it doesn't seem too difficult to hit. It's rare but we do see it happening repeatedly in one of the setups.

Quoted 4 lines

we have a bdi per device and we call synchronize_rcu_expedited() only when unregistering bdi (and the corresponding request queue must be gone at that point as well as that's otherwise holding a reference). Am I understanding the situation correctly?

Yeah, I think so.

Quoted 9 lines

Quoted 4 lines
We shouldn't be scheduling hundreds of cgwb_release_workfn() at the same time. There's nothing to be gained from that. This patch updates cgwb release path to use a dedicated percpu workqueue with @max_active of 1.
As Rik wrote, some paralelism is good to reduce number of forced grace periods so raising this to 16 is good. I was thinking whether we could not batch rcu grace periods in some explicit way but that would be difficult to do.

Agreed, already sent a patch to increase @max_active to 16.

Quoted 8 lines

But thinking a bit more about this, if we made bdi RCU freed, we could just avoid the synchronize_rcu_expedited() in bdi_remove_from_list() altogether. The uses of bdi list are pretty limited and everybody ends up testing WB_registered bit before doing anything anyway... What do you think?
Other than that you can add:
Reviewed-by Jan Kara <[email protected]>

Thanks.

Signature · 1 line

tejun