vmm: Expose per-cpu guest time counters in a new sysctl
Needs ReviewPublic
Actions

Authored by crowston_protonmail.com on Jan 3 2020, 3:42 PM.

Details

Reviewers

None

Group Reviewers

Summary

Many system monitoring tools, such as htop, can express the CPU time spent by the system in the execution of virtual machine guests. To facilitate such metrics on FreeBSD, I add a new performance counter in the vmm subsystem and expose it through a new sysctl, hw.vmm.stat. Access to this sysctl is restricted by a new privilege, PRIV_VMM_HOST_STAT.

(This feature differs from the existing vmm_stats code in that the existing code expresses per-vm information on a per-virtual-cpu basis. This new patch provides a per-physical-cpu counter of the aggregate number of ticks dedicated to executing guest-mode code since the vmm.ko module was loaded.)

Test Plan

Tested over several days on my AMD64 machine. There is an accompanying patch to htop which allows these counters to be tested: https://github.com/hishamhm/htop/compare/master...RobCrowston:freebsd-vmm-counter-1

Diff Detail

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 28465
Build 26529: arc lint + arc unit

Event Timeline

crowston_protonmail.com created this revision.Jan 3 2020, 3:42 PM

Herald added a reviewer: bhyve. · View Herald TranscriptJan 3 2020, 3:42 PM

Herald added subscribers: bcran, rgrimes. · View Herald Transcript

Harbormaster completed remote builds in B28459: Diff 66302.Jan 3 2020, 3:42 PM

crowston_protonmail.com edited the summary of this revision. (Show Details)Jan 3 2020, 3:44 PM

crowston_protonmail.com edited the test plan for this revision. (Show Details)

crowston_protonmail.com edited the summary of this revision. (Show Details)Jan 3 2020, 3:52 PM

vmm: Accesses to guest_counters pointer itself need not be atomic.

This was a hold-over from an earlier design where the guest counters were lazily initialized on first use.

Harbormaster completed remote builds in B28465: Diff 66310.Jan 3 2020, 5:58 PM

val_packett.cool added a subscriber: val_packett.cool.Jan 4 2020, 10:25 PM

Generally looks ok to me, just some style nits and small suggestions.

sys/amd64/vmm/vmm.c
1699	If you do this inside of critical_exit(), you can probably avoid the need for atomics entirely as you won't be migrated to a different CPU during the increment.
sys/amd64/vmm/vmm_host_stat.c
57
73
82
109
111
114	Note that error is already 0 at the start of the for loop due to lines 107/108.
118
sys/amd64/vmm/vmm_host_stat.h
39	This can likely move into the .c file?

Updated as per comments.

Harbormaster completed remote builds in B33973: Diff 77841.Oct 4 2020, 1:56 PM

crowston_protonmail.com marked 8 inline comments as done.Oct 4 2020, 2:01 PM

crowston_protonmail.com added inline comments.

sys/amd64/vmm/vmm.c
1699	I believe, formally speaking, without the atomics, we will still have a data race between the load and store operations unless I add a fence on the load side. On x86, relaxed atomics are essentially free, so it seems like using atomics is a lower cost than extending the life of the critical section.

counter(9) should be used to implement this.

jhb added inline comments.Oct 5 2020, 5:46 PM

sys/amd64/vmm/vmm.c
1699	There's always a race on reading and it doesn't really matter if you lose (these are just stats after all). I would echo Peter's comment that if it's going to bother trying to use atomics instead of some other existing synchronization mechanism, it would be best to use counter64. I just think doing the ++ (which doesn't even have to be atomic) inside critical_enter() is even cheaper than that.
sys/amd64/vmm/vmm_host_stat.c
121	In fact, given that the race doesn't really matter, this entire loop could just be error = SYSCTL_OUT(req, guest_counters, sizeof(guest_counters[0]) * mp_ncpus); Once you've done that, you can also remove the special case for 'req->oldptr == NULL` to make this even simpler.