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

Groups > linux.kernel > #1677732 > unrolled thread

Re: [PATCH 2/4] swait: add the missing killable swaits

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 2/4] swait: add the missing killable swaits Greg KH <gregkh@linuxfoundation.org> - 2017-06-29 15:00 +0200
    Re: [PATCH 2/4] swait: add the missing killable swaits Thomas Gleixner <tglx@linutronix.de> - 2017-06-29 15:10 +0200
      Re: [PATCH 2/4] swait: add the missing killable swaits Greg KH <gregkh@linuxfoundation.org> - 2017-06-29 15:40 +0200
        Re: [PATCH 2/4] swait: add the missing killable swaits Thomas Gleixner <tglx@linutronix.de> - 2017-06-29 15:50 +0200
          Re: [PATCH 2/4] swait: add the missing killable swaits Linus Torvalds <torvalds@linux-foundation.org> - 2017-06-29 18:20 +0200
            Re: [PATCH 2/4] swait: add the missing killable swaits "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-06-29 19:30 +0200
            Re: [PATCH 2/4] swait: add the missing killable swaits Davidlohr Bueso <dave@stgolabs.net> - 2017-06-29 19:50 +0200
              Re: [PATCH 2/4] swait: add the missing killable swaits Linus Torvalds <torvalds@linux-foundation.org> - 2017-06-29 20:00 +0200
                Re: [PATCH 2/4] swait: add the missing killable swaits Davidlohr Bueso <dave@stgolabs.net> - 2017-06-29 20:40 +0200
                  Re: [PATCH 2/4] swait: add the missing killable swaits Linus Torvalds <torvalds@linux-foundation.org> - 2017-06-29 21:00 +0200
                    Re: [PATCH 2/4] swait: add the missing killable swaits "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-06-29 21:50 +0200
                      Re: [PATCH 2/4] swait: add the missing killable swaits "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-06-29 21:50 +0200
                        Re: [PATCH 2/4] swait: add the missing killable swaits Jakub Kicinski <jakub.kicinski@netronome.com> - 2017-06-29 23:00 +0200
                          Re: [PATCH 2/4] swait: add the missing killable swaits Jakub Kicinski <jakub.kicinski@netronome.com> - 2017-06-30 01:00 +0200
                            Re: [PATCH 2/4] swait: add the missing killable swaits Jakub Kicinski <jakub.kicinski@netronome.com> - 2017-06-30 01:10 +0200
                            Re: [PATCH 2/4] swait: add the missing killable swaits "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-06-30 01:10 +0200
                          Re: [PATCH 2/4] swait: add the missing killable swaits "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-06-30 01:00 +0200
                      Re: [PATCH 2/4] swait: add the missing killable swaits Linus Torvalds <torvalds@linux-foundation.org> - 2017-06-29 23:00 +0200
            Re: [PATCH 2/4] swait: add the missing killable swaits Marcelo Tosatti <mtosatti@redhat.com> - 2017-06-30 04:40 +0200
              Re: [PATCH 2/4] swait: add the missing killable swaits Linus Torvalds <torvalds@linux-foundation.org> - 2017-06-30 06:10 +0200
                Re: [PATCH 2/4] swait: add the missing killable swaits Marcelo Tosatti <mtosatti@redhat.com> - 2017-06-30 14:00 +0200
                Re: [PATCH 2/4] swait: add the missing killable swaits Marcelo Tosatti <mtosatti@redhat.com> - 2017-06-30 14:10 +0200
                Re: [PATCH 2/4] swait: add the missing killable swaits Krister Johansen <kjlx@templeofstupid.com> - 2017-06-30 19:40 +0200

Page 1 of 2  [1] 2  Next page →

#1677732 — Re: [PATCH 2/4] swait: add the missing killable swaits

On Wed, Jun 14, 2017 at 03:20:15PM -0700, Luis R. Rodriguez wrote:
> Code in kernel which incorrectly used the non-killable variants could
> end up having waits killed improperly. The respective killable waits
> have been upstream for a while:
> 
>   o wait_for_completion_killable()
>   o wait_for_completion_killable_timeout()
> 
> swait has been upstream since v4.6. Older kernels have had the
> above variants in place for a long time.
> 
> Cc: stable <stable@vger.kernel.org> # 4.6
> Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
> ---
>  include/linux/swait.h | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
> 
> diff --git a/include/linux/swait.h b/include/linux/swait.h
> index c1f9c62a8a50..2c700694d50a 100644
> --- a/include/linux/swait.h
> +++ b/include/linux/swait.h
> @@ -169,4 +169,29 @@ do {									\
>  	__ret;								\
>  })
>  
> +#define __swait_event_killable(wq, condition)				\
> +	___swait_event(wq, condition, TASK_KILLABLE, 0, schedule())
> +
> +#define swait_event_killable(wq, condition)				\
> +({									\
> +	int __ret = 0;							\
> +	if (!(condition))						\
> +		__ret = __swait_event_killable(wq, condition);		\
> +	__ret;								\
> +})
> +
> +#define __swait_event_killable_timeout(wq, condition, timeout)		\
> +	___swait_event(wq, ___wait_cond_timeout(condition),		\
> +		       TASK_KILLABLE, timeout,				\
> +		       __ret = schedule_timeout(__ret))
> +
> +#define swait_event_killable_timeout(wq, condition, timeout)		\
> +({									\
> +	long __ret = timeout;						\
> +	if (!___wait_cond_timeout(condition))				\
> +		__ret = __swait_event_killable_timeout(wq,		\
> +						condition, timeout);	\
> +	__ret;								\
> +})
> +
>  #endif /* _LINUX_SWAIT_H */

Do you really still want to add these, now that we know we shouldn't be
using swait in "real" code? :)

thanks,

greg k-h

[toc] | [next] | [standalone]

#1677740

On Thu, 29 Jun 2017, Greg KH wrote:
> On Wed, Jun 14, 2017 at 03:20:15PM -0700, Luis R. Rodriguez wrote:
> Do you really still want to add these, now that we know we shouldn't be
> using swait in "real" code? :)

And who defined that it should not be used in real code?

Thanks,

	tglx

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

#1677760

On Thu, Jun 29, 2017 at 03:05:26PM +0200, Thomas Gleixner wrote:
> On Thu, 29 Jun 2017, Greg KH wrote:
> > On Wed, Jun 14, 2017 at 03:20:15PM -0700, Luis R. Rodriguez wrote:
> > Do you really still want to add these, now that we know we shouldn't be
> > using swait in "real" code? :)
> 
> And who defined that it should not be used in real code?

Linus did, in a different firmware thread.  You have to _really_ know
what you are doing to use this interface, and the firmware interface
shouldn't be using it.  So adding new apis just for firmware does not
seem like a wise decision at this point in time.

thanks,

greg k-h

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

#1677766

On Thu, 29 Jun 2017, Greg KH wrote:
> On Thu, Jun 29, 2017 at 03:05:26PM +0200, Thomas Gleixner wrote:
> > On Thu, 29 Jun 2017, Greg KH wrote:
> > > On Wed, Jun 14, 2017 at 03:20:15PM -0700, Luis R. Rodriguez wrote:
> > > Do you really still want to add these, now that we know we shouldn't be
> > > using swait in "real" code? :)
> > 
> > And who defined that it should not be used in real code?
> 
> Linus did, in a different firmware thread.  You have to _really_ know
> what you are doing to use this interface, and the firmware interface
> shouldn't be using it.  So adding new apis just for firmware does not
> seem like a wise decision at this point in time.

So it's not about code in general, it's about a particular piece of
code. Fair enough.

Thanks,

	tglx

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

#1677902

On Thu, Jun 29, 2017 at 6:46 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
> On Thu, 29 Jun 2017, Greg KH wrote:
>> On Thu, Jun 29, 2017 at 03:05:26PM +0200, Thomas Gleixner wrote:
>> >
>> > And who defined that it should not be used in real code?
>>
>> Linus did, in a different firmware thread.  You have to _really_ know
>> what you are doing to use this interface, and the firmware interface
>> shouldn't be using it.  So adding new apis just for firmware does not
>> seem like a wise decision at this point in time.
>
> So it's not about code in general, it's about a particular piece of
> code. Fair enough.

Well, I'd actually say it the other way around: swait should not be
used in general, only in _very_ particular pieces of code that
actually explicitly need the odd swait semantics.

swait uses special locking and has odd semantics that are not at all
the same as the default wait queue ones. It should not be used without
very strong reasons (and honestly, the only strong enough reason seems
to be "RT").

The special locking means that swait doesn't do DEBUG_LOCK_ALLOC, but
it also means that it doesn't even work in all contexts.

So "swake_up()" does surprising things (only wake up one - that's what
caused a firmware loading bug), and "swake_up_all()" has magic rules
about interrupt disabling.

The thing is simply a collection of small hacks and should NOT be used
in general.

I never want to see a driver use that code, for example. It was
designed for RCU and RT, and it should damn well be limited to that.

              Linus

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

#1677994

On Thu, Jun 29, 2017 at 04:31:08PM +0000, Matthew Wilcox wrote:
> Linus Torvalds wrote:
> > The thing is simply a collection of small hacks and should NOT be used
> > in general.
> > 
> > I never want to see a driver use that code, for example. It was
> > designed for RCU and RT, and it should damn well be limited to that.
> 
> Maybe put #ifndef MODULE around the entire include file then?
> And definitely remove this line:
> 
> * One would recommend using this wait queue where possible.

I'll respin and port away from swait, and add the #ifndef MODULE and special
note on swait.h.

  Luis

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

#1677999

On Thu, 29 Jun 2017, Linus Torvalds wrote:

>Well, I'd actually say it the other way around: swait should not be
>used in general, only in _very_ particular pieces of code that
>actually explicitly need the odd swait semantics.
>
>swait uses special locking and has odd semantics that are not at all
>the same as the default wait queue ones. It should not be used without
>very strong reasons (and honestly, the only strong enough reason seems
>to be "RT").
>
>The special locking means that swait doesn't do DEBUG_LOCK_ALLOC, but
>it also means that it doesn't even work in all contexts.
>
>So "swake_up()" does surprising things (only wake up one - that's what
>caused a firmware loading bug), and "swake_up_all()" has magic rules
>about interrupt disabling.
>
>The thing is simply a collection of small hacks and should NOT be used
>in general.

For all the above, what do you think of my 'sswait' proposal?

Thanks,
Davidlohr

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

#1678005

On Thu, Jun 29, 2017 at 10:40 AM, Davidlohr Bueso <dave@stgolabs.net> wrote:
>
> For all the above, what do you think of my 'sswait' proposal?

I see no actual users of such a specialty interface.

I don't think we've *ever* had any actual problems with our current
wait-queues, apart from the RT issues, which were not about the
waitqueues themselves, but purely about RT itself.

So without some very compelling reason, I'd not want to add yet
another wait-queue.

I actually think swait is pure garbage. Most users only wake up one
process anyway, and using swait for that is stupid. If you only wake
up one, you might as well just have a single process pointer, not a
wait list at all, and then use "wake_up_process()".

There is *one* single user of swake_up_all(), and that one looks like
bogus crap also: it does it outside of the spinlock that could have
been used to protect the queue - p,lus I'm not sure there's really a
queue anyway, since I think it's just the grace-period kthread that is
there.

kvm uses swait, but doesn't use swake_up_all(), so it always just
wakes up a single waiter. That may be the right thing to do. Or it's
just another bug. I don't know. The KVM use looks broken too, since it
does

        if (swait_active(wqp)) {
                swake_up(wqp);

which is racy wrt new waiters, which implies that there is some
upper-level synchronization - possibly the same "only one thread
anyway".

So swake really looks like crap. It has crap semantics, it has crap
users, it's just broken.

The last thing we want to do is to create something _else_ specialized
like this.

                Linus

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

#1678050

On Thu, 29 Jun 2017, Linus Torvalds wrote:

>So without some very compelling reason, I'd not want to add yet
>another wait-queue.

Yes, I was expecting this and very much agree.

I'll actually take a look at wake_q for wake_up_all() and co. to see if
we can reduce the spinlock hold times. Of course it would only make sense
for more than a one wakeup.

>I actually think swait is pure garbage. Most users only wake up one
>process anyway, and using swait for that is stupid. If you only wake
>up one, you might as well just have a single process pointer, not a
>wait list at all, and then use "wake_up_process()".

But you still need the notion of a queue, even if you wake one task
at a time... I'm probably missing your point here.

>There is *one* single user of swake_up_all(), and that one looks like
>bogus crap also: it does it outside of the spinlock that could have
>been used to protect the queue - p,lus I'm not sure there's really a
>queue anyway, since I think it's just the grace-period kthread that is
>there.

So those cases when there's only one waiter I completely agree should
not be using waitqueues. pcpu-rwsems in the past suffered from this.

Thanks,
Davidlohr

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

#1678067

On Thu, Jun 29, 2017 at 11:33 AM, Davidlohr Bueso <dave@stgolabs.net> wrote:
> On Thu, 29 Jun 2017, Linus Torvalds wrote:
>
>> I actually think swait is pure garbage. Most users only wake up one
>> process anyway, and using swait for that is stupid. If you only wake
>> up one, you might as well just have a single process pointer, not a
>> wait list at all, and then use "wake_up_process()".
>
> But you still need the notion of a queue, even if you wake one task
> at a time... I'm probably missing your point here.

The *reason* they wake up only one seems to be that there really is
just one. It's some per-cpu idle thread for kvm, and for RCU it's the
RCU workqueue thread.

So the queue literally looks suspiciously pointless.

But I might be wrong, and there can actually be multiple entries. If
there are, I don't see why the wake-up-one semantics the code uses
would be valid, though.

                   Linus

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

#1678134

On Thu, Jun 29, 2017 at 11:59:29AM -0700, Linus Torvalds wrote:
> On Thu, Jun 29, 2017 at 11:33 AM, Davidlohr Bueso <dave@stgolabs.net> wrote:
> > On Thu, 29 Jun 2017, Linus Torvalds wrote:
> >
> >> I actually think swait is pure garbage. Most users only wake up one
> >> process anyway, and using swait for that is stupid. If you only wake
> >> up one, you might as well just have a single process pointer, not a
> >> wait list at all, and then use "wake_up_process()".
> >
> > But you still need the notion of a queue, even if you wake one task
> > at a time... I'm probably missing your point here.
> 
> The *reason* they wake up only one seems to be that there really is
> just one. It's some per-cpu idle thread for kvm, and for RCU it's the
> RCU workqueue thread.
> 
> So the queue literally looks suspiciously pointless.
> 
> But I might be wrong, and there can actually be multiple entries.

Since this swake_up() --> swake_up_all() reportedly *fixed* the one wake up
issue it would seem this does queue [0]. That said, I don't see any simple tests
tools/testing/selftests/swait but then again we don't have test for regular
waits either...

[0] https://bugzilla.kernel.org/show_bug.cgi?id=195477

> If there are, I don't see why the wake-up-one semantics the code uses
> would be valid, though.

Not sure what's wrong with it?

I believe one use case for example is for when we know that waker alone would
be able to ensure the next item in queue will also be woken up. Such was the
case for the kmod.c conversion I tested, and behold it seemed to have wored
with just swake_up(). Its obviously *fragile* though given you *assume* error
cases also wake up. In the case of kmod.c we have no such error cases but in
firmware_class.c we *do*, and actually that is part of the next set of fixes I
have to address next, but that issue would be present even if we move to wait
for completion and complete_all() is used.

  Luis

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

#1678137

On Thu, Jun 29, 2017 at 09:40:15PM +0200, Luis R. Rodriguez wrote:
> On Thu, Jun 29, 2017 at 11:59:29AM -0700, Linus Torvalds wrote:
> > On Thu, Jun 29, 2017 at 11:33 AM, Davidlohr Bueso <dave@stgolabs.net> wrote:
> > > On Thu, 29 Jun 2017, Linus Torvalds wrote:
> > >
> > >> I actually think swait is pure garbage. Most users only wake up one
> > >> process anyway, and using swait for that is stupid. If you only wake
> > >> up one, you might as well just have a single process pointer, not a
> > >> wait list at all, and then use "wake_up_process()".
> > >
> > > But you still need the notion of a queue, even if you wake one task
> > > at a time... I'm probably missing your point here.
> > 
> > The *reason* they wake up only one seems to be that there really is
> > just one. It's some per-cpu idle thread for kvm, and for RCU it's the
> > RCU workqueue thread.
> > 
> > So the queue literally looks suspiciously pointless.
> > 
> > But I might be wrong, and there can actually be multiple entries.
> 
> Since this swake_up() --> swake_up_all() reportedly *fixed* the one wake up
> issue it would seem this does queue [0]. That said, I don't see any simple tests
> tools/testing/selftests/swait but then again we don't have test for regular
> waits either...
> 
> [0] https://bugzilla.kernel.org/show_bug.cgi?id=195477

I should also note that the swake_up_all() should have only helped in cases where
3 cards were used, as if only 2 were used that should have been covered by just
the swake_up(). Unless of course I hear otherwise by the reporter, Nicolas or
from Jakub.

  Luis

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

#1678191

On Thu, 29 Jun 2017 21:44:55 +0200, Luis R. Rodriguez wrote:
> On Thu, Jun 29, 2017 at 09:40:15PM +0200, Luis R. Rodriguez wrote:
> > On Thu, Jun 29, 2017 at 11:59:29AM -0700, Linus Torvalds wrote:  
> > > On Thu, Jun 29, 2017 at 11:33 AM, Davidlohr Bueso <dave@stgolabs.net> wrote:  
> > > > On Thu, 29 Jun 2017, Linus Torvalds wrote:
> > > >  
> > > >> I actually think swait is pure garbage. Most users only wake up one
> > > >> process anyway, and using swait for that is stupid. If you only wake
> > > >> up one, you might as well just have a single process pointer, not a
> > > >> wait list at all, and then use "wake_up_process()".  
> > > >
> > > > But you still need the notion of a queue, even if you wake one task
> > > > at a time... I'm probably missing your point here.  
> > > 
> > > The *reason* they wake up only one seems to be that there really is
> > > just one. It's some per-cpu idle thread for kvm, and for RCU it's the
> > > RCU workqueue thread.
> > > 
> > > So the queue literally looks suspiciously pointless.
> > > 
> > > But I might be wrong, and there can actually be multiple entries.  
> > 
> > Since this swake_up() --> swake_up_all() reportedly *fixed* the one wake up
> > issue it would seem this does queue [0]. That said, I don't see any simple tests
> > tools/testing/selftests/swait but then again we don't have test for regular
> > waits either...
> > 
> > [0] https://bugzilla.kernel.org/show_bug.cgi?id=195477  
> 
> I should also note that the swake_up_all() should have only helped in cases where
> 3 cards were used, as if only 2 were used that should have been covered by just
> the swake_up(). Unless of course I hear otherwise by the reporter, Nicolas or
> from Jakub.

I was hitting this with 2 cards.

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

#1678259

On Fri, 30 Jun 2017 00:50:03 +0200, Luis R. Rodriguez wrote:
> On Thu, Jun 29, 2017 at 01:58:22PM -0700, Jakub Kicinski wrote:
> > On Thu, 29 Jun 2017 21:44:55 +0200, Luis R. Rodriguez wrote:  
> > > > Since this swake_up() --> swake_up_all() reportedly *fixed* the one wake up
> > > > issue it would seem this does queue [0]. That said, I don't see any simple tests
> > > > tools/testing/selftests/swait but then again we don't have test for regular
> > > > waits either...
> > > > 
> > > > [0] https://bugzilla.kernel.org/show_bug.cgi?id=195477    
> > > 
> > > I should also note that the swake_up_all() should have only helped in cases where
> > > 3 cards were used, as if only 2 were used that should have been covered by just
> > > the swake_up(). Unless of course I hear otherwise by the reporter, Nicolas or
> > > from Jakub.  
> > 
> > I was hitting this with 2 cards.  
> 
> Thanks!
> 
> Thing is I'm not convinced the issue with 2 cards was the swake_up() Vs
> swake_up_all() in this case though. Let's recall also the missing wake up on
> errors! And the fact that netronome has optional firmware, which naturally can
> fail.
> 
> So could the issue with 2 cards instead of the miss of a wake up on error due
> to batched requests ? If so then that still would not put blame on the
> swake_up()!

Sorry, that was during manual tests.  I had the driver request the
firmware with _nowait() without an uevent, and then after I manually
wrote -1 to loading only one would get woken up.

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

#1678276

On Thu, 29 Jun 2017 16:00:32 -0700, Luis R. Rodriguez wrote:
> On Thu, Jun 29, 2017 at 3:53 PM, Jakub Kicinski
> <jakub.kicinski@netronome.com> wrote:
> > On Fri, 30 Jun 2017 00:50:03 +0200, Luis R. Rodriguez wrote:  
> >> On Thu, Jun 29, 2017 at 01:58:22PM -0700, Jakub Kicinski wrote:  
> >> > On Thu, 29 Jun 2017 21:44:55 +0200, Luis R. Rodriguez wrote:  
> >> > > > Since this swake_up() --> swake_up_all() reportedly *fixed* the one wake up
> >> > > > issue it would seem this does queue [0]. That said, I don't see any simple tests
> >> > > > tools/testing/selftests/swait but then again we don't have test for regular
> >> > > > waits either...
> >> > > >
> >> > > > [0] https://bugzilla.kernel.org/show_bug.cgi?id=195477  
> >> > >
> >> > > I should also note that the swake_up_all() should have only helped in cases where
> >> > > 3 cards were used, as if only 2 were used that should have been covered by just
> >> > > the swake_up(). Unless of course I hear otherwise by the reporter, Nicolas or
> >> > > from Jakub.  
> >> >
> >> > I was hitting this with 2 cards.  
> >>
> >> Thanks!
> >>
> >> Thing is I'm not convinced the issue with 2 cards was the swake_up() Vs
> >> swake_up_all() in this case though. Let's recall also the missing wake up on
> >> errors! And the fact that netronome has optional firmware, which naturally can
> >> fail.
> >>
> >> So could the issue with 2 cards instead of the miss of a wake up on error due
> >> to batched requests ? If so then that still would not put blame on the
> >> swake_up()!  
> >
> > Sorry, that was during manual tests.  I had the driver request the
> > firmware with _nowait() without an uevent,  
> 
> Can you provide the output of
> 
> grep  CONFIG_FW_LOADER_USER_HELPER .config
> 
> I'd like to see if CONFIG_FW_LOADER_USER_HELPER_FALLBACK was disabled.
> Not to be confused with the CONFIG_FW_LOADER_USER_HELPER.
> 
> When the uevent is not set its also known as "a custom fallback
> mechanism" and by default that has set a timeout of
> MAX_SCHEDULE_TIMEOUT, which means that even if the direct filesystem
> lookup failed the fallback mechanism would be used and just sit and
> wait for what seems to be forever until user input was provided.

FWIW

CONFIG_FW_LOADER=y
CONFIG_FW_LOADER_USER_HELPER=y
CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y
# CONFIG_FW_CFG_SYSFS is not set

> > and then after I manually
> > wrote -1 to loading only one would get woken up.  
> 
> Great, this helps, so for those not familiar with the firmware sysfs
> fallback mechanism:
> 
> The relevant values one could use are:
> 
> 1: Start a load, discarding any previous partial load.
> 0: Conclude the load and hand the data to the driver code.
> -1: Conclude the load with an error and discard any written data.
> 
> So you are triggering a failure. And indeed I expect the patch I just
> provided to be the one to fix your issue with 2 cards.

Cool, thanks.

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

#1678278

On Thu, Jun 29, 2017 at 3:53 PM, Jakub Kicinski
<jakub.kicinski@netronome.com> wrote:
> On Fri, 30 Jun 2017 00:50:03 +0200, Luis R. Rodriguez wrote:
>> On Thu, Jun 29, 2017 at 01:58:22PM -0700, Jakub Kicinski wrote:
>> > On Thu, 29 Jun 2017 21:44:55 +0200, Luis R. Rodriguez wrote:
>> > > > Since this swake_up() --> swake_up_all() reportedly *fixed* the one wake up
>> > > > issue it would seem this does queue [0]. That said, I don't see any simple tests
>> > > > tools/testing/selftests/swait but then again we don't have test for regular
>> > > > waits either...
>> > > >
>> > > > [0] https://bugzilla.kernel.org/show_bug.cgi?id=195477
>> > >
>> > > I should also note that the swake_up_all() should have only helped in cases where
>> > > 3 cards were used, as if only 2 were used that should have been covered by just
>> > > the swake_up(). Unless of course I hear otherwise by the reporter, Nicolas or
>> > > from Jakub.
>> >
>> > I was hitting this with 2 cards.
>>
>> Thanks!
>>
>> Thing is I'm not convinced the issue with 2 cards was the swake_up() Vs
>> swake_up_all() in this case though. Let's recall also the missing wake up on
>> errors! And the fact that netronome has optional firmware, which naturally can
>> fail.
>>
>> So could the issue with 2 cards instead of the miss of a wake up on error due
>> to batched requests ? If so then that still would not put blame on the
>> swake_up()!
>
> Sorry, that was during manual tests.  I had the driver request the
> firmware with _nowait() without an uevent,

Can you provide the output of

grep  CONFIG_FW_LOADER_USER_HELPER .config

I'd like to see if CONFIG_FW_LOADER_USER_HELPER_FALLBACK was disabled.
Not to be confused with the CONFIG_FW_LOADER_USER_HELPER.

When the uevent is not set its also known as "a custom fallback
mechanism" and by default that has set a timeout of
MAX_SCHEDULE_TIMEOUT, which means that even if the direct filesystem
lookup failed the fallback mechanism would be used and just sit and
wait for what seems to be forever until user input was provided.

> and then after I manually
> wrote -1 to loading only one would get woken up.

Great, this helps, so for those not familiar with the firmware sysfs
fallback mechanism:

The relevant values one could use are:

1: Start a load, discarding any previous partial load.
0: Conclude the load and hand the data to the driver code.
-1: Conclude the load with an error and discard any written data.

So you are triggering a failure. And indeed I expect the patch I just
provided to be the one to fix your issue with 2 cards.

  Luis

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

#1678267

On Thu, Jun 29, 2017 at 01:58:22PM -0700, Jakub Kicinski wrote:
> On Thu, 29 Jun 2017 21:44:55 +0200, Luis R. Rodriguez wrote:
> > > Since this swake_up() --> swake_up_all() reportedly *fixed* the one wake up
> > > issue it would seem this does queue [0]. That said, I don't see any simple tests
> > > tools/testing/selftests/swait but then again we don't have test for regular
> > > waits either...
> > > 
> > > [0] https://bugzilla.kernel.org/show_bug.cgi?id=195477  
> > 
> > I should also note that the swake_up_all() should have only helped in cases where
> > 3 cards were used, as if only 2 were used that should have been covered by just
> > the swake_up(). Unless of course I hear otherwise by the reporter, Nicolas or
> > from Jakub.
> 
> I was hitting this with 2 cards.

Thanks!

Thing is I'm not convinced the issue with 2 cards was the swake_up() Vs
swake_up_all() in this case though. Let's recall also the missing wake up on
errors! And the fact that netronome has optional firmware, which naturally can
fail.

So could the issue with 2 cards instead of the miss of a wake up on error due
to batched requests ? If so then that still would not put blame on the
swake_up()!

We can find out by you testing the series I just posted [0] and if that did not
fix the issue then try this patch, which I do expect to actually have fixed
most issues considering optional firmware.

ie, I expect the combination of both to fix your issues, not just the last
series I just posted [0]. If you want this in git form you can find all of
the patches bundled on the 20170629-fw-fixes-wait-v4 branch [1]. I just
wrote this patch it but it seems to have not broken the tests:

$ sudo tools/testing/selftests/firmware/fw_fallback.sh 
tools/testing/selftests/firmware/fw_fallback.sh: timeout works
tools/testing/selftests/firmware/fw_fallback.sh: firmware comparison works
tools/testing/selftests/firmware/fw_fallback.sh: fallback mechanism works
tools/testing/selftests/firmware/fw_fallback.sh: cancelling fallback mechanism works
tools/testing/selftests/firmware/fw_fallback.sh: custom fallback loading mechanism works
tools/testing/selftests/firmware/fw_fallback.sh: cancelling custom fallback mechanism works
tools/testing/selftests/firmware/fw_fallback.sh: SIGCHLD on sync ignored as expected

$ sudo tools/testing/selftests/firmware/fw_filesystem.sh 
tools/testing/selftests/firmware/fw_filesystem.sh: timeout works
tools/testing/selftests/firmware/fw_filesystem.sh: filesystem loading works
tools/testing/selftests/firmware/fw_filesystem.sh: async filesystem loading works

[0] https://lkml.kernel.org/r/20170629205151.5329-1-mcgrof@kernel.org
[1] https://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux-next.git/log/?h=20170629-fw-fixes-wait-v4

 Luis

From cb7fee12c6d539405793e883dfd79e0b21c2baad Mon Sep 17 00:00:00 2001
From: "Luis R. Rodriguez" <mcgrof@kernel.org>
Date: Thu, 29 Jun 2017 15:19:04 -0700
Subject: [RFT] firmware: send wake up on failure for batched requests

Fix batched requests from waiting forever on failure.

The firmware API supports "batched requests" which means requests with
the same name share the same lookup effort. They wait for the first
request to complete, however they are set to always wait for what seem
to be forever (MAX_SCHEDULE_TIMEOUT).

We currently handle informing waited batched requests on success but we
never seem to have sent smoke signals of any kind on failure! This
should mean secondary requests batched in seem to just wait forever when
the request fails.

For device drivers with optional firmware schemes (Intel, or Netronome),
this could mean that when you boot a system with multiple cards the
firmware will seem to never load on the system, or that the card is just
not responsive even the driver initialized. Due to differences in scheduling
possible this should not always trigger, so triggering batched requests
actually needs to be triggered for this to be an issue.

Its reported that at least with the Intel WiFi cards on one system this
issue was creeping up 50% of the boots [0].

[0] https://bugzilla.kernel.org/show_bug.cgi?id=195477

Reported-by: Nicolas <nbroeking@me.com>
Reported-by: John Ewalt  <jewalt@lgsinnovations.com>
Reported-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
 drivers/base/firmware_class.c | 40 ++++++++++++++++++++++++++++++++--------
 1 file changed, 32 insertions(+), 8 deletions(-)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index d3d071dbd2a5..40d1351660c0 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -152,28 +152,27 @@ static void __fw_state_set(struct fw_state *fw_st,
 	__fw_state_set(fw_st, FW_STATUS_LOADING)
 #define fw_state_done(fw_st)					\
 	__fw_state_set(fw_st, FW_STATUS_DONE)
+#define fw_state_aborted(fw_st)					\
+	__fw_state_set(fw_st, FW_STATUS_ABORTED)
 #define fw_state_wait(fw_st)					\
 	__fw_state_wait_common(fw_st, MAX_SCHEDULE_TIMEOUT)
 
-#ifndef CONFIG_FW_LOADER_USER_HELPER
-
-#define fw_state_is_aborted(fw_st)	false
-
-#else /* CONFIG_FW_LOADER_USER_HELPER */
-
 static int __fw_state_check(struct fw_state *fw_st, enum fw_status status)
 {
 	return fw_st->status == status;
 }
 
+#define fw_state_is_aborted(fw_st)				\
+	__fw_state_check(fw_st, FW_STATUS_ABORTED)
+
+#ifdef CONFIG_FW_LOADER_USER_HELPER
+
 #define fw_state_aborted(fw_st)					\
 	__fw_state_set(fw_st, FW_STATUS_ABORTED)
 #define fw_state_is_done(fw_st)					\
 	__fw_state_check(fw_st, FW_STATUS_DONE)
 #define fw_state_is_loading(fw_st)				\
 	__fw_state_check(fw_st, FW_STATUS_LOADING)
-#define fw_state_is_aborted(fw_st)				\
-	__fw_state_check(fw_st, FW_STATUS_ABORTED)
 #define fw_state_wait_timeout(fw_st, timeout)			\
 	__fw_state_wait_common(fw_st, timeout)
 
@@ -332,6 +331,7 @@ static struct firmware_buf *__fw_lookup_buf(const char *fw_name)
 	return NULL;
 }
 
+/* Returns 1 for batching firmware requests with the same name */
 static int fw_lookup_and_allocate_buf(const char *fw_name,
 				      struct firmware_cache *fwc,
 				      struct firmware_buf **buf, void *dbuf,
@@ -345,6 +345,7 @@ static int fw_lookup_and_allocate_buf(const char *fw_name,
 		kref_get(&tmp->ref);
 		spin_unlock(&fwc->lock);
 		*buf = tmp;
+		/* requests are batched ! */
 		return 1;
 	}
 	tmp = __allocate_fw_buf(fw_name, fwc, dbuf, size);
@@ -1200,6 +1201,28 @@ _request_firmware_prepare(struct firmware **firmware_p, const char *name,
 	return 1; /* need to load */
 }
 
+/*
+ * Batched requests need only one wake, we need to do this step last due to the
+ * fallback mechanism. The buf is protected with kref_get(), and it won't be
+ * released until the last user calls release_firmware().
+ *
+ * Failed batched requests are possible as well, in such cases we just share
+ * the struct firmware_buf and won't release it until all requests are woken
+ * and have gone through this same path.
+ */
+static void fw_abort_batch_reqs(struct firmware *fw)
+{
+	struct firmware_buf *buf;
+
+	/* Loaded directly? */
+	if (!fw || !fw->priv)
+		return;
+
+	buf = fw->priv;
+	if (!fw_state_is_aborted(&buf->fw_st))
+		fw_state_aborted(&buf->fw_st);
+}
+
 /* called from request_firmware() and request_firmware_work_func() */
 static int
 _request_firmware(const struct firmware **firmware_p, const char *name,
@@ -1243,6 +1266,7 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
 
  out:
 	if (ret < 0) {
+		fw_abort_batch_reqs(fw);
 		release_firmware(fw);
 		fw = NULL;
 	}
-- 
2.11.0

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

#1678195

On Thu, Jun 29, 2017 at 12:40 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
> On Thu, Jun 29, 2017 at 11:59:29AM -0700, Linus Torvalds wrote:
>> > at a time... I'm probably missing your point here.
>>
>> The *reason* they wake up only one seems to be that there really is
>> just one. It's some per-cpu idle thread for kvm, and for RCU it's the
>> RCU workqueue thread.
>>
>> So the queue literally looks suspiciously pointless.
>>
>> But I might be wrong, and there can actually be multiple entries.
>
> Since this swake_up() --> swake_up_all() reportedly *fixed* the one wake up
> issue it would seem this does queue [0].

I'm not talking about the firmware code.

That thing never had an excuse to use swait in the first place.

I'm talking about kvm and rcu, which *do* have excuses to use it, but
where I argue that swait is _still_ a questionable interface for other
reasons.

                Linus

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

#1678412

On Thu, Jun 29, 2017 at 09:13:29AM -0700, Linus Torvalds wrote:
> On Thu, Jun 29, 2017 at 6:46 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
> > On Thu, 29 Jun 2017, Greg KH wrote:
> >> On Thu, Jun 29, 2017 at 03:05:26PM +0200, Thomas Gleixner wrote:
> >> >
> >> > And who defined that it should not be used in real code?
> >>
> >> Linus did, in a different firmware thread.  You have to _really_ know
> >> what you are doing to use this interface, and the firmware interface
> >> shouldn't be using it.  So adding new apis just for firmware does not
> >> seem like a wise decision at this point in time.
> >
> > So it's not about code in general, it's about a particular piece of
> > code. Fair enough.
> 
> Well, I'd actually say it the other way around: swait should not be
> used in general, only in _very_ particular pieces of code that
> actually explicitly need the odd swait semantics.
> 
> swait uses special locking and has odd semantics that are not at all
> the same as the default wait queue ones. It should not be used without
> very strong reasons (and honestly, the only strong enough reason seems
> to be "RT").

Performance shortcut:

https://lkml.org/lkml/2016/2/25/301

> The special locking means that swait doesn't do DEBUG_LOCK_ALLOC, but
> it also means that it doesn't even work in all contexts.
> 
> So "swake_up()" does surprising things (only wake up one - that's what
> caused a firmware loading bug), and "swake_up_all()" has magic rules
> about interrupt disabling.
> 
> The thing is simply a collection of small hacks and should NOT be used
> in general.

Its a very smart performance speed up ;-)

> I never want to see a driver use that code, for example. It was
> designed for RCU and RT, and it should damn well be limited to that.
> 
>               Linus

If KVM is the only user, feel free to remove it, you're past the point
where that performance improvement matters (due to VMX hardware
improvements).

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

#1678453

On Thu, Jun 29, 2017 at 12:15 PM, Marcelo Tosatti <mtosatti@redhat.com> wrote:
> On Thu, Jun 29, 2017 at 09:13:29AM -0700, Linus Torvalds wrote:
>>
>> swait uses special locking and has odd semantics that are not at all
>> the same as the default wait queue ones. It should not be used without
>> very strong reasons (and honestly, the only strong enough reason seems
>> to be "RT").
>
> Performance shortcut:
>
> https://lkml.org/lkml/2016/2/25/301

Yes, I know why kvm uses it, I just don't think it's necessarily the
right thing.

That kvm commit is actually a great example: it uses swake_up() from
an interrupt, and that's in fact the *reason* it uses swake_up().

But that also fundamentally means that it cannot use swake_up_all(),
so it basically *relies* on there only ever being one single entry
that needs to be woken up.

And as far as I can tell, it really is because the queue only ever has
one entry (ie it's per-vcpu, and when the vcpu is blocked, it's
blocked - so no other user will be waiting there).

So it isn't that you migth queue multiple entries and then just wake
them up one at a time. There really is just one entry at a time,
right?

And that means that swait is actuially completely the wrong thing to
do. It's more expensive and more complex than just saving the single
process pointer away and just doing "wake_up_process()".

Now, it really is entirely possible that I'm missing something, but it
does look like that to me.

We've had wake_up_process() since pretty much day #1. THAT is the
fastest and simplest direct wake-up there is, not some "simple
wait-queue".

Now, admittedly I don't know the code and really may be entirely off,
but looking at the commit (no need to go to the lkml archives - it's
commit 8577370fb0cb ("KVM: Use simple waitqueue for vcpu->wq") in
mainline), I really think the swait() use is simply not correct if
there can be multiple waiters, exactly because swake_up() only wakes
up a single entry.

So either there is only a single entry, or *all* the code like

        dvcpu->arch.wait = 0;

-       if (waitqueue_active(&dvcpu->wq))
-               wake_up_interruptible(&dvcpu->wq);
+       if (swait_active(&dvcpu->wq))
+               swake_up(&dvcpu->wq);

is simply wrong. If there are multiple blockers, and you just cleared
"arch.wait", I think they should *all* be woken up. And that's not
what swake_up() does.

So I think that kvm_vcpu_block() could easily have instead done

    vcpu->process = current;

as the "prepare_to_wait()" part, and "finish_wait()" would be to just
clear vcpu->process. No wait-queue, just a single pointer to the
single blocking thread.

(Of course, you still need serialization, so that
"wake_up_process(vcpu->process)" doesn't end up using a stale value,
but since processes are already freed with RCU because of other things
like that, the serialization is very low-cost, you only need to be
RCU-read safe when waking up).

See what I'm saying?

Note that "wake_up_process()" really is fairly widely used. It's
widely used because it's fairly obvious, and because that really *is*
the lowest-possible cost: a single pointer to the sleeping thread, and
you can often do almost no locking at all.

And unlike swake_up(), it's obvious that you only wake up a single thread.

           Linus

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

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web