ddb ps: Use the pidhash to enumerate processes not in allproc.
ClosedPublic
Actions

Authored by jhb on Dec 29 2020, 11:47 PM.

Details

Reviewers

kib
mjg

Commits

rG47877889f2b8: ddb ps: Use the pidhash to enumerate processes not in allproc.

Summary

Exiting processes that have been removed from allproc but are still
executing are not yet marked PRS_ZOMBIE, so they were not listed (for
example, if a thread panics during exit1()). To detect these
processes, clear p_list.le_prev to NULL explicitly after removing a
process from the allproc list and check for this sentinel rather than
PRS_ZOMBIE when walking the pidhash.

While here, simplify the pidhash walk to use a single outer loop.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 35828
Build 32717: arc lint + arc unit

Event Timeline

jhb created this revision.Dec 29 2020, 11:47 PM

Herald added a subscriber: imp. · View Herald TranscriptDec 29 2020, 11:47 PM

jhb requested review of this revision.Dec 29 2020, 11:47 PM

Harbormaster completed remote builds in B35766: Diff 81340.Dec 29 2020, 11:47 PM

jhb added a child revision: D27825: Add a proc_off_p_hash helper variable..Dec 29 2020, 11:51 PM

Can we leave the allproc list iteration as is, please ? My motivation is that having the list ordered by recent time of creation is actually useful behavior I accustomed to, it is useful when debugging very large dumps.

We can have some flag like P2_REMOVED to indicate that process was removed from the allproc list, and 'do zombies' loop of loops would check the flag instead of PRS_ZOMBIE.

Hmmm, I could do that. Note that for kgdb I've just given up on allproc altogether (including in my kgdb scripts which have their own ps).

I tried using a P2_ALLPROC flag instead, but that was a bit more awkward. With P2_REMOVED_ALLPROC there's still a small race between removing from the allproc list and setting the flag, partly because I didn't want to add a new PROC_LOCK/UNLOCK pair just for this case that only helps ddb's ps. The other option would be change the last loop here to just check for the process in allproc again so the last loop ends up O(n^3), but it's also for a rare case ('ps') in ddb, so maybe it's worth making that slow/more complicated in exchange for keeping the hot path simple.

I think it would be fine to report some process twice in ddb. After all we are interrogating inconsistent structure.

Add P2_ALLPROC_REMOVED and use that instead of PRS_ZOMBIE.

Harbormaster completed remote builds in B35828: Diff 81439.Dec 31 2020, 7:59 PM

jhb added inline comments.Dec 31 2020, 8:01 PM

sys/kern/kern_exit.c
443	So to report twice, I'd have to move this earlier before the LIST_REMOVE, and then I'd have to add a separate PROC_LOCK/PROC_UNLOCK. The extra locking in the hot path solely for ddb 'ps' is a tradeoff I didn't think was probably worth it.

kib added inline comments.Dec 31 2020, 8:34 PM

sys/kern/kern_exit.c
443	What about storing NULL into p->p_list.lh_first right after LIST_REMOVE, and checking it in pidhash iterations ?

jhb added inline comments.Dec 31 2020, 8:49 PM

sys/kern/kern_exit.c
443	What about storing NULL into p->p_list.lh_first right after LIST_REMOVE, and checking it in pidhash iterations ? Hmm, that kind of conflicts with the TRASHIT stuff in the queue macros, but that's probably ok so long as we document the weirdness. I could even stick it under #ifdef DDB.

kib added inline comments.Dec 31 2020, 10:02 PM

sys/kern/kern_exit.c
443	Then you can store not NULL, but the TRASHIT pattern, and compare with it.

Use p_list.le_prev == NULL as the sentinel instead.

Harbormaster completed remote builds in B35831: Diff 81447.Dec 31 2020, 11:24 PM

jhb marked an inline comment as done.Dec 31 2020, 11:26 PM

jhb added inline comments.

sys/kern/kern_exit.c
443	Then you can store not NULL, but the TRASHIT pattern, and compare with it. Oh, I went with NULL. I think we want to be explicit (and not depend on queue macros, etc.), and a kernel will panic for NULL just as it will panic for the TRASHIT pattern.

kib accepted this revision.Dec 31 2020, 11:40 PM

This revision is now accepted and ready to land.Dec 31 2020, 11:40 PM

jhb retitled this revision from Use the pidhash to enumerate all processes, not just zombies. to ddb ps: Use the pidhash to enumerate processes not in allproc..Dec 31 2020, 11:57 PM

jhb edited the summary of this revision. (Show Details)

Closed by commit rG47877889f2b8: ddb ps: Use the pidhash to enumerate processes not in allproc. (authored by jhb). · Explain WhyJan 1 2021, 12:04 AM

This revision was automatically updated to reflect the committed changes.

jhb added a commit: rG47877889f2b8: ddb ps: Use the pidhash to enumerate processes not in allproc..