From owner-svn-src-all@freebsd.org Tue Feb 20 16:10:53 2018 Return-Path: Delivered-To: svn-src-all@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 9430EF10331; Tue, 20 Feb 2018 16:10:53 +0000 (UTC) (envelope-from mjguzik@gmail.com) Received: from mail-qt0-x242.google.com (mail-qt0-x242.google.com [IPv6:2607:f8b0:400d:c0d::242]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 2EC787DDBE; Tue, 20 Feb 2018 16:10:53 +0000 (UTC) (envelope-from mjguzik@gmail.com) Received: by mail-qt0-x242.google.com with SMTP id d14so17019023qtg.1; Tue, 20 Feb 2018 08:10:53 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=/4PLTBUDbNjmERDBh3iHgv3CBQbfxYQ8HaOESp7LDe8=; b=MSgnPpTEMi5kk6+Gy3objQLNT5ftXoBlrO2T+YvPwHW3j6IumU1tsuFSfwxYEgjQOA mUiLfoCk9clr7dB+fpT9ygNdgUDR0w1FYFOVk6FuDBbJJBiCC1oTrpkuPsSawgTqI5Vm vMFx7mosrUL2Cz6h9yveG7uP1mDOHeiIjfZJ5yMY8n6E0bKdS8cpx7TgaTPF32SJ1Ke5 TeFKlo8s+lLjQV6zW2zXCvb9Fw997kR02hW0vVRR+LTqwncs4tseblWq5B33NfWtkL+i /aDXgq4gAlbeky4n6hpsVfgmgAzUaSVNe52vPsWQcUANP8AVz2JeENxb6CR4QHIjPI7r 8TlQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=/4PLTBUDbNjmERDBh3iHgv3CBQbfxYQ8HaOESp7LDe8=; b=HUm4986ZGIGCWbArn7tMCSj1DdNwBh2D8EoxfCAQInWlxW3wywcHZLPVDGJ5umj6kM ySznuioPwyIvoAaMd7h1q91/PnvNawKtVn59ooQw9GucZ1gPbf+zULDjr7NcN7p5IQwP XzhO13u8tADKYrV4gO/o0AMHE0Px8QvuK7EGSctfTWialu4afRmZxjdq24vD0O6qM0bA 8ohKAIUSSOb2/yDhzzq38BrEfUS9+5wY7jShNYlLJcyUqu/Ws0CxcT0ljBIzmE3JNtgC 6E0PAMKFcJgnIKI/BCUUl6ICcBwyQzMCwSRX4BaSY47gN5qbgktrK7srjk/50njMeYM/ Qf4A== X-Gm-Message-State: APf1xPBwN4n6MkJZoKrhOi4qlT2tBCuaJtVTNAcdiov+r/XIkPIjtbGj IZKuDgO53YronY75nFgXoy+mFv4/GBzKgTQUCy5L5A== X-Google-Smtp-Source: AH8x225sWyYT/4sPk3q1JiOwjL0FOqK0Y2LZR3oxTS/WlcIie5yErHPI8Ba93tLNhWC9DMlgmsmhqB8w4ya76Q/V7Q0= X-Received: by 10.200.16.11 with SMTP id z11mr197762qti.292.1519143052725; Tue, 20 Feb 2018 08:10:52 -0800 (PST) MIME-Version: 1.0 Received: by 10.237.35.42 with HTTP; Tue, 20 Feb 2018 08:10:52 -0800 (PST) In-Reply-To: <20180220114241.GH94212@kib.kiev.ua> References: <201802201052.w1KAq7jQ057924@repo.freebsd.org> <2A592C68-C6B3-4BAA-975C-02D325292C02@lists.zabbadoz.net> <20180220114241.GH94212@kib.kiev.ua> From: Mateusz Guzik Date: Tue, 20 Feb 2018 17:10:52 +0100 Message-ID: Subject: Re: svn commit: r329639 - head/sys/kern To: Konstantin Belousov Cc: "Bjoern A. Zeeb" , Mateusz Guzik , src-committers , svn-src-all@freebsd.org, svn-src-head@freebsd.org Content-Type: text/plain; charset="UTF-8" X-Content-Filtered-By: Mailman/MimeDel 2.1.25 X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.25 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 20 Feb 2018 16:10:53 -0000 On Tue, Feb 20, 2018 at 12:42 PM, Konstantin Belousov wrote: > On Tue, Feb 20, 2018 at 10:58:33AM +0000, Bjoern A. Zeeb wrote: > > On 20 Feb 2018, at 10:52, Mateusz Guzik wrote: > > > > > Author: mjg > > > Date: Tue Feb 20 10:52:07 2018 > > > New Revision: 329639 > > > URL: https://svnweb.freebsd.org/changeset/base/329639 > > > > > > Log: > > > Make killpg1 perform process validity checks without proc lock held. > > > > I appreciate all these locking improvements! > > > > I would feel a lot more easy about them if the commit message would also > > detail why these changes are possible (e.g. only read-only variables > > accessed, or variables only ever accessed thread local, ..) and not just > > what the change is (which the diff also tells). > Removing PRS_NEW is certainly not safe. > > As in doing unlocked? I don't see why. You mean it can't be safely tested once without the lock or it needs to be re-checked after locked? Due to allproc held at most it can transition from PRS_NEW to PRS_NORMAL. Thus if non-PRS_NEW is spotted, there is no need to recheck locked. As for racing the other way, the loop was already racy against fork. The only difference I see from that standapoint is that if the race catches the target locked, it will wait and have higher chances of seeing it fully formed. That is with previous code: fork: killpg xlock(&allproc); p->p_state = PRS_NEW; PROC_LOCK(p); xunlock(&allproc); ... PROC_UNLOCK(p); slock(&allproc); PROC_LOCK(p); if (p->p_state == PRS_NEW) /* tests true */ PROC_UNLOCK(p); sunlock(&allproc); ... PROC_LOCK(p); p->p_state = PRS_NORMAL; PROC_UNLOCK(p); For this case proc locking in killpg played no role. It can catch the process still locked from the area protected by allproc, which makes no difference in the outcome. For when it catches later: fork: killpg xlock(&allproc); p->p_state = PRS_NEW; PROC_LOCK(p); xunlock(&allproc); ... PROC_UNLOCK(p); ... PROC_LOCK(p); slock(&allproc); PROC_LOCK(p); starts.. p->p_state = PRS_NORMAL; PROC_UNLOCK(p); PROC_LOCK(p); ..finishes if (p->p_state == PRS_NEW) /* * tests false, signal * is delivered */ PROC_UNLOCK(p); sunlock(&allproc); But that was not guaranteed to begin with. The new code will simply miss this case, just like both old and new can miss it in other variations. Am I missing something here? -- Mateusz Guzik