Page MenuHomeFreeBSD

Add ability to fetch kernel threads from cores in kvm_getprocs in libkvm
ClosedPublic

Authored by karels on Aug 29 2019, 3:09 AM.

Details

Summary

Add support for kernel threads in kvm_getprocs() and the underlying
kvm_proclist() in libkvm when fetching from a kernel core file. This
has been missing/needed for several releases, when kernel threads became
normal threads. Iterating to the next thread or proc uses a goto; I
tried to avoid that, but the result was a larger modification that seemed
more fragile than this version.

Test Plan

Tested on amd64 and i386. I couldn't figure out how to generate a
core file on armv6 or aarch64.

Sponsored by: Forcepoint LLC

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

karels created this revision.Aug 29 2019, 3:09 AM
jhb added a comment.Aug 29 2019, 8:55 PM

Does this fix anything in the base system? I thought 'ps H' worked on vmcores already? Hmm, just tried it on one and indeed it does not.

lib/libkvm/kvm_proc.c
142 ↗(On Diff #61441)

Maybe make this a while() since it has no pre and post ops?

144 ↗(On Diff #61441)

So we re-read the struct proc for each thread. Even though it would mean a lot of indenting, I do wonder if it wouldn't be overall cleaner to structure this as nested loops instead walking over processes and then threads inside the process?

You could do the process-common things like p_ucred before the thread loop so that you avoid re-reading them for each thread as well in that case.

587 ↗(On Diff #61441)

zombproc was just removed from head, so this might have to get more complicated to deal with that, but that could be a separate change.

611 ↗(On Diff #61441)

Maybe at least a comment. We could also have a way to see if kvm_deadprocs ran out of room and if so grow it and try again.

Thanks, John.

lib/libkvm/kvm_proc.c
142 ↗(On Diff #61441)

Sure.

144 ↗(On Diff #61441)

Yes, I didn't worry about the performance of ps on a core dump. I did this some time ago (with FreeBSD 8), and was trying to minimize code changes. It's a little odd, as it usually loops over processes but sometimes over both, and both share some termination conditions (buffer full). I'll take another look at that.

587 ↗(On Diff #61441)

That certainly makes it easier to MFC. I'll have to look at the zombproc change.

karels added inline comments.Aug 30 2019, 2:14 AM
lib/libkvm/kvm_proc.c
144 ↗(On Diff #61441)

I started refactoring along these lines. It is very messy; the code to read the substructures and to copy out the bits are interwoven, and share the logic to decide which is valid. I moved the read of the creds, then noticed that it was copying things into the kinfo_proc as it went. Also, it zeros the kinfo_proc at the top of the current loop. If it decides not to include the current entry, it just continues and re-zeros. That makes it harder to preserve the proc and whatever might be common. I'll look a little more, but I'm not convinced this will be a win.

jhb added inline comments.Aug 30 2019, 9:39 PM
lib/libkvm/kvm_proc.c
144 ↗(On Diff #61441)

Ok, if it's a disaster then don't worry about it. The in-kernel code does do it in passes like this (there's a fill_kinfo_proc_only() we call once and then a fill_kinfo_thread()), though there's also some special magic to deal with the INC_THREADS case vs !INC_THREADS (I think the various fill_kinfo_* functions are to deal with that without duplicating lots of code). But libkvm may not warrant that level of complexity. As you note, the perf doesn't matter.

I took yet another tack last night, and it is better than previous attempts. It seems to be working correctly, but needs more testing. The changes are much bigger, mostly due to code motions. (svn diff is now 286 lines, about twice as big.) I'll test more, then decide whether to proceed with it.

karels updated this revision to Diff 61729.Sep 6 2019, 1:23 PM

Change to use nested loops for procs and threads; deal with zombproc

Rework to use two nested loops; add changes to work with and without
zombproc in the kernel.

karels updated this revision to Diff 61730.Sep 6 2019, 1:38 PM

Upload correct version with nested loops.

The previous version was wrong.

karels added inline comments.Sep 6 2019, 11:38 PM
lib/libkvm/kvm_proc.c
83 ↗(On Diff #61730)

I meant to remove this and the ifdef; it didn't work out cleanly.

514 ↗(On Diff #61730)

Will be removed.

karels updated this revision to Diff 61960.Sep 12 2019, 1:56 AM

Remove incomplete ifdefs.

vangyzen accepted this revision.Sep 12 2019, 8:53 PM
This revision is now accepted and ready to land.Sep 12 2019, 8:53 PM
This revision was automatically updated to reflect the committed changes.