svn commit: r329639 - head/sys/kern

[Konstantin Belousov]

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 at 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>