Date: Tue, 20 Feb 2018 18:27:11 +0200 From: Konstantin Belousov <kib@freebsd.org> To: Mateusz Guzik <mjguzik@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: <20180220162711.GK94212@kib.kiev.ua> In-Reply-To: <CAGudoHEmEDZYnCspAW_-XxoKFJb-7tivXYXZpsi_anxWLWRLyQ@mail.gmail.com> References: <201802201052.w1KAq7jQ057924@repo.freebsd.org> <2A592C68-C6B3-4BAA-975C-02D325292C02@lists.zabbadoz.net> <20180220114241.GH94212@kib.kiev.ua> <CAGudoHEmEDZYnCspAW_-XxoKFJb-7tivXYXZpsi_anxWLWRLyQ@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Tue, Feb 20, 2018 at 05:10:52PM +0100, Mateusz Guzik wrote: > 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? The p_state field is read unlocked and it is possible to not see the last update to it. I believe that it must be re-checked after the proc lock is acquired. > > 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? p_state is declared to be locked by the proc lock (and proc slock, but I think this is a comment bug). Relying on occational allproc protection is wrong, or re-declare it as being double-protected both by proc lock and allproc. > > -- > Mateusz Guzik <mjguzik gmail.com>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20180220162711.GK94212>