svn commit: r329639 - head/sys/kern

[Mateusz Guzik]

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?

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>