pdwait(2): change handling of the exited processes

[kib]

Summary

Instead of accessing the struct proc and gathering data from it,
memoize the data needed for pdwait() on exited process in struct
procdesc, at the time of process termination.

This allows unlimited number of calls to pdwait(2) on procdesc for
terminated process.

Change the locking requirements for pd_flags to proctree_lock. This does
not modify the pre-patch locking regime, but the change requires it.

[kib]

Rebase after NOTE_REAP.

[markj]

sys/kern/kern_exit.c

1579

Not a new bug, but p is uninitialized here on the first iteration. It's probably enough to just print the pointer to the pd...?

1602

This breaks compatibility. So I am not sure it is acceptable, but in either case the review description does not mention this explicitly.

1606

What's the purpose of calling p_canwait() now?

sys/kern/sys_procdesc.c

301

I think this can only be called after exit1() is finished, one of the last things it does is call ruadd(&p->p_ru, &p->p_rux, &p->p_stats->p_cru, &p->p_crux), but procdesc_exit() is called earlier.

[kib]

sys/kern/kern_exit.c

1602

The change does not make sense otherwise. I think it should be considered together with D58264 in this regard.

1606

We still need to check the rights, because there we are waiting on the live process.

[kib]

Fill waitinfo in the procdesc on the first pdwait() call, or when pd_proc is cleared.
This way the last ruadd() in exit1() should be not lost.

Move the chunk that drops reaping from kern_pdwait(), into the next patch.

[markj]

In the due course: change the locking requirements for pd_flags to proctree_lock. This does not modify actual locking regime, only the annotation.

But procdesc_fill_winfo() already assumes the new locking protocol: it does not hold the procdesc mutex while modifying pd_flags.

sys/kern/kern_exit.c

1599

Can this condition happen now?

1608–1609

This condition is always false now.

1612–1613

This condition is always false now, I believe.

sys/kern/sys_procdesc.c

437

What's the purpose of this call? We just lost the last reference to the procdesc.

[kib]

In D58407#1342980, @markj wrote:

In the due course: change the locking requirements for pd_flags to proctree_lock. This does not modify actual locking regime, only the annotation.

But procdesc_fill_winfo() already assumes the new locking protocol: it does not hold the procdesc mutex while modifying pd_flags.

I mean that the code as is before the patch, is compliant.

sys/kern/kern_exit.c

1608–1609

This was removed in the D58264 review. I moved that chunk into this one.

1612–1613

Same.

sys/kern/sys_procdesc.c

437

Out of precaution I added the calls into all places where pd->pd_proc = NULL is assigned.
Removed now.

[kib]

Remove the procdesc_fill_winfo() call right before the procdesc_free().
Move two chunks there, from D58264

[markj]

Of course, some of the procdesc tests need to be updated as well.

sys/kern/kern_exit.c

1575

I wonder if we actually need the proctree lock now? At least, if the process already exited, can we avoid acquiring it, and acquire the procdesc lock instead? (Just wondering, not something to be addressed in this patch.)

1610

wait_fill_siginfo() assumes that the process has exited, no? It'll set siginfo->si_code = CLD_EXITED by default.

[kib]

In D58407#1343001, @markj wrote:

Of course, some of the procdesc tests need to be updated as well.

Yes, there will be another review after this one and D58264, and they will have to go in together.

sys/kern/kern_exit.c

1575

Perhaps not, but then setting the PDF_EXITED and PDF_EXIT_INFO need to have release store semantic, and there the load of pd_flags should become acquire. Indeed better to postpone that.

1599

Might be not, but I think it is better to leave it. It is protective programming, and if talking about moving the proctree lock acquiring down, it is useful.

1610

It is overwritten by wait6_check_alive()->report_alive_proc() in the next block as needed.