Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752376AbdF3Ra5 (ORCPT ); Fri, 30 Jun 2017 13:30:57 -0400 Received: from sub5.mail.dreamhost.com ([208.113.200.129]:43509 "EHLO homiemail-a119.g.dreamhost.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752302AbdF3Rax (ORCPT ); Fri, 30 Jun 2017 13:30:53 -0400 Date: Fri, 30 Jun 2017 10:30:48 -0700 From: Krister Johansen To: Linus Torvalds Cc: Marcelo Tosatti , h@amt.cnet, Thomas Gleixner , Greg KH , "Luis R. Rodriguez" , Martin Fuzzey , "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 , 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: <20170630173048.GA2392@templeofstupid.com> References: <20170614222017.14653-1-mcgrof@kernel.org> <20170614222017.14653-3-mcgrof@kernel.org> <20170629125402.GH26046@kroah.com> <20170629133530.GA14747@kroah.com> <20170629191506.GB12368@amt.cnet> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2263 Lines: 59 On Thu, Jun 29, 2017 at 09:03:42PM -0700, Linus Torvalds wrote: > On Thu, Jun 29, 2017 at 12:15 PM, Marcelo Tosatti 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 > > 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. Code like this is probably wrong for another reason too. The swait_active() is likely redudant, since swake_up() also calls swait_active(). The check in swake_up() returns if it thinks there are no active waiters. However, the synchronization needed to ensure a proper wakeup is left as an exercise to swake_up's caller. There have been a couple of other discussions around this topic recently: https://lkml.org/lkml/2017/5/25/722 https://lkml.org/lkml/2017/6/8/1222 The above is better written as the following, but even then you still have the single/multiple wakeup problem: - if (waitqueue_active(&dvcpu->wq)) - wake_up_interruptible(&dvcpu->wq); + smp_mb(); + swake_up(&dvcpu->wq); Just to add to the confusion, the last time I checked, the semantics of swake_up() even differ between RT Linux and mainline, which makes this even more confusing. -K