Re: [PATCH 0/3] workqueue: Make the workqueue code PREEMPT_RT safe

[Sebastian Andrzej Siewior]

On 2020-05-26 14:46:59 [-0700], Linus Torvalds wrote:
> On Tue, May 26, 2020 at 2:41 PM Linus Torvalds
> <torva...@linux-foundation.org> wrote:
> >
> > Almost all users of swait have historically been buggy and/or
> > pointless.
> 
> Yeah, looking at this one, it really seems to fundamentally fall in
> the "pointless" category.
> 
> So yeah - instead of extending swait with a new primitive that nobody
> else wants than this pointless case, just don't use swait at all.
> 
> We have better models. We have "rcuwait", and we have
> "wake_up_process()". Either of which is simpler and more efficient
> than swait, and are actually useful. rcuwait isn't exactly widely
> used, but it has very nice semantics for when that is what you want.
> And wake_up_process() is both simple and straightforward, particularly
> when you already have a spinlock for protecting whatever state it is
> you're waking up on or waiting for.

rcuwait would be this:

diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 891ccad5f2716..3259f52bfe765 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -301,7 +301,7 @@ static struct workqueue_attrs 
*wq_update_unbound_numa_attrs_buf;
 static DEFINE_MUTEX(wq_pool_mutex);    /* protects pools and workqueues list */
 static DEFINE_MUTEX(wq_pool_attach_mutex); /* protects worker attach/detach */
 static DEFINE_SPINLOCK(wq_mayday_lock);        /* protects wq->maydays list */
-static DECLARE_WAIT_QUEUE_HEAD(wq_manager_wait); /* wait for manager to go 
away */
+static struct rcuwait manager_wait;    /* wait for manager to go away */
 
 static LIST_HEAD(workqueues);          /* PR: list of all workqueues */
 static bool workqueue_freezing;                /* PL: have wqs started 
freezing? */
@@ -2140,7 +2140,7 @@ static bool manage_workers(struct worker *worker)
 
        pool->manager = NULL;
        pool->flags &= ~POOL_MANAGER_ACTIVE;
-       wake_up(&wq_manager_wait);
+       rcuwait_wake_up(&manager_wait);
        return true;
 }
 
@@ -3504,6 +3504,17 @@ static void rcu_free_pool(struct rcu_head *rcu)
        kfree(pool);
 }
 
+static bool wq_manager_inactive(struct worker_pool *pool)
+{
+       spin_lock_irq(&pool->lock);
+
+       if (pool->flags & POOL_MANAGER_ACTIVE) {
+               spin_unlock_irq(&pool->lock);
+               return false;
+       }
+       return true;
+}
+
 /**
  * put_unbound_pool - put a worker_pool
  * @pool: worker_pool to put
@@ -3540,9 +3551,8 @@ static void put_unbound_pool(struct worker_pool *pool)
         * @pool's workers from blocking on attach_mutex.  We're the last
         * manager and @pool gets freed with the flag set.
         */
-       spin_lock_irq(&pool->lock);
-       wait_event_lock_irq(wq_manager_wait,
-                           !(pool->flags & POOL_MANAGER_ACTIVE), pool->lock);
+       rcuwait_wait_event(&manager_wait, wq_manager_inactive(pool),
+                          TASK_UNINTERRUPTIBLE);
        pool->flags |= POOL_MANAGER_ACTIVE;
 
        while ((worker = first_idle_worker(pool)))

There is the unbalanced lock/unlock in wq_manager_inactive() which
sparse complains about. Other than that it looks straight forward.

>                  Linus

Sebastian