Re: [PATCH v2] workqueue: Add pool_workqueue to pending_pwqs list when unplugging multiple inactive works

[Waiman Long <longman@redhat.com>]

On 3/31/26 9:07 PM, Matthew Brost wrote:
> In unplug_oldest_pwq(), the first inactive work item on the
> pool_workqueue is activated correctly. However, if multiple inactive
> works exist on the same pool_workqueue, subsequent works fail to
> activate because wq_node_nr_active.pending_pwqs is empty — the list
> insertion is skipped when the pool_workqueue is plugged.
>
> Fix this by checking for additional inactive works in
> unplug_oldest_pwq() and updating wq_node_nr_active.pending_pwqs
> accordingly.
>
> v2:
>   - Use pwq_activate_first_inactive(pwq, false) rather than open coding
>     list operations (Tejun)
>
> Cc: Carlos Santa <carlos.santa@intel.com>
> Cc: Ryan Neph <ryanneph@google.com>
> Cc: stable@vger.kernel.org
> Cc: Tejun Heo <tj@kernel.org>
> Cc: Lai Jiangshan <jiangshanlai@gmail.com>
> Cc: Waiman Long <longman@redhat.com>
> Cc: linux-kernel@vger.kernel.org
> Fixes: 4c065dbce1e8 ("workqueue: Enable unbound cpumask update on ordered workqueues")
> Signed-off-by: Matthew Brost <matthew.brost@intel.com>
>
> ---
>
> This bug was first reported by Google, where the Xe driver appeared to
> hang due to a fencing signal not completing. We traced the issue to work
> items not being scheduled, and it can be trivially reproduced on drm-tip
> with the following commands:
>
> shell0:
> for i in {1..100}; do echo "Run $i"; xe_exec_threads --r \
> threads-rebind-bindexecqueue; done
>
> shell1:
> for i in {1..1000}; do echo "toggle $i"; echo f > \
> /sys/devices/virtual/workqueue/cpumask; echo ff > \
> /sys/devices/virtual/workqueue/cpumask; echo fff > \
> /sys/devices/virtual/workqueue/cpumask ; echo ffff > \
> /sys/devices/virtual/workqueue/cpumask; sleep .1; done
> ---
>   kernel/workqueue.c | 11 ++++++++++-
>   1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/workqueue.c b/kernel/workqueue.c
> index b77119d71641..bee3f37fffde 100644
> --- a/kernel/workqueue.c
> +++ b/kernel/workqueue.c
> @@ -1849,8 +1849,17 @@ static void unplug_oldest_pwq(struct workqueue_struct *wq)
>   	raw_spin_lock_irq(&pwq->pool->lock);
>   	if (pwq->plugged) {
>   		pwq->plugged = false;
> -		if (pwq_activate_first_inactive(pwq, true))
> +		if (pwq_activate_first_inactive(pwq, true)) {
> +			/*
> +			 * pwq is unbound. Additional inactive work_items need
> +			 * to reinsert the pwq into nna->pending_pwqs, which
> +			 * was skipped while pwq->plugged was true. See
> +			 * pwq_tryinc_nr_active() for additional details.
> +			 */
> +			pwq_activate_first_inactive(pwq, false);
> +
>   			kick_pool(pwq->pool);
> +		}
>   	}
>   	raw_spin_unlock_irq(&pwq->pool->lock);
>   }

Thanks for fixing this bug. However, calling pwq_activate_first_inactive 
twice can be a bit hard to understand. Will modifying 
pwq_tryinc_nr_active() like the following works?

Thanks,
Longman

diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index b77119d71641..b35e6e62e474 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -1738,9 +1738,6 @@ static bool pwq_tryinc_nr_active(struct pool_workqueue *pwq, bool fill)
                 goto out;
         }
  
-       if (unlikely(pwq->plugged))
-               return false;
-
         /*
          * Unbound workqueue uses per-node shared nr_active $nna. If @pwq is
          * already waiting on $nna, pwq_dec_nr_active() will maintain the
@@ -1749,13 +1746,19 @@ static bool pwq_tryinc_nr_active(struct pool_workqueue *pwq, bool fill)
          * We need to ignore the pending test after max_active has increased as
          * pwq_dec_nr_active() can only maintain the concurrency level but not
          * increase it. This is indicated by @fill.
+        *
+        * If @pwq is plugged, we need to make sure that it is linked to a
+        * pending_pwqs of a $nna.
+        *
          */
-       if (!list_empty(&pwq->pending_node) && likely(!fill))
+       if (!list_empty(&pwq->pending_node) && likely(!fill || pwq->plugged))
                 goto out;
  
-       obtained = tryinc_node_nr_active(nna);
-       if (obtained)
-               goto out;
+       if (likely(!pwq->plugged)) {
+               obtained = tryinc_node_nr_active(nna);
+               if (obtained)
+                       goto out;
+       }
  
         /*
          * Lockless acquisition failed. Lock, add ourself to $nna->pending_pwqs
@@ -1773,7 +1776,8 @@ static bool pwq_tryinc_nr_active(struct pool_workqueue *pwq, bool fill)
  
         smp_mb();
  
-       obtained = tryinc_node_nr_active(nna);
+       if (likely(!pwq->plugged))
+               obtained = tryinc_node_nr_active(nna);
  
         /*
          * If @fill, @pwq might have already been pending. Being spuriously