Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1681632 > unrolled thread

Re: [PATCH] firmware: wake all waiters

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] firmware: wake all waiters Peter Zijlstra <peterz@infradead.org> - 2017-07-05 18:20 +0200
    Re: [PATCH] firmware: wake all waiters Linus Torvalds <torvalds@linux-foundation.org> - 2017-07-05 18:40 +0200
      Re: [PATCH] firmware: wake all waiters "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-07-12 20:50 +0200

#1681632 — Re: [PATCH] firmware: wake all waiters

On Tue, Jun 27, 2017 at 01:30:30AM +0200, Luis R. Rodriguez wrote:

> > But the *only* reason for swait is extreme memory issues and some very
> > special realtime issues, where it saves a couple of bytes in
> > structures that need close packing, and doesn't even use normal
> > spinlocks, so it saves a couple of cycles at wakeup/sleep because it
> > doesn't do a good job in general.

Yes, its about real-time. But it shouldn't be _that_ special purpose.

So swait should be capable of most things people want from a waitqueue.

Things it explicitly does not do are things like:

  - custom wake functions
  - exclusive mode
  - mixing different sleep types on a queue (which I would recommend
    against in any case).

But only very few people need those.

> > The "avoid normal spinlocks" is because it is meant for code that is
> > *so* special that it needs the magical low-level raw spinlocks.

> > I think the two valid users are RCU (which needed it for RT), and kvm
> > (which also needed it for similar issues - it needs to be
> > non-preemptible).

Right, so we need the raw_spinlock in order to run from non-preemptible
code on RT. And we then also need bounded runtimes on stuff.

The only function which is affected by that is swake_up_all(), that does
an unbounded list iteration and is therefore required to be called from
the schedulable context (so we can drop the lock and gain bounds on the
preemption latency).


But aside of all that, it should provide 'everything' people want from a
bog standard regular waitqueue. So I don't see why people shouldn't use
it more.

In any case, I'm not seeing why you call it idiotic.

[toc] | [next] | [standalone]

#1681645

On Wed, Jul 5, 2017 at 9:18 AM, Peter Zijlstra <peterz@infradead.org> wrote:
>
> So swait should be capable of most things people want from a waitqueue.

But what's the point?

Regular wait-queues work fine. The advantages of swait aren't huge
even if you fix the crap it does now.

And the disadvantages of "another interface to do the same thing" are
big indeed.

Keep it specialized.

> Right, so we need the raw_spinlock in order to run from non-preemptible
> code on RT. And we then also need bounded runtimes on stuff.
>
> The only function which is affected by that is swake_up_all(),

No.

The fact is, "swake_up()" needs to do that "wake up all" for this
interface to be even *REMOTELY* acceptable for general use.

Seriously. If  the regular "swake_up()" doesn't do what people expect
from a wakeup, then it damn well should not be used, and not be taught
to people. It's a very specialized interface for only two users, and
right now it looks like one of those two users shouldn't have used it
in the first place.

> In any case, I'm not seeing why you call it idiotic.

Have you read the problems?

There were originally three users:

 - one of them is firmware loading, and it was actively *buggy* due to
using hat shit interface, and it's going away

 - one of them is kvm, and for kvm that interface was shit.

 - the final one is RCU, and even there it is very questionable.

There is no question: swait() is a mistake and should not be used. And
you're apparently still in denial about how completely broken it is to
have "swake_up()" have different semantics from "swake_up_all()".

                  Linus

[toc] | [prev] | [next] | [standalone]

#1685964

On Wed, Jul 05, 2017 at 09:33:38AM -0700, Linus Torvalds wrote:
> On Wed, Jul 5, 2017 at 9:18 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> > In any case, I'm not seeing why you call it idiotic.
> 
> Have you read the problems?
> 
> There were originally three users:
> 
>  - one of them is firmware loading, and it was actively *buggy* due to
> using hat shit interface, and it's going away

To be fair the issue with the firmware loading was due to a regression on the
port from using swait/completion to swait. swake_up() was used instead of
swake_up_all(). Before the port complate_all() was used instead of just
complete().

So I would personally not add the firmware API to the list of reasons why swait
would be crap.

The more *general* semantics issue you pointed out with swake_up() though seem
more reasonable to be attacking the swait API and those are best followed on
the other thread [0].

I should also point out that there are *other* issues with the firmware API on
this whole wait stuff, but that have been present since even before when we
were using the regular wait / completion API -- a wake was never issued upon an
error in some situations on some kernel builds. I have a fix for that now along
with a test case for it which I'll post soon. After all these fixes the code
works as expected with either the swait API or the good 'ol wait/completion
API. But given swait is still out by you as only specialized I did already post
patches to revert the firmware API to switch back to the wait/completion API.

[0] https://lkml.kernel.org/r/CA+55aFykNULx-b6M6FmUYdK2cn-OJKKfjaPwLN5xZGK+bioGaA@mail.gmail.com

  Luis

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web