Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753269AbdF2Tk3 (ORCPT ); Thu, 29 Jun 2017 15:40:29 -0400 Received: from mx2.suse.de ([195.135.220.15]:47064 "EHLO mx1.suse.de" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751612AbdF2TkU (ORCPT ); Thu, 29 Jun 2017 15:40:20 -0400 Date: Thu, 29 Jun 2017 21:40:15 +0200 From: "Luis R. Rodriguez" To: Linus Torvalds Cc: Davidlohr Bueso , Thomas Gleixner , Greg KH , "Luis R. Rodriguez" , mfuzzey@parkeon.com, "Eric W. Biederman" , Dmitry Torokhov , Daniel Wagner , David Woodhouse , jewalt@lgsinnovations.com, rafal@milecki.pl, Arend Van Spriel , "Rafael J. Wysocki" , "Li, Yi" , atull@kernel.org, Moritz Fischer , Petr Mladek , Johannes Berg , Emmanuel Grumbach , "Coelho, Luciano" , Kalle Valo , Andrew Lutomirski , Kees Cook , "AKASHI, Takahiro" , David Howells , Peter Jones , Hans de Goede , Alan Cox , "Theodore Ts'o" , Michael Kerrisk , Paul Gortmaker , Marcelo Tosatti , Matthew Wilcox , Linux API , linux-fsdevel , Linux Kernel Mailing List , "stable # 4 . 6" Subject: Re: [PATCH 2/4] swait: add the missing killable swaits Message-ID: <20170629194015.GQ21846@wotan.suse.de> References: <20170614222017.14653-3-mcgrof@kernel.org> <20170629125402.GH26046@kroah.com> <20170629133530.GA14747@kroah.com> <20170629174046.GC3954@linux-80c1.suse> <20170629183339.GD3954@linux-80c1.suse> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.6.0 (2016-04-01) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 1932 Lines: 42 On Thu, Jun 29, 2017 at 11:59:29AM -0700, Linus Torvalds wrote: > On Thu, Jun 29, 2017 at 11:33 AM, Davidlohr Bueso 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