Date: Tue, 20 Feb 2018 17:10:52 +0100 From: Mateusz Guzik <mjguzik@gmail.com> To: Konstantin Belousov <kostikbel@gmail.com> Cc: "Bjoern A. Zeeb" <bzeeb-lists@lists.zabbadoz.net>, Mateusz Guzik <mjg@freebsd.org>, src-committers <src-committers@freebsd.org>, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r329639 - head/sys/kern Message-ID: <CAGudoHEmEDZYnCspAW_-XxoKFJb-7tivXYXZpsi_anxWLWRLyQ@mail.gmail.com> 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>
next in thread | previous in thread | raw e-mail | index | archive | help
On Tue, Feb 20, 2018 at 12:42 PM, Konstantin Belousov <kostikbel@gmail.com> 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 <mjguzik gmail.com>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAGudoHEmEDZYnCspAW_-XxoKFJb-7tivXYXZpsi_anxWLWRLyQ>