Page MenuHomeFreeBSD
Differential D27463

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

Authored by jhb on Dec 3 2020, 6:36 PM.
Tags
None
Referenced Files
F105957603: D27463.id99769.diff
Mon, Dec 23, 2:50 AM
Unknown Object (File)
Fri, Dec 20, 8:52 PM
Unknown Object (File)
Mon, Dec 16, 3:05 PM
Unknown Object (File)
Fri, Dec 13, 1:52 AM
Unknown Object (File)
Mon, Dec 9, 4:36 AM
Unknown Object (File)
Mon, Dec 2, 11:37 PM
Unknown Object (File)
Thu, Nov 28, 12:51 AM
Unknown Object (File)
Mon, Nov 25, 4:22 AM
View All Files
Subscribers
bcran
evgueni.gavrilov_itglobal.com
gusev.vitaliy_gmail.com
imp
markj

Details

Reviewers
rgrimes
pmooney_pfmooney.com
markj
Group Reviewers
bhyve
Commits
rGffd150099ca0: Extend the VMM stats interface to support a dynamic count of statistics.
rG64269786170f: Extend the VMM stats interface to support a dynamic count of statistics.
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
rS FreeBSD src repository - subversion
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 35172
Build 32122: arc lint + arc unit

Event Timeline

jhb created this revision.Dec 3 2020, 6:36 PM
Herald added a reviewer: bhyve. · View Herald TranscriptDec 3 2020, 6:36 PM
Herald added subscribers: bcran, imp. · View Herald Transcript
jhb requested review of this revision.Dec 3 2020, 6:36 PM
Harbormaster completed remote builds in B35172: Diff 80274.Dec 3 2020, 6:36 PM
jhb added inline comments.Dec 3 2020, 6:39 PM
sys/amd64/vmm/vmm_stat.c
99

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

rgrimes added a reviewer: pmooney_pfmooney.com.Dec 4 2020, 3:43 PM
markj added a subscriber: markj.Dec 7 2020, 11:22 PM
markj added inline comments.
sys/amd64/vmm/vmm_stat.c
94

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

evgueni.gavrilov_itglobal.com added a subscriber: evgueni.gavrilov_itglobal.com.Dec 17 2020, 6:34 AM
jhb updated this revision to Diff 95251.Sep 16 2021, 4:43 PM
  • Rebase.
  • COMPAT_FREEBSD12 -> COMPAT_FREEBSD13.
  • Reject negative 'index' and 'count'.
Harbormaster completed remote builds in B41542: Diff 95251.Sep 16 2021, 4:43 PM
jhb marked an inline comment as done.Sep 16 2021, 4:44 PM
jhb updated this revision to Diff 99769.Dec 9 2021, 5:56 PM

Rebased and tested.

Harbormaster completed remote builds in B43261: Diff 99769.Dec 9 2021, 5:56 PM
markj accepted this revision as: markj.Dec 10 2021, 3:23 PM

Seems ok, this is just nitpicking.

lib/libvmmapi/vmmapi.c
1038

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.

1049

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

Long line.

This revision is now accepted and ready to land.Dec 10 2021, 3:23 PM
jhb added inline comments.Dec 10 2021, 7:00 PM
lib/libvmmapi/vmmapi.c
1038

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.

1049

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
97

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

markj added inline comments.Dec 10 2021, 7:08 PM
lib/libvmmapi/vmmapi.c
1038

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

sys/amd64/vmm/vmm_stat.c
97

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.

gusev.vitaliy_gmail.com added a subscriber: gusev.vitaliy_gmail.com.Dec 13 2021, 1:14 PM
jhb updated this revision to Diff 101694.Jan 20 2022, 6:34 PM

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
Harbormaster completed remote builds in B44014: Diff 101694.Jan 20 2022, 6:34 PM
markj added inline comments.Jan 20 2022, 7:49 PM
lib/libvmmapi/vmmapi.c
1058

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

jhb added inline comments.Jan 20 2022, 8:38 PM
lib/libvmmapi/vmmapi.c
1055

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

markj added inline comments.Jan 20 2022, 8:40 PM
lib/libvmmapi/vmmapi.c
1055

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 updated this revision to Diff 102344.Feb 3 2022, 8:08 PM
jhb marked an inline comment as done.
  • Remove extraneous free().
  • Add _Thread_local.
Harbormaster completed remote builds in B44281: Diff 102344.Feb 3 2022, 8:08 PM
markj accepted this revision as: markj.Feb 7 2022, 3:30 PM
markj added inline comments.
sys/amd64/vmm/vmm_stat.c
85

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

Oops, fixed before commit.

Closed by commit rG64269786170f: Extend the VMM stats interface to support a dynamic count of statistics. (authored by jhb). · Explain WhyFeb 7 2022, 10:14 PM
This revision was automatically updated to reflect the committed changes.
jhb marked an inline comment as done.
jhb added a commit: rG64269786170f: Extend the VMM stats interface to support a dynamic count of statistics..
jhb added a commit: rGffd150099ca0: Extend the VMM stats interface to support a dynamic count of statistics..Apr 29 2022, 11:14 PM

Revision Contents
Changeset List

PathSize
lib/
libvmmapi/
40 lines
sys/
amd64/
include/
1 line
vmm/
29 lines
6 lines
13 lines

Diff 80274

View Options

lib/libvmmapi/vmmapi.c

Loading...
View Options

sys/amd64/include/vmm_dev.h

Loading...
View Options

sys/amd64/vmm/vmm_dev.c

Loading...
View Options

sys/amd64/vmm/vmm_stat.h

Loading...
View Options

sys/amd64/vmm/vmm_stat.c

Loading...