[PATCH] cgroup: avoid flushing global workqueue in cgroup1_pidlist_destroy_all

[PATCH] cgroup: avoid flushing global workqueue in cgroup1_pidlist_destroy_all

[Junnan Zhang]

From: Junnan Zhang <zhangjn11@chinatelecom.cn>

cgroup1_pidlist_destroy_all() flushes the global
cgroup_pidlist_destroy_wq while destroying a cgroup. Because all cgroup
v1 pidlist destruction work items are queued on the same shared workqueue,
a single slow or stuck work item (e.g. waiting for pidlist_mutex held by a
user-space reader) blocks every concurrent cgroup destruction path.

This can lead to kworker tasks stuck in flush_workqueue() for over
hung_task_timeout seconds, as observed on busy systems running Docker or
Kubernetes workloads.

  INFO: task kworker/0:1:1438499 blocked for more than 120 seconds.
  "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
  kworker/0:1 D 0 1438499 2 0x80000080
  Workqueue: cgroup_destroy css_free_rwork_fn
  ? __schedule+0x296/0x900
  schedule+0x28/0x80
  schedule_timeout+0x1ee/0x3a0
  ? kvm_sched_clock_read+0xd/0x20
  wait_for_completion+0x12c/0x190
  ? wake_up_q+0x70/0x70
  flush_workqueue+0x132/0x430
  ? cgroup1_pidlist_destroy_all+0x7c/0xa0
  cgroup1_pidlist_destroy_all+0x7c/0xa0
  css_free_rwork_fn+0xb5/0x390
  process_one_work+0x195/0x3e0
  worker_thread+0x30/0x390
  ? process_one_work+0x3e0/0x3e0
  kthread+0x113/0x130
  ? kthread_create_worker_on_cpu+0x70/0x70
  ret_from_fork+0x1f/0x40

Fix it by moving the cgroup's pidlists to a local orphan list under
pidlist_mutex, clearing their ->owner pointer, and then cancelling each
pidlist's delayed work outside the lock. The destroy work function now
checks ->owner and skips freeing orphaned pidlists, so
cgroup1_pidlist_destroy_all() can free them safely without flushing the
whole shared workqueue.

Signed-off-by: Junnan Zhang <zhangjn11@chinatelecom.cn>
Signed-off-by: Shouxin Sun <sunshx@chinatelecom.cn>
---
 kernel/cgroup/cgroup-v1.c | 53 ++++++++++++++++++++++++++++-----------
 1 file changed, 38 insertions(+), 15 deletions(-)

diff --git a/kernel/cgroup/cgroup-v1.c b/kernel/cgroup/cgroup-v1.c
index a4337c9b5287..874fe4dc6ffd 100644
--- a/kernel/cgroup/cgroup-v1.c
+++ b/kernel/cgroup/cgroup-v1.c
@@ -206,13 +206,31 @@ struct cgroup_pidlist {
 void cgroup1_pidlist_destroy_all(struct cgroup *cgrp)
 {
 	struct cgroup_pidlist *l, *tmp_l;
+	LIST_HEAD(orphan);
+
+	/*
+	 * Move pidlists to a local orphan list and mark them as owner-less.
+	 * The destroy work function will see ->owner == NULL and skip freeing.
+	 * We then cancel and free them outside pidlist_mutex to avoid
+	 * flush_workqueue() blocking on the shared workqueue.
+	 */
 
 	mutex_lock(&cgrp->pidlist_mutex);
-	list_for_each_entry_safe(l, tmp_l, &cgrp->pidlists, links)
-		mod_delayed_work(cgroup_pidlist_destroy_wq, &l->destroy_dwork, 0);
+	list_for_each_entry_safe(l, tmp_l, &cgrp->pidlists, links) {
+		list_del(&l->links);
+		l->owner = NULL;
+		list_add(&l->links, &orphan);
+	}
 	mutex_unlock(&cgrp->pidlist_mutex);
 
-	flush_workqueue(cgroup_pidlist_destroy_wq);
+	list_for_each_entry_safe(l, tmp_l, &orphan, links) {
+		list_del(&l->links);
+		cancel_delayed_work_sync(&l->destroy_dwork);
+		kvfree(l->list);
+		put_pid_ns(l->key.ns);
+		kfree(l);
+	}
+
 	BUG_ON(!list_empty(&cgrp->pidlists));
 }
 
@@ -222,21 +240,26 @@ static void cgroup_pidlist_destroy_work_fn(struct work_struct *work)
 	struct cgroup_pidlist *l = container_of(dwork, struct cgroup_pidlist,
 						destroy_dwork);
 	struct cgroup_pidlist *tofree = NULL;
+	struct cgroup *owner;
 
-	mutex_lock(&l->owner->pidlist_mutex);
+	owner = l->owner;
+	if (owner) {
+		mutex_lock(&owner->pidlist_mutex);
 
-	/*
-	 * Destroy iff we didn't get queued again.  The state won't change
-	 * as destroy_dwork can only be queued while locked.
-	 */
-	if (!delayed_work_pending(dwork)) {
-		list_del(&l->links);
-		kvfree(l->list);
-		put_pid_ns(l->key.ns);
-		tofree = l;
-	}
+		/*
+		 * Destroy iff we didn't get queued again and we're still
+		 * owned by the cgroup.  If ->owner was cleared by
+		 * cgroup1_pidlist_destroy_all(), it will free us.
+		 */
+		if (l->owner == owner && !delayed_work_pending(dwork)) {
+			list_del(&l->links);
+			kvfree(l->list);
+			put_pid_ns(l->key.ns);
+			tofree = l;
+		}
 
-	mutex_unlock(&l->owner->pidlist_mutex);
+		mutex_unlock(&l->owner->pidlist_mutex);
+	}
 	kfree(tofree);
 }
 
-- 
2.43.0

[PATCH v2] cgroup: avoid flushing global workqueue in cgroup1_pidlist_destroy_all

[Junnan Zhang]

From: Junnan Zhang <zhangjn11@chinatelecom.cn>

cgroup1_pidlist_destroy_all() flushes the global
cgroup_pidlist_destroy_wq while destroying a cgroup. Because all cgroup
v1 pidlist destruction work items are queued on the same shared workqueue,
a single slow or stuck work item (e.g. waiting for pidlist_mutex held by a
user-space reader) blocks every concurrent cgroup destruction path.

This can lead to kworker tasks stuck in flush_workqueue() for over
hung_task_timeout seconds, as observed on busy systems running Docker or
Kubernetes workloads.

  INFO: task kworker/0:1:1438499 blocked for more than 120 seconds.
  "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
  kworker/0:1 D 0 1438499 2 0x80000080
  Workqueue: cgroup_destroy css_free_rwork_fn
  ? __schedule+0x296/0x900
  schedule+0x28/0x80
  schedule_timeout+0x1ee/0x3a0
  ? kvm_sched_clock_read+0xd/0x20
  wait_for_completion+0x12c/0x190
  ? wake_up_q+0x70/0x70
  flush_workqueue+0x132/0x430
  ? cgroup1_pidlist_destroy_all+0x7c/0xa0
  cgroup1_pidlist_destroy_all+0x7c/0xa0
  css_free_rwork_fn+0xb5/0x390
  process_one_work+0x195/0x3e0
  worker_thread+0x30/0x390
  ? process_one_work+0x3e0/0x3e0
  kthread+0x113/0x130
  ? kthread_create_worker_on_cpu+0x70/0x70
  ret_from_fork+0x1f/0x40

Fix it by moving the cgroup's pidlists to a local orphan list under
pidlist_mutex, clearing their ->owner pointer, and then cancelling each
pidlist's delayed work outside the lock. The destroy work function now
checks ->owner and skips freeing orphaned pidlists, so
cgroup1_pidlist_destroy_all() can free them safely without flushing the
whole shared workqueue.

Signed-off-by: Junnan Zhang <zhangjn11@chinatelecom.cn>
Signed-off-by: Shouxin Sun <sunshx@chinatelecom.cn>

---
v1 -> v2:
 - Fix a NULL pointer dereference in cgroup_pidlist_destroy_work_fn():
   use the cached owner pointer for mutex_unlock(), as ->owner may have
   been cleared concurrently by cgroup1_pidlist_destroy_all(). Spotted
   by Sashiko AI review.
---
 kernel/cgroup/cgroup-v1.c | 53 ++++++++++++++++++++++++++++-----------
 1 file changed, 38 insertions(+), 15 deletions(-)

diff --git a/kernel/cgroup/cgroup-v1.c b/kernel/cgroup/cgroup-v1.c
index a4337c9b5287..88b10cd59c7b 100644
--- a/kernel/cgroup/cgroup-v1.c
+++ b/kernel/cgroup/cgroup-v1.c
@@ -206,13 +206,31 @@ struct cgroup_pidlist {
 void cgroup1_pidlist_destroy_all(struct cgroup *cgrp)
 {
 	struct cgroup_pidlist *l, *tmp_l;
+	LIST_HEAD(orphan);
+
+	/*
+	 * Move pidlists to a local orphan list and mark them as owner-less.
+	 * The destroy work function will see ->owner == NULL and skip freeing.
+	 * We then cancel and free them outside pidlist_mutex to avoid
+	 * flush_workqueue() blocking on the shared workqueue.
+	 */
 
 	mutex_lock(&cgrp->pidlist_mutex);
-	list_for_each_entry_safe(l, tmp_l, &cgrp->pidlists, links)
-		mod_delayed_work(cgroup_pidlist_destroy_wq, &l->destroy_dwork, 0);
+	list_for_each_entry_safe(l, tmp_l, &cgrp->pidlists, links) {
+		list_del(&l->links);
+		l->owner = NULL;
+		list_add(&l->links, &orphan);
+	}
 	mutex_unlock(&cgrp->pidlist_mutex);
 
-	flush_workqueue(cgroup_pidlist_destroy_wq);
+	list_for_each_entry_safe(l, tmp_l, &orphan, links) {
+		list_del(&l->links);
+		cancel_delayed_work_sync(&l->destroy_dwork);
+		kvfree(l->list);
+		put_pid_ns(l->key.ns);
+		kfree(l);
+	}
+
 	BUG_ON(!list_empty(&cgrp->pidlists));
 }
 
@@ -222,21 +240,26 @@ static void cgroup_pidlist_destroy_work_fn(struct work_struct *work)
 	struct cgroup_pidlist *l = container_of(dwork, struct cgroup_pidlist,
 						destroy_dwork);
 	struct cgroup_pidlist *tofree = NULL;
+	struct cgroup *owner;
 
-	mutex_lock(&l->owner->pidlist_mutex);
+	owner = l->owner;
+	if (owner) {
+		mutex_lock(&owner->pidlist_mutex);
 
-	/*
-	 * Destroy iff we didn't get queued again.  The state won't change
-	 * as destroy_dwork can only be queued while locked.
-	 */
-	if (!delayed_work_pending(dwork)) {
-		list_del(&l->links);
-		kvfree(l->list);
-		put_pid_ns(l->key.ns);
-		tofree = l;
-	}
+		/*
+		 * Destroy iff we didn't get queued again and we're still
+		 * owned by the cgroup.  If ->owner was cleared by
+		 * cgroup1_pidlist_destroy_all(), it will free us.
+		 */
+		if (l->owner == owner && !delayed_work_pending(dwork)) {
+			list_del(&l->links);
+			kvfree(l->list);
+			put_pid_ns(l->key.ns);
+			tofree = l;
+		}
 
-	mutex_unlock(&l->owner->pidlist_mutex);
+		mutex_unlock(&owner->pidlist_mutex);
+	}
 	kfree(tofree);
 }
 
-- 
2.43.0

[PATCH v3] cgroup: avoid flushing global workqueue in cgroup1_pidlist_destroy_all

[Junnan Zhang]

From: Junnan Zhang <zhangjn11@chinatelecom.cn>

cgroup1_pidlist_destroy_all() flushes the global
cgroup_pidlist_destroy_wq while destroying a cgroup. Because all cgroup
v1 pidlist destruction work items are queued on the same shared workqueue,
a single slow or stuck work item (e.g. waiting for pidlist_mutex held by a
user-space reader) blocks every concurrent cgroup destruction path.

This can lead to kworker tasks stuck in flush_workqueue() for over
hung_task_timeout seconds, as observed on busy systems running Docker or
Kubernetes workloads.

  INFO: task kworker/0:1:1438499 blocked for more than 120 seconds.
  "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
  kworker/0:1 D 0 1438499 2 0x80000080
  Workqueue: cgroup_destroy css_free_rwork_fn
  ? __schedule+0x296/0x900
  schedule+0x28/0x80
  schedule_timeout+0x1ee/0x3a0
  ? kvm_sched_clock_read+0xd/0x20
  wait_for_completion+0x12c/0x190
  ? wake_up_q+0x70/0x70
  flush_workqueue+0x132/0x430
  ? cgroup1_pidlist_destroy_all+0x7c/0xa0
  cgroup1_pidlist_destroy_all+0x7c/0xa0
  css_free_rwork_fn+0xb5/0x390
  process_one_work+0x195/0x3e0
  worker_thread+0x30/0x390
  ? process_one_work+0x3e0/0x3e0
  kthread+0x113/0x130
  ? kthread_create_worker_on_cpu+0x70/0x70
  ret_from_fork+0x1f/0x40

Fix it by moving the cgroup's pidlists to a local orphan list under
pidlist_mutex, clearing their ->owner pointer, and then cancelling each
pidlist's delayed work outside the lock. The destroy work function now
checks ->owner and skips freeing orphaned pidlists, so
cgroup1_pidlist_destroy_all() can free them safely without flushing the
whole shared workqueue.

Signed-off-by: Junnan Zhang <zhangjn11@chinatelecom.cn>
Signed-off-by: Shouxin Sun <sunshx@chinatelecom.cn>

---
v1 -> v2:
 - Fix a NULL pointer dereference in cgroup_pidlist_destroy_work_fn():
   use the cached owner pointer for mutex_unlock(), as ->owner may have
   been cleared concurrently by cgroup1_pidlist_destroy_all(). Spotted
   by Sashiko AI review.

v2 -> v3:
 - Use READ_ONCE()/WRITE_ONCE() for the ->owner field shared between
   cgroup1_pidlist_destroy_all() and cgroup_pidlist_destroy_work_fn() to
   prevent data races. Spotted by Sashiko AI review.
---
 kernel/cgroup/cgroup-v1.c | 53 ++++++++++++++++++++++++++++-----------
 1 file changed, 38 insertions(+), 15 deletions(-)

diff --git a/kernel/cgroup/cgroup-v1.c b/kernel/cgroup/cgroup-v1.c
index a4337c9b5287..9abe8b791bb1 100644
--- a/kernel/cgroup/cgroup-v1.c
+++ b/kernel/cgroup/cgroup-v1.c
@@ -206,13 +206,31 @@ struct cgroup_pidlist {
 void cgroup1_pidlist_destroy_all(struct cgroup *cgrp)
 {
 	struct cgroup_pidlist *l, *tmp_l;
+	LIST_HEAD(orphan);
+
+	/*
+	 * Move pidlists to a local orphan list and mark them as owner-less.
+	 * The destroy work function will see ->owner == NULL and skip freeing.
+	 * We then cancel and free them outside pidlist_mutex to avoid
+	 * flush_workqueue() blocking on the shared workqueue.
+	 */
 
 	mutex_lock(&cgrp->pidlist_mutex);
-	list_for_each_entry_safe(l, tmp_l, &cgrp->pidlists, links)
-		mod_delayed_work(cgroup_pidlist_destroy_wq, &l->destroy_dwork, 0);
+	list_for_each_entry_safe(l, tmp_l, &cgrp->pidlists, links) {
+		list_del(&l->links);
+		WRITE_ONCE(l->owner, NULL);
+		list_add(&l->links, &orphan);
+	}
 	mutex_unlock(&cgrp->pidlist_mutex);
 
-	flush_workqueue(cgroup_pidlist_destroy_wq);
+	list_for_each_entry_safe(l, tmp_l, &orphan, links) {
+		list_del(&l->links);
+		cancel_delayed_work_sync(&l->destroy_dwork);
+		kvfree(l->list);
+		put_pid_ns(l->key.ns);
+		kfree(l);
+	}
+
 	BUG_ON(!list_empty(&cgrp->pidlists));
 }
 
@@ -222,21 +240,26 @@ static void cgroup_pidlist_destroy_work_fn(struct work_struct *work)
 	struct cgroup_pidlist *l = container_of(dwork, struct cgroup_pidlist,
 						destroy_dwork);
 	struct cgroup_pidlist *tofree = NULL;
+	struct cgroup *owner;
 
-	mutex_lock(&l->owner->pidlist_mutex);
+	owner = READ_ONCE(l->owner);
+	if (owner) {
+		mutex_lock(&owner->pidlist_mutex);
 
-	/*
-	 * Destroy iff we didn't get queued again.  The state won't change
-	 * as destroy_dwork can only be queued while locked.
-	 */
-	if (!delayed_work_pending(dwork)) {
-		list_del(&l->links);
-		kvfree(l->list);
-		put_pid_ns(l->key.ns);
-		tofree = l;
-	}
+		/*
+		 * Destroy iff we didn't get queued again and we're still
+		 * owned by the cgroup.  If ->owner was cleared by
+		 * cgroup1_pidlist_destroy_all(), it will free us.
+		 */
+		if (READ_ONCE(l->owner) == owner && !delayed_work_pending(dwork)) {
+			list_del(&l->links);
+			kvfree(l->list);
+			put_pid_ns(l->key.ns);
+			tofree = l;
+		}
 
-	mutex_unlock(&l->owner->pidlist_mutex);
+		mutex_unlock(&owner->pidlist_mutex);
+	}
 	kfree(tofree);
 }
 
-- 
2.43.0

Re: [PATCH v3] cgroup: avoid flushing global workqueue in cgroup1_pidlist_destroy_all

[Junnan Zhang]

Gentle ping.

It's been a couple of weeks since v3, and there hasn't been any
maintainer feedback yet. Could you take a look when you have a
chance? Happy to address any comments or rebase if needed.

Thanks,
Junnan

Re: [PATCH v3] cgroup: avoid flushing global workqueue in cgroup1_pidlist_destroy_all

[Michal Koutný]

[-- Attachment #1: Type: text/plain, Size: 3099 bytes --]

Hi Junnan.

(Sorry for late response, I sketched some notes and then didn't get down
to sent them. Now they're below.)

On Fri, Aug 14, 2026 at 06:20:52PM +0800, Junnan Zhang <zhangjn_dev@163.com> wrote:
> From: Junnan Zhang <zhangjn11@chinatelecom.cn>
> 
> cgroup1_pidlist_destroy_all() flushes the global
> cgroup_pidlist_destroy_wq while destroying a cgroup. Because all cgroup
> v1 pidlist destruction work items are queued on the same shared workqueue,
> a single slow or stuck work item (e.g. waiting for pidlist_mutex held by a
> user-space reader) blocks every concurrent cgroup destruction path.
> 
> This can lead to kworker tasks stuck in flush_workqueue() for over
> hung_task_timeout seconds, as observed on busy systems running Docker or
> Kubernetes workloads.

Since the cgroup_pidlist_destroy_wq is already a dedicated workqueue (no
other conteders), the pursuit of pidlist_mutex holder is a feasible
theory. However, that would also mean:
a) a single reader taking more than hung_task_timeout_secs (that'd be
   a softlockup earlier),
b) starvation of cgroup1_pidlist_destroy_all() by many (queued)
   cgroup_pidlist_start() callers which goes against the second-long
   caching of pidlists,
c) there is large number of nr_cgroups * nr_pidnses which makes the
   caching ineffective

> 
>   INFO: task kworker/0:1:1438499 blocked for more than 120 seconds.
>   "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
>   kworker/0:1 D 0 1438499 2 0x80000080
>   Workqueue: cgroup_destroy css_free_rwork_fn
>   ? __schedule+0x296/0x900
>   schedule+0x28/0x80
>   schedule_timeout+0x1ee/0x3a0
>   ? kvm_sched_clock_read+0xd/0x20
>   wait_for_completion+0x12c/0x190
>   ? wake_up_q+0x70/0x70
>   flush_workqueue+0x132/0x430
>   ? cgroup1_pidlist_destroy_all+0x7c/0xa0
>   cgroup1_pidlist_destroy_all+0x7c/0xa0
>   css_free_rwork_fn+0xb5/0x390
>   process_one_work+0x195/0x3e0
>   worker_thread+0x30/0x390
>   ? process_one_work+0x3e0/0x3e0
>   kthread+0x113/0x130
>   ? kthread_create_worker_on_cpu+0x70/0x70
>   ret_from_fork+0x1f/0x40
> 
> Fix it by moving the cgroup's pidlists to a local orphan list under
> pidlist_mutex, clearing their ->owner pointer, and then cancelling each
> pidlist's delayed work outside the lock. The destroy work function now
> checks ->owner and skips freeing orphaned pidlists, so
> cgroup1_pidlist_destroy_all() can free them safely without flushing the
> whole shared workqueue.

What's the point of the workqueue after this change? (Mainly the
expiration + having process context for the handler.)

The flushing isn't necessary if there's a way how to ensure pidlists
head won't be used after cgrp removal, which the fix should achieve.


So I'd say, the narrow-focused cancellation may work, no need to wait
for other cgroups. OTOH, I'm surprised this v1-issue popped up only now
and whether such a long contention can happen over pidlist_mutex as your
commit message implies. What nr_cgroups, nr_pidnses could cause this in
your theory?

Thanks,
Michal


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 265 bytes --]

Re: [PATCH v3] cgroup: avoid flushing global workqueue in cgroup1_pidlist_destroy_all

[Ridong Chen]

On 8/31/2026 4:16 PM, Michal Koutný wrote:
> Hi Junnan.
> 
> (Sorry for late response, I sketched some notes and then didn't get down
> to sent them. Now they're below.)
> 
> On Fri, Aug 14, 2026 at 06:20:52PM +0800, Junnan Zhang <zhangjn_dev@163.com> wrote:
>> From: Junnan Zhang <zhangjn11@chinatelecom.cn>
>>
>> cgroup1_pidlist_destroy_all() flushes the global
>> cgroup_pidlist_destroy_wq while destroying a cgroup. Because all cgroup
>> v1 pidlist destruction work items are queued on the same shared workqueue,
>> a single slow or stuck work item (e.g. waiting for pidlist_mutex held by a
>> user-space reader) blocks every concurrent cgroup destruction path.
>>
>> This can lead to kworker tasks stuck in flush_workqueue() for over
>> hung_task_timeout seconds, as observed on busy systems running Docker or
>> Kubernetes workloads.
> 
> Since the cgroup_pidlist_destroy_wq is already a dedicated workqueue (no
> other conteders), the pursuit of pidlist_mutex holder is a feasible
> theory. However, that would also mean:
> a) a single reader taking more than hung_task_timeout_secs (that'd be
>     a softlockup earlier),
> b) starvation of cgroup1_pidlist_destroy_all() by many (queued)
>     cgroup_pidlist_start() callers which goes against the second-long
>     caching of pidlists,
> c) there is large number of nr_cgroups * nr_pidnses which makes the
>     caching ineffective
> 
>>
>>    INFO: task kworker/0:1:1438499 blocked for more than 120 seconds.
>>    "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
>>    kworker/0:1 D 0 1438499 2 0x80000080
>>    Workqueue: cgroup_destroy css_free_rwork_fn
>>    ? __schedule+0x296/0x900
>>    schedule+0x28/0x80
>>    schedule_timeout+0x1ee/0x3a0
>>    ? kvm_sched_clock_read+0xd/0x20
>>    wait_for_completion+0x12c/0x190
>>    ? wake_up_q+0x70/0x70
>>    flush_workqueue+0x132/0x430
>>    ? cgroup1_pidlist_destroy_all+0x7c/0xa0
>>    cgroup1_pidlist_destroy_all+0x7c/0xa0
>>    css_free_rwork_fn+0xb5/0x390
>>    process_one_work+0x195/0x3e0
>>    worker_thread+0x30/0x390
>>    ? process_one_work+0x3e0/0x3e0
>>    kthread+0x113/0x130
>>    ? kthread_create_worker_on_cpu+0x70/0x70
>>    ret_from_fork+0x1f/0x40
>>

I'm not sure we've found the real root cause yet, and the current analysis 
doesn't convince me.

Shouldn't we first figure out why flush_workqueue() waited 120s? Was it because 
there were too many pids, or because someone held pidlist_mutex for too long?

>> Fix it by moving the cgroup's pidlists to a local orphan list under
>> pidlist_mutex, clearing their ->owner pointer, and then cancelling each
>> pidlist's delayed work outside the lock. The destroy work function now
>> checks ->owner and skips freeing orphaned pidlists, so
>> cgroup1_pidlist_destroy_all() can free them safely without flushing the
>> whole shared workqueue.
> 
> What's the point of the workqueue after this change? (Mainly the
> expiration + having process context for the handler.)
> 
> The flushing isn't necessary if there's a way how to ensure pidlists
> head won't be used after cgrp removal, which the fix should achieve.
> 
> 
> So I'd say, the narrow-focused cancellation may work, no need to wait
> for other cgroups. OTOH, I'm surprised this v1-issue popped up only now
> and whether such a long contention can happen over pidlist_mutex as your
> commit message implies. What nr_cgroups, nr_pidnses could cause this in
> your theory?
> 
> Thanks,
> Michal

-- 
Best regards
Ridong

Re: [PATCH v3] cgroup: avoid flushing global workqueue in cgroup1_pidlist_destroy_all

[Junnan Zhang]

Hi Ridong,

> Shouldn't we first figure out why flush_workqueue() waited 120s? Was
> it because there were too many pids, or because someone held
> pidlist_mutex for too long?

That's the same question Michal raised; please see my reply to him in
this thread for the full analysis. The short version: it's neither a
single long mutex holder nor oversized pidlists per se -- the wait is
backlog x per-work latency. flush_workqueue() waits for every work
already queued on the shared wq, which drains serially
(WQ_PERCPU, max_active=1). Container churn keeps queueing destroy
works, and each work must take the owner's pidlist_mutex behind
readers whose pidlist_array_load() runs entirely under that mutex.
A few thousand queued works each delayed by tens of ms is enough to
exceed 120s.

Unfortunately the guest memory dump captured at the incident couldn't
be analyzed with crash, so exact queue depths aren't available; I've
offered to build a synthetic reproducer with measured latency data if
that's needed to move this forward.

Thanks,
Junnan

Re: [PATCH v3] cgroup: avoid flushing global workqueue in cgroup1_pidlist_destroy_all

[Junnan Zhang]

Hi Ridong,

> Shouldn't we first figure out why flush_workqueue() waited 120s? Was
> it because there were too many pids, or because someone held
> pidlist_mutex for too long?

That's the same question Michal raised; please see my reply to him in
this thread for the full analysis. The short version: it's neither a
single long mutex holder nor oversized pidlists per se -- the wait is
backlog x per-work latency. flush_workqueue() waits for every work
already queued on the shared wq, which drains serially
(WQ_PERCPU, max_active=1). Container churn keeps queueing destroy
works, and each work must take the owner's pidlist_mutex behind
readers whose pidlist_array_load() runs entirely under that mutex.
A few thousand queued works each delayed by tens of ms is enough to
exceed 120s.

Unfortunately the guest memory dump captured at the incident couldn't
be analyzed with crash, so exact queue depths aren't available; I've
offered to build a synthetic reproducer with measured latency data if
that's needed to move this forward.

Thanks,
Junnan

Re: [PATCH v3] cgroup: avoid flushing global workqueue in cgroup1_pidlist_destroy_all

[Junnan Zhang]

Hi Michal,

Thanks for the review.

> a) a single reader taking more than hung_task_timeout_secs (that'd be
>    a softlockup earlier),
> b) starvation of cgroup1_pidlist_destroy_all() by many (queued)
>    cgroup_pidlist_start() callers which goes against the second-long
>    caching of pidlists,
> c) there is large number of nr_cgroups * nr_pidnses which makes the
>    caching ineffective

An honest caveat first: this was reported by a customer on a production
Kubernetes node. A guest memory dump was taken at the time, but it
couldn't be analyzed with crash, so I can't give you the exact
nr_cgroups/nr_pidnses.

That said, I believe c) alone is sufficient, and neither a) nor b) is
needed to explain the 120s stall, because the flush latency is
backlog x per-work latency rather than the latency of any single work:

- backlog: flush_workqueue() waits for all works already queued on the
  shared wq, which drains serially (WQ_PERCPU, max_active=1). Container
  churn constantly runs cgroup destruction, and each destroyed cgroup
  queues one work per cached (type, ns) pidlist, so the queue ahead of
  the flusher grows with churn rate x nr_cgroups.

- per-work latency: each destroy work must acquire the owner's
  pidlist_mutex, contending with readers (kubelet/cadvisor/runtime
  scraping cgroup.procs). pidlist_array_load() runs entirely under
  that mutex (css_set walk, kvmalloc, sort), so on a node with frequent
  scraping each work can sit tens of ms behind a reader.

A few thousand queued works each delayed by tens of ms already exceed
hung_task_timeout_secs -- no single reader needs to hold the mutex for
that long (so a) doesn't apply), and each individual work does get the
mutex eventually, so it's not sustained starvation either (so b)
doesn't apply).

More on b): the second-long caching only helps readers that re-read
within that 1s window (e.g. a single `cat` doing several seq_file
iterations). Periodic scrapers like kubelet/cadvisor, with typical
intervals of 10-30s, never hit the cache at all: every scrape round
rebuilds the pidlist under the mutex and queues an expiry work 1s
later. So heavy reader traffic and the existence of the cache are not
in conflict -- the cache is simply ineffective for this access
pattern. It also means the wq steadily carries ~nr_cgroups expiry
works per scrape round, on top of the works queued by churn, which is
what the flusher ends up waiting behind.

On nr_pidnses specifically: containers typically share the pod/host
pid namespace, so in the common case the multiplier is really
nr_cgroups x scraping frequency rather than nr_pidnses. I can't confirm
the customer's pidns usage for the same reason as above.

> OTOH, I'm surprised this v1-issue popped up only now

cgroup v1 is legacy but still the default on widely deployed enterprise
distros, and per-node cgroup density plus metrics scraping frequency
have grown a lot in recent years, so the backlog needed to trip this
only became common recently. Triggering it also requires all three
conditions at once -- many cgroups, frequent full-sweep scraping, and
sustained create/destroy churn -- which is presumably why it isn't
seen more often: missing any one of them, the queue never builds up.
That's my best explanation -- admittedly not provable without an
analyzable dump.

> What's the point of the workqueue after this change? (Mainly the
> expiration + having process context for the handler.)

It still serves the normal path: the deferred expiry that makes the
pidlist cache work, and process context for freeing. The patch only
removes the flush from the cgroup-removal path where, as you noted,
the orphan list guarantees the pidlists head won't be used after cgrp
removal.

If the lack of exact field numbers is a blocker, I can put together a
synthetic reproducer (N cgroups with concurrent cgroup.procs readers
plus destroy churn) and report measured flush_workqueue() latency with
and without the patch. Would that address your concern?

Thanks,
Junnan

Re: [PATCH v3] cgroup: avoid flushing global workqueue in cgroup1_pidlist_destroy_all

[Junnan Zhang]

Hi Michal,

Thanks for the review.

> a) a single reader taking more than hung_task_timeout_secs (that'd be
>    a softlockup earlier),
> b) starvation of cgroup1_pidlist_destroy_all() by many (queued)
>    cgroup_pidlist_start() callers which goes against the second-long
>    caching of pidlists,
> c) there is large number of nr_cgroups * nr_pidnses which makes the
>    caching ineffective

An honest caveat first: this was reported by a customer on a production
Kubernetes node. A guest memory dump was taken at the time, but it
couldn't be analyzed with crash, so I can't give you the exact
nr_cgroups/nr_pidnses.

That said, I believe c) alone is sufficient, and neither a) nor b) is
needed to explain the 120s stall, because the flush latency is
backlog x per-work latency rather than the latency of any single work:

- backlog: flush_workqueue() waits for all works already queued on the
  shared wq, which drains serially (WQ_PERCPU, max_active=1). Container
  churn constantly runs cgroup destruction, and each destroyed cgroup
  queues one work per cached (type, ns) pidlist, so the queue ahead of
  the flusher grows with churn rate x nr_cgroups.

- per-work latency: each destroy work must acquire the owner's
  pidlist_mutex, contending with readers (kubelet/cadvisor/runtime
  scraping cgroup.procs). pidlist_array_load() runs entirely under
  that mutex (css_set walk, kvmalloc, sort), so on a node with frequent
  scraping each work can sit tens of ms behind a reader.

A few thousand queued works each delayed by tens of ms already exceed
hung_task_timeout_secs -- no single reader needs to hold the mutex for
that long (so a) doesn't apply), and each individual work does get the
mutex eventually, so it's not sustained starvation either (so b)
doesn't apply).

More on b): the second-long caching only helps readers that re-read
within that 1s window (e.g. a single `cat` doing several seq_file
iterations). Periodic scrapers like kubelet/cadvisor, with typical
intervals of 10-30s, never hit the cache at all: every scrape round
rebuilds the pidlist under the mutex and queues an expiry work 1s
later. So heavy reader traffic and the existence of the cache are not
in conflict -- the cache is simply ineffective for this access
pattern. It also means the wq steadily carries ~nr_cgroups expiry
works per scrape round, on top of the works queued by churn, which is
what the flusher ends up waiting behind.

On nr_pidnses specifically: containers typically share the pod/host
pid namespace, so in the common case the multiplier is really
nr_cgroups x scraping frequency rather than nr_pidnses. I can't confirm
the customer's pidns usage for the same reason as above.

> OTOH, I'm surprised this v1-issue popped up only now

cgroup v1 is legacy but still the default on widely deployed enterprise
distros, and per-node cgroup density plus metrics scraping frequency
have grown a lot in recent years, so the backlog needed to trip this
only became common recently. Triggering it also requires all three
conditions at once -- many cgroups, frequent full-sweep scraping, and
sustained create/destroy churn -- which is presumably why it isn't
seen more often: missing any one of them, the queue never builds up.
That's my best explanation -- admittedly not provable without an
analyzable dump.

> What's the point of the workqueue after this change? (Mainly the
> expiration + having process context for the handler.)

It still serves the normal path: the deferred expiry that makes the
pidlist cache work, and process context for freeing. The patch only
removes the flush from the cgroup-removal path where, as you noted,
the orphan list guarantees the pidlists head won't be used after cgrp
removal.

If the lack of exact field numbers is a blocker, I can put together a
synthetic reproducer (N cgroups with concurrent cgroup.procs readers
plus destroy churn) and report measured flush_workqueue() latency with
and without the patch. Would that address your concern?

Thanks,
Junnan