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

From: "Luis R. Rodriguez" <mcgrof@kernel.org>
To: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Davidlohr Bueso <dave@stgolabs.net>,
	Thomas Gleixner <tglx@linutronix.de>,
	Greg KH <gregkh@linuxfoundation.org>,
	"Luis R. Rodriguez" <mcgrof@kernel.org>,
	mfuzzey@parkeon.com, "Eric W. Biederman" <ebiederm@xmission.com>,
	Dmitry Torokhov <dmitry.torokhov@gmail.com>,
	Daniel Wagner <wagi@monom.org>,
	David Woodhouse <dwmw2@infradead.org>,
	jewalt@lgsinnovations.com, rafal@milecki.pl,
	Arend Van Spriel <arend.vanspriel@broadcom.com>,
	"Rafael J. Wysocki" <rjw@rjwysocki.net>,
	"Li, Yi" <yi1.li@linux.intel.com>,
	atull@kernel.org, Moritz Fischer <moritz.fischer@ettus.com>,
	Petr Mladek <pmladek@suse.com>,
	Johannes Berg <johannes.berg@intel.com>,
	Emmanuel Grumbach <emmanuel.grumbach@intel.com>,
	"Coelho, Luciano" <luciano.coelho@intel.com>,
	Kalle Valo <kvalo@codeaurora.org>,
	Andrew Lutomirski <luto@kernel.org>,
	Kees Cook <keescook@chromium.org>,
	"AKASHI, Takahiro" <takahiro.akashi@linaro.org>,
	David Howells <dhowells@redhat.com>,
	Peter Jones <pjones@redhat.com>,
	Hans de Goede <hdegoede@redhat.com>,
	Alan Cox <alan@linux.intel.com>, "Theodore Ts'o" <tytso@mit.edu>,
	Michael Kerrisk <mtk.manpages@gmail.com>,
	Paul Gortmaker <paul.gortmaker@windriver.com>,
	Marcelo Tosatti <mtosatti@redhat.com>,
	Matthew Wilcox <mawilcox@microsoft.com>,
	Linux API <linux-api@vger.kernel.org>,
	linux-fsdevel <linux-fsdevel@vger.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	"stable # 4 . 6" <stable@vger.kernel.org>
Subject: Re: [PATCH 2/4] swait: add the missing killable swaits
Date: Thu, 29 Jun 2017 21:40:15 +0200	[thread overview]
Message-ID: <20170629194015.GQ21846@wotan.suse.de> (raw)
In-Reply-To: <CA+55aFz8Mhx+A-g-5yOG-O1ZLRUR_fpeeA4iBNGH8EnDBZEdpA@mail.gmail.com>

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