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

[Linus Torvalds]

On Wed, May 27, 2020 at 8:20 AM Sebastian Andrzej Siewior
<bigeasy@xxxxxxxxxxxxx> wrote:
>
> On 2020-05-26 14:46:59 [-0700], Linus Torvalds wrote:
> >
> > 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:

Hmm. That patch certainly looks fairly simple and straightforward to me.

That said, I think you're missing a rcuwait_init() to initialize the
thing (or probably better - a __RCUWAIT_INITIALIZER(name)
initializer).

Not that it's actually needed in the current implementation (a NULL
initializer is fine, and you get it from the variable being static),
but it would be a good thing for future-proofing in case people add
debugging or whatever to it.

> +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;
> +}

Heh. Ok, I see what and why you're doing it this way, and it's even clever.

But it's clever enough to want a comment both here and in the
rcuwait_wait_event() use. Just something simple like "this returns
with the lock held on success" here, and then at the wait-event
something like "because of how wq_manager_inactive() works, we will
hold the spinlock after a successful wait".

But yes, it looks quite simple and straightforward other than this.

Famous last words. Maybe I'm missing something, but I like this a lot
more than the swait thing.

                 Linus