Page MenuHomeFreeBSD

Extend the VMM stats interface to support a dynamic count of statistics.
ClosedPublic

Authored by jhb on Dec 3 2020, 6:36 PM.
Tags
None
Referenced Files
Unknown Object (File)
Dec 31 2022, 10:43 PM
Unknown Object (File)
Dec 13 2022, 1:09 AM

Details

Summary
  • Add a starting index to 'struct vmstats' and change the VM_STATS ioctl to fetch the 64 stats starting at that index. A compat shim for 12 continues to fetch only the first 64 stats.
  • Extend vm_get_stats() in libvmmapi to use a loop and a static buffer which grows to hold the stats needed. Currently this function is not thread-safe, but it's only user in the tree is single-threaded. It could be made thread-safe by converting the static variables to be per-thread variables.
Test Plan
  • this has been compiled, but it has not been run-tested

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

jhb requested review of this revision.Dec 3 2020, 6:36 PM
sys/amd64/vmm/vmm_stat.c
109

Possibly the stats should only be refreshed for requests with an index of 0.

markj added inline comments.
sys/amd64/vmm/vmm_stat.c
95

Should we guard against a negative index? Or perhaps change index and count to be unsigned.

  • Rebase.
  • COMPAT_FREEBSD12 -> COMPAT_FREEBSD13.
  • Reject negative 'index' and 'count'.
jhb marked an inline comment as done.Sep 16 2021, 4:44 PM

Seems ok, this is just nitpicking.

lib/libvmmapi/vmmapi.c
1069

It seems kind of weird for a libvmmapi function to not be thread-safe. I see that this is only used by bhyvectl for now, so I guess that's ok.

1081

Perhaps check explicitly for errno == ENOENT to decide whether to return an error to the caller. Or perhaps the ioctl should just return success and num_entries == 0 if the index is equal to the number of stats.

sys/amd64/vmm/vmm_stat.c
85–86

Long line.

This revision is now accepted and ready to land.Dec 10 2021, 3:23 PM
lib/libvmmapi/vmmapi.c
1069

It indeed might be a bit weird, though it was already non-safe before. I don't know how often this will really be used anyway. The main point of this change is to make it easier to raise the maximum number of supported virtual CPUs without breaking the ABI of this ioctl.

1081

You can also get EINVAL for an invalid vcpu passed in by the caller. Hmm, but yes I guess if the total stat count % vmstats.statbuf == 0 you can get a bogus ENOENT here and the ioctl should return 0 in that case with num_entries == 0 instead.

sys/amd64/vmm/vmm_stat.c
98

So here we would just need to return (0) and set *num_stats to 0 for the 'index == vst_num_elems' case?

lib/libvmmapi/vmmapi.c
1069

Yeah, I think it's ok, I was just surprised.

sys/amd64/vmm/vmm_stat.c
98

Yeah, that was my thinking. If the index > vst_num_elems case continues to return an error, then vm_get_stats() needs to be adjusted to increment index by the number of entries returned rather than nitems(vmstats.statbuf). I'm not sure if it's worth the hassle, just a suggestion.

Return success with a count of 0 for a copy starting at the end.

This revision now requires review to proceed.Jan 20 2022, 6:34 PM
lib/libvmmapi/vmmapi.c
1090

The realloc() will free stats_buf, so I don't think this free() call is right.

lib/libvmmapi/vmmapi.c
1087

I need to move the free() up here then I think (or use reallocf()).

lib/libvmmapi/vmmapi.c
1087

stats_buf is defined as static, so it's technically ok to leave it allocated so long as it stays in harmony with stats_count.

jhb marked an inline comment as done.
  • Remove extraneous free().
  • Add _Thread_local.
markj added inline comments.
sys/amd64/vmm/vmm_stat.c
85–86

Still unwrapped.

This revision is now accepted and ready to land.Feb 7 2022, 3:30 PM
jhb marked 6 inline comments as done.Feb 7 2022, 9:52 PM
jhb added inline comments.
sys/amd64/vmm/vmm_stat.c
85–86

Oops, fixed before commit.

This revision was automatically updated to reflect the committed changes.
jhb marked an inline comment as done.