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

Groups > linux.kernel > #1673944 > unrolled thread

[PATCH] firmware: wake all waiters

Back to article view | Back to linux.kernel

Contents

  [PATCH] firmware: wake all waiters Jakub Kicinski <jakub.kicinski@netronome.com> - 2017-06-24 01:40 +0200
    Re: [PATCH] firmware: wake all waiters "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-06-26 23:30 +0200
      Re: [PATCH] firmware: wake all waiters "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-06-27 01:50 +0200
      Re: [PATCH] firmware: wake all waiters Jakub Kicinski <jakub.kicinski@netronome.com> - 2017-06-27 04:20 +0200
        Re: [PATCH] firmware: wake all waiters "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-06-27 18:40 +0200
          Re: [PATCH] firmware: wake all waiters Jakub Kicinski <jakub.kicinski@netronome.com> - 2017-06-27 23:30 +0200
            Re: [PATCH] firmware: wake all waiters "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-06-28 00:30 +0200
              Re: [PATCH] firmware: wake all waiters Jakub Kicinski <jakub.kicinski@netronome.com> - 2017-06-28 00:50 +0200
                Re: [PATCH] firmware: wake all waiters "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-06-28 02:00 +0200
              Re: [systemd-devel] [PATCH] firmware: wake all waiters Lennart Poettering <mzxreary@0pointer.de> - 2017-06-28 09:10 +0200
                Re: [systemd-devel] [PATCH] firmware: wake all waiters "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-06-28 18:10 +0200
                  Re: [systemd-devel] [PATCH] firmware: wake all waiters Lennart Poettering <mzxreary@0pointer.de> - 2017-06-28 20:00 +0200
                    Re: [systemd-devel] [PATCH] firmware: wake all waiters "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-06-28 20:00 +0200
                  Re: [systemd-devel] [PATCH] firmware: wake all waiters Daniel Wagner <wagi@monom.org> - 2017-06-29 22:10 +0200
        Re: [PATCH] firmware: wake all waiters Bjorn Andersson <bjorn.andersson@linaro.org> - 2017-06-27 20:00 +0200
          Re: [PATCH] firmware: wake all waiters "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-06-27 20:10 +0200
            Re: [PATCH] firmware: wake all waiters Bjorn Andersson <bjorn.andersson@linaro.org> - 2017-06-27 21:00 +0200
              Re: [PATCH] firmware: wake all waiters "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-06-27 21:10 +0200
                Re: [PATCH] firmware: wake all waiters Bjorn Andersson <bjorn.andersson@linaro.org> - 2017-06-27 22:00 +0200
                  Re: [PATCH] firmware: wake all waiters "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-06-27 22:30 +0200
    Re: [PATCH] firmware: wake all waiters Linus Torvalds <torvalds@linux-foundation.org> - 2017-06-26 23:50 +0200
      Re: [PATCH] firmware: wake all waiters "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-06-27 01:40 +0200
        Re: [PATCH] firmware: wake all waiters Linus Torvalds <torvalds@linux-foundation.org> - 2017-06-27 01:50 +0200
          Re: [PATCH] firmware: wake all waiters "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-06-27 02:20 +0200
            Re: [PATCH] firmware: wake all waiters Davidlohr Bueso <dave@stgolabs.net> - 2017-06-28 15:50 +0200
              Re: [PATCH] firmware: wake all waiters "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-06-28 18:00 +0200
                Re: [PATCH] firmware: wake all waiters Davidlohr Bueso <dave@stgolabs.net> - 2017-06-28 21:10 +0200
            Re: [PATCH] firmware: wake all waiters Davidlohr Bueso <dave@stgolabs.net> - 2017-06-29 21:10 +0200
              Re: [PATCH] firmware: wake all waiters "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-06-29 21:50 +0200
                Re: [PATCH] firmware: wake all waiters Davidlohr Bueso <dave@stgolabs.net> - 2017-06-30 18:40 +0200

Page 2 of 2 — ← Prev page 1 [2]

#1675122

On Fri, Jun 23, 2017 at 4:37 PM, Jakub Kicinski
<jakub.kicinski@netronome.com> wrote:
> -               swake_up(&fw_st->wq);
> +               swake_up_all(&fw_st->wq);

Why is that code using the braindamaed "swait" model in the first place?

That's the real problem here - swait() is a very specialized
interface, and it does not make sense to use it here.

Among all the simplifications it has is exactly the fact that it wakes
up only one thing, because it is *so* specialized.

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.

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 really have *no* idea why the firmware code uses that idiotic
special wait-queue. It has no reason to do so, except this comment
from the commit that added it:

  "We use also swait instead of wait because don't need all the additional
    features wait provides."

which is bogus, since it clearly just got the waiting wrong exactly
*because* swait is pretty damn bad and specialized.

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).

I don't see any similar reason for the firmware loading, and all it
did was use an odd interface that resulted in this bug.

Why is the firmware code being so damn odd on purpose?

           Linus

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

#1675190

On Mon, Jun 26, 2017 at 02:44:17PM -0700, Linus Torvalds wrote:
> On Fri, Jun 23, 2017 at 4:37 PM, Jakub Kicinski
> <jakub.kicinski@netronome.com> wrote:
> > -               swake_up(&fw_st->wq);
> > +               swake_up_all(&fw_st->wq);
> 
> Why is that code using the braindamaed "swait" model in the first place?

The conversion was done via commit 5b029624948d ("firmware: do not use fw_lock
for fw_state protection").  This commit overlooked this "batched request"
feature.

> That's the real problem here - swait() is a very specialized
> interface, and it does not make sense to use it here.
> 
> Among all the simplifications it has is exactly the fact that it wakes
> up only one thing, because it is *so* specialized.

Not sure I follow, it can wake up all items in queue with swake_up_all(), no?

At first I *thought* we don't add further items with __prepare_to_swait() but
actually that if (list_empty(&wait->task_list)) check just checks if the
item was already added to a wait list. So swait can queue more waiters, no?

> 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.

I see.

> 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 really have *no* idea why the firmware code uses that idiotic
> special wait-queue. It has no reason to do so, except this comment
> from the commit that added it:
> 
>   "We use also swait instead of wait because don't need all the additional
>     features wait provides."
> 
> which is bogus, since it clearly just got the waiting wrong exactly
> *because* swait is pretty damn bad and specialized.

If indeed it cannot queue and wake all then surely this is buggered!

> 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).

Got it.

> I don't see any similar reason for the firmware loading, and all it
> did was use an odd interface that resulted in this bug.

From my review when this was suggested it seemed to cover all grounds
and an API that was more light weight seemed sensible. The swait documentation
seems to note "One would recommend using this wait queue where possible."
If its non suitable for multiple waits then indeed this needs fixing so
this doesn't happen again.

> Why is the firmware code being so damn odd on purpose?

I've been asking myself that since I started reviewing the firmware code :) But
in this case I believe the commit overlooked batched requests functionality and
to use swake_up_all().

  Luis

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

#1675194

On Mon, Jun 26, 2017 at 4:30 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
>
> On Mon, Jun 26, 2017 at 02:44:17PM -0700, Linus Torvalds wrote:
> >
> > Among all the simplifications it has is exactly the fact that it wakes
> > up only one thing, because it is *so* specialized.
>
> Not sure I follow, it can wake up all items in queue with swake_up_all(), no?

You can, yes.

But it's like using assembly language to build a compiler. Sure, it's
possible, but it's the wrong thing to do.

The thing should just use regular wait/wakeup, which has sane
*default* behavior that people are used to.

> If indeed it cannot queue and wake all then surely this is buggered!

It's not that it cannot, but that it's much more limited than all our
normal waiting support (and it's *meant* for much more limited
situations).

For example, our regular waiting code can handle not just "wake up
one" and "wake up all", it can handle "wake up <n> exclusive waiters,
and whoever isn't exclusive".

And by default, normal wait queues just do the right thing, so you
don't have to specify the exact details of the behavior.

Sure, the firmware loader code doesn't _need_ that, but there also
isn't one of the users who really needs the very limited interface
that has odd semantics and odd default behavior that will trip you up.

The swait interface is so special and so undocumented, that I really
didn't expect anybody to even know about it unless they had very
specific needs, much less use it.

             Linus

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

#1675208

On Mon, Jun 26, 2017 at 04:43:10PM -0700, Linus Torvalds wrote:
> The swait interface is so special and so undocumented, that I really
> didn't expect anybody to even know about it unless they had very
> specific needs, much less use it.

If swait is really not designed and intended to be used for cases that do not
require all the bells and whistles of wait, and is just very special-case, a
nice big warning about it seems appropriate on swait.h, instead of the
welcoming open armed, "One would recommend using this wait queue where
possible".

From d751201aebf7ca8acb765284a9017a711ddbe791 Mon Sep 17 00:00:00 2001
From: "Luis R. Rodriguez" <mcgrof@kernel.org>
Date: Mon, 26 Jun 2017 17:06:10 -0700
Subject: [PATCH] swait: annotate swait's special use

Before kernel hipsters start thinking swait is the cool thing to do.

Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
 include/linux/swait.h | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/include/linux/swait.h b/include/linux/swait.h
index 4a4e180d0a35..14fcf23cece4 100644
--- a/include/linux/swait.h
+++ b/include/linux/swait.h
@@ -29,7 +29,10 @@
  *
  * As a side effect of this; the data structures are slimmer.
  *
- * One would recommend using this wait queue where possible.
+ * NOTE: swait is for cases of extreme memory considerations and some very
+ * special realtime issues, where it saves a couple of bytes in structures that
+ * need close packing. As such its very special-use. Consider using regular
+ * waits queues from wait.h instead *first*.
  */
 
 struct task_struct;
-- 
2.11.0

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

#1676671

On Tue, 27 Jun 2017, Luis R. Rodriguez wrote:

>diff --git a/include/linux/swait.h b/include/linux/swait.h
>index 4a4e180d0a35..14fcf23cece4 100644
>--- a/include/linux/swait.h
>+++ b/include/linux/swait.h
>@@ -29,7 +29,10 @@
>  *
>  * As a side effect of this; the data structures are slimmer.
>  *
>- * One would recommend using this wait queue where possible.

So I think this was added due to the smaller footprint and fewer
cycles that swait has compared to the traditional (bulkier)
waitqueues. While probably not worth it, I guess we could offer
super-simple waitqueues (sswait? :-) which do not have the rt caveats
and uses a regular spinlock. The wakeup_all() call would not drop
the lock upon every wakeup as we are stripping the waitqueue not
for determinism, but for overhead. To mitigate this, we might
also want to use wake_q for reduced hold q->lock hold times.

But I don't think its worth yet another wait interface.
Alternatively, it crossed my mind we could also have wakeup_all()
use in the regular waitqueues, but I'd have to audit all the
current users to make sure we could actually do this.

Thanks,
Davidlohr

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

#1676873

On Wed, Jun 28, 2017 at 06:45:14AM -0700, Davidlohr Bueso wrote:
> On Tue, 27 Jun 2017, Luis R. Rodriguez wrote:
> 
> > diff --git a/include/linux/swait.h b/include/linux/swait.h
> > index 4a4e180d0a35..14fcf23cece4 100644
> > --- a/include/linux/swait.h
> > +++ b/include/linux/swait.h
> > @@ -29,7 +29,10 @@
> >  *
> >  * As a side effect of this; the data structures are slimmer.
> >  *
> > - * One would recommend using this wait queue where possible.
> 
> So I think this was added due to the smaller footprint and fewer
> cycles that swait has compared to the traditional (bulkier)
> waitqueues. While probably not worth it, I guess we could offer
> super-simple waitqueues (sswait? :-) which do not have the rt caveats
> and uses a regular spinlock. The wakeup_all() call would not drop
> the lock upon every wakeup as we are stripping the waitqueue not
> for determinism, but for overhead. To mitigate this, we might
> also want to use wake_q for reduced hold q->lock hold times.
> 
> But I don't think its worth yet another wait interface.
> Alternatively, it crossed my mind we could also have wakeup_all()
> use in the regular waitqueues, but I'd have to audit all the
> current users to make sure we could actually do this.

But this open-welcoming invite for swait then, should it go?

  Luis

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

#1677086

Hi Luis!

On Wed, 28 Jun 2017, Luis R. Rodriguez wrote:

>On Wed, Jun 28, 2017 at 06:45:14AM -0700, Davidlohr Bueso wrote:
>> On Tue, 27 Jun 2017, Luis R. Rodriguez wrote:
>>
>> > diff --git a/include/linux/swait.h b/include/linux/swait.h
>> > index 4a4e180d0a35..14fcf23cece4 100644
>> > --- a/include/linux/swait.h
>> > +++ b/include/linux/swait.h
>> > @@ -29,7 +29,10 @@
>> >  *
>> >  * As a side effect of this; the data structures are slimmer.
>> >  *
>> > - * One would recommend using this wait queue where possible.
>>
>> So I think this was added due to the smaller footprint and fewer
>> cycles that swait has compared to the traditional (bulkier)
>> waitqueues. While probably not worth it, I guess we could offer
>> super-simple waitqueues (sswait? :-) which do not have the rt caveats
>> and uses a regular spinlock. The wakeup_all() call would not drop
>> the lock upon every wakeup as we are stripping the waitqueue not
>> for determinism, but for overhead. To mitigate this, we might
>> also want to use wake_q for reduced hold q->lock hold times.
>>
>> But I don't think its worth yet another wait interface.
>> Alternatively, it crossed my mind we could also have wakeup_all()
>> use in the regular waitqueues, but I'd have to audit all the
>> current users to make sure we could actually do this.
>
>But this open-welcoming invite for swait then, should it go?

I have nothing against removing it.

Thanks,
Davidlohr

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

#1678099

On Tue, 27 Jun 2017, Luis R. Rodriguez wrote:
>  * As a side effect of this; the data structures are slimmer.
>  *
>- * One would recommend using this wait queue where possible.
>+ * NOTE: swait is for cases of extreme memory considerations and some very
>+ * special realtime issues, where it saves a couple of bytes in structures that
>+ * need close packing. As such its very special-use. Consider using regular
>+ * waits queues from wait.h instead *first*.

How about the following?

diff --git a/include/linux/swait.h b/include/linux/swait.h
index 4a4e180d0a35..f72f274f2a5f 100644
--- a/include/linux/swait.h
+++ b/include/linux/swait.h
@@ -9,13 +9,16 @@
 /*
  * Simple wait queues
  *
- * While these are very similar to the other/complex wait queues (wait.h) the
- * most important difference is that the simple waitqueue allows for
- * deterministic behaviour -- IOW it has strictly bounded IRQ and lock hold
- * times.
+ * While these are very similar to regular wait queues (wait.h) the most
+ * important difference is that the simple waitqueue allows for deterministic
+ * behaviour -- IOW it has strictly bounded IRQ and lock hold times.
  *
- * In order to make this so, we had to drop a fair number of features of the
- * other waitqueue code; notably:
+ * Mainly, this is accomplished by two things. Firstly not allowing swake_up_all
+ * from IRQ disabled, and dropping the lock upon every wakeup, giving a higher
+ * priority task a chance to run.
+ *
+ * Secondly, we had to drop a fair number of features of the other waitqueue
+ * code; notably:
  *
  *  - mixing INTERRUPTIBLE and UNINTERRUPTIBLE sleeps on the same waitqueue;
  *    all wakeups are TASK_NORMAL in order to avoid O(n) lookups for the right
@@ -24,12 +27,14 @@
  *  - the exclusive mode; because this requires preserving the list order
  *    and this is hard.
  *
- *  - custom wake functions; because you cannot give any guarantees about
- *    random code.
- *
- * As a side effect of this; the data structures are slimmer.
+ *  - custom wake callback functions; because you cannot give any guarantees
+ *    about random code. This also allows swait to be used in RT, such that
+ *    raw spinlock can be used for the swait queue head.
  *
- * One would recommend using this wait queue where possible.
+ * As a side effect of these; the data structures are slimmer albeit more ad-hoc.
+ * For all the above, note that simple wait queues should _only_ be used under
+ * very specific realtime constraints -- it is best to stick with the regular
+ * wait queues in most cases.
  */
 
 struct task_struct;

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

#1678132

On Thu, Jun 29, 2017 at 12:08 PM, Davidlohr Bueso <dave@stgolabs.net> wrote:
> On Tue, 27 Jun 2017, Luis R. Rodriguez wrote:
>>
>>  * As a side effect of this; the data structures are slimmer.
>>  *
>> - * One would recommend using this wait queue where possible.
>> + * NOTE: swait is for cases of extreme memory considerations and some
>> very
>> + * special realtime issues, where it saves a couple of bytes in
>> structures that
>> + * need close packing. As such its very special-use. Consider using
>> regular
>> + * waits queues from wait.h instead *first*.
>
>
> How about the following?

And do we want to make it even more *special-use* by wrapping it with
#ifndef MODULE as suggested by Matthew Wilcox ?

  Luis

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

#1678983

On Thu, 29 Jun 2017, Luis R. Rodriguez wrote:

>And do we want to make it even more *special-use* by wrapping it with
>#ifndef MODULE as suggested by Matthew Wilcox ?

Not if we want to add any sort of module that tests swait vs wait.

Thanks,
Davidlohr

[toc] | [prev] | [standalone]

Page 2 of 2 — ← Prev page 1 [2]

Back to top | Article view | linux.kernel

csiph-web