linux-kernel - Re: [PATCH 0/3] workqueue: Make the workqueue code PREEMPT

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Wed, 27 May 2020 08:52:09 -0700
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Sebastian Andrzej Siewior <bigeasy@...utronix.de>
Cc:     Tejun Heo <tj@...nel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Lai Jiangshan <jiangshanlai@...il.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...nel.org>
Subject: Re: [PATCH 0/3] workqueue: Make the workqueue code PREEMPT_RT safe

On Wed, May 27, 2020 at 8:20 AM Sebastian Andrzej Siewior
<bigeasy@...utronix.de> 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