Page MenuHomeFreeBSD
Differential D9930

Bhyve cpu topology control
ClosedPublic
Actions

Authored by rgrimes on Mar 9 2017, 8:55 AM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Dec 9, 12:12 AM
Unknown Object (File)
Sun, Dec 8, 11:59 PM
Unknown Object (File)
Fri, Dec 6, 4:02 PM
Unknown Object (File)
Fri, Dec 6, 4:02 PM
Unknown Object (File)
Fri, Dec 6, 4:02 PM
Unknown Object (File)
Fri, Dec 6, 4:02 PM
Unknown Object (File)
Fri, Dec 6, 4:02 PM
Unknown Object (File)
Fri, Dec 6, 4:02 PM
View All Files
Subscribers
araujo
bcr
darkfiberiru_gmail.com
fabian.freyer_physik.tu-berlin.de
htwiz2002_gmail.com
imp
kmacy
View All 9 Subscribers

Details

Reviewers
grehan
anish
editor_callfortesting.org
bcr
Group Reviewers
manpages
bhyve
Commits
rS332298: Add the ability to control the CPU topology of created VMs
Summary

This adds the ability to control the CPU topology of created VMs from userland
without the need to tweak sysctls, it allows the old sysctls to continue to function.

The command line of bhyve is maintained in a backwards compatible way.
The API of libvmmapi is maintained in a backwards compatible way.
The sysctl's are maintained in a backwards compatible way.

Added command option looks like:
bhyve -c [cpus=]n[,sockets=n][,cores=n][,threads=n][,maxcpus=n]
The optional parts can be specified in any order, but only a single integer
invokes the backwards compatible parse.

bhyvectl --get-cpu-topology option added.

Diff Detail

Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
grehan added inline comments.Mar 16 2017, 4:13 AM
sys/amd64/vmm/vmm.c
469

minor - style(9) wants spaces either side of the '='.

479

As above, style(9) on the '=', but don't need the indirect reference here.

Also there should be some error checking on the params, with an error propagated back up to the ioctl caller.

sys/amd64/vmm/vmm_dev.c
646

As above, no need for pointers here.

sys/amd64/vmm/x86.c
64

I think this can be deleted. There weren't large numbers of users of this due to it being global, and the bhyve parameters are much better to use.

usr.sbin/bhyve/bhyverun.c
187

maxcpus is currently a no-op so it doesn't need to be exposed. No harm in having it in the internal api though.

828

Should check an error return here.

htwiz2002_gmail.com added a subscriber: htwiz2002_gmail.com.Mar 20 2017, 5:48 PM
rgrimes mentioned this in D10070: vmm.ko: retire compile time VM_MAXCPU, replace with per VM topology maxcpus.Apr 11 2017, 6:12 PM
fabian.freyer_physik.tu-berlin.de added a subscriber: fabian.freyer_physik.tu-berlin.de.Apr 11 2017, 6:16 PM
araujo added a subscriber: araujo.Apr 21 2017, 9:46 AM
kmacy added a subscriber: kmacy.Sep 6 2017, 11:56 PM

@rgrimes what is the state of this review?

darkfiberiru_gmail.com added a subscriber: darkfiberiru_gmail.com.Oct 4 2017, 10:58 PM
rgrimes marked an inline comment as done.Feb 15 2018, 9:25 PM

I have all the other comments corrected in my copy, seeking clarification on error check value for vm_set_topology before I correct that. Plan to delay depreciation of old sysctl method to allow simpler merge to stable/11. That is going to require a direct commit to stable/11, and a different commit to ^head

sys/amd64/vmm/vmm.c
479

From reading manual page and code we currently have a limit of 16 vCPU, so should I just simply check for sockets*cores*threads <=16, and if not through EINVAL?

sys/amd64/vmm/x86.c
64

I am leaving this here for now to simplify merging to stable/11.
I'll write another differential to mark the depreciation in stable/11 and delete from ^head.

rgrimes updated this revision to Diff 39405.Feb 17 2018, 2:37 AM
rgrimes marked an inline comment as done.

Correct issues and add man page changes

rgrimes marked 8 inline comments as done.Feb 17 2018, 2:41 AM
rgrimes added a reviewer: manpages.Feb 17 2018, 2:47 AM
grehan added a comment.Feb 17 2018, 3:40 AM

Thanks for getting back to this Rod.

On the validation issue, AMD and Intel have their own limits on how these numbers are exposed, and even that can be model-specific. While they can be verified in the kernel, the error return is somewhat limited - it may be worth doing the check in user-space where a more useful error can be returned.

rgrimes added a comment.Feb 17 2018, 10:15 AM
In D9930#301957, @grehan wrote:

Thanks for getting back to this Rod.

On the validation issue, AMD and Intel have their own limits on how these numbers are exposed, and even that can be model-specific. While they can be verified in the kernel, the error return is somewhat limited - it may be worth doing the check in user-space where a more useful error can be returned.

I basically came to that same conclusion already and if you look at usr.sbin/bhyve/bhyverun.c line 210 I validate that the vCPU count and sockets * core * threads are self consistent, elsewhere this script checks that the vCPU count is supported by the kernel.
I did add a minimal in kernel check in case someone is using something other than bhyve(8) at sys/amd64/vmm/vmm.c line 485 for sockets*cores*threads > MAX_VCPU, and for any use of the unimplemented maxcpus != 0, there is also an XXX comment here asking how do I check this against the vm's vCPU count, which I am unsure how to do, do I have to iterate over the vcpuid from 0 to MAX_VCPU to figure out how many threads are created? Is this valid at the time we set topology?

grehan added a comment.Mar 5 2018, 7:23 PM

I'd like to see some guest test results (FreeBSD, Linux, Windows) for varying numbers of these, in particular, with sockets > 1, and comparing what Qemu shows for the same configuration.

lib/libvmmapi/vmmapi.c
1425

uint64_t -> uint16_t

lib/libvmmapi/vmmapi.h
220

The type for these should be uin16_t - there is no need for it to be a 64-bit type.

sys/amd64/include/vmm.h
185

uint64_t -> uint16_t

sys/amd64/vmm/vmm.c
169

uint16_t -> uint16_t

431

u_int -> uint16_t

462

Minor style(9), space around =

475

uint64_t -> uint16_t

485

uint64_t -> uint16_t

487

minor style(9), return statement should be on newline.

sys/amd64/vmm/vmm_dev.c
651

If GET_TOPOLOGY isn't going to be implemented it should have an EINVAL put in.

Seems like it should be in bhyvectl.

sys/amd64/vmm/x86.c
94

cores/cpus/sockets/threads should be uint16_t

usr.sbin/bhyve/bhyverun.c
96

uint16_t -> uint16_t

rgrimes marked 2 inline comments as done.Mar 5 2018, 8:21 PM
In D9930#306276, @grehan wrote:

I'd like to see some guest test results (FreeBSD, Linux, Windows) for varying numbers of these, in particular, with sockets > 1, and comparing what Qemu shows for the same configuration.

This code makes no change in what a guest sees using the old sysctl or the new options, if you want to see this you could of seen this anytime over the last N years, this is a non-request to this change as far as I am concerned.

lib/libvmmapi/vmmapi.h
220

When I started on this task you told me to be QEMU compatible, so I did that, I looked at QEMU both for syntax and data types:
This is from qemu/vl.c
Static void smp_parse(QemuOpts *opts)
{

if (opts) {
    unsigned cpus    = qemu_opt_get_number(opts, "cpus", 0);
    unsigned sockets = qemu_opt_get_number(opts, "sockets", 0);
    unsigned cores   = qemu_opt_get_number(opts, "cores", 0);
    unsigned threads = qemu_opt_get_number(opts, "threads", 0);

We usually express "unsigned" as uint64_t in the kernel, are
you sure you want me to change these all to uint16_t?.

sys/amd64/vmm/vmm.c
462

Fixed in my local copy

487

Done in local copy, also next line has same problem. Fixed.

sys/amd64/vmm/vmm_dev.c
651

I am investigating this issue.

grehan added a comment.Mar 5 2018, 9:40 PM

You might have to explain further why you think you are exempt from having to demonstrate testing with this.

lib/libvmmapi/vmmapi.h
220

"unsigned" would correspond to uint32_t, but there is no point in even using something that size so I prefer uint16_t.

rgrimes marked 2 inline comments as done.Mar 5 2018, 10:00 PM
In D9930#306330, @grehan wrote:

You might have to explain further why you think you are exempt from having to demonstrate testing with this.

Let me try again. This differential in no way effects what a guest sees when the new method to specify
topology is used. Another words running guests with "sockets > 1" would yield no difference
what so ever in running it with vCPU > 1. Further more there is no change in what a guests sees
if you use hw.vmm.topology.cores_per_package or hw.vmm.topology.threads_per_core to effect
threads or cores, so I do not see why running a bunch of tests would yield.
I have run FreeBSD as a guest to make sure that the reported topology matches what was set
using the command line, and also tested that the results are correct if the boot time
sysctl tunables are used to ensure backwards compatibility.
Asking me to setup and run tests in KVM/Qemu is overkill, IMHO.
These tests could of been done using the already existing tunables any time over the
life time of the existing hw.vmm.topology.*

lib/libvmmapi/vmmapi.h
220

My mistake, I was thinking the sizeof(int) on amd64 would be 8 not 4. I'll change all the types to uint16_t.
It would of been more productive to have this request in the first review.

rgrimes updated this revision to Diff 40026.Mar 7 2018, 4:53 AM
rgrimes marked 16 inline comments as done.
rgrimes edited the summary of this revision. (Show Details)

Update per Peter Grehan

rgrimes added inline comments.Mar 7 2018, 4:56 AM
sys/amd64/vmm/vmm.c
431

This is an existing interface with an existing data type, I wish to maintain the old type for compatibility with stable/11, once this change is merged back there shall be a review for depreciating these 2 sysctl's with notification in stable/11 and removal in ^head.

sys/amd64/vmm/vmm_dev.c
651

I have implemented GET_TOPOLOGY and taught bhyvectl how to print it.

usr.sbin/bhyve/bhyve.8
36–43

Fix long line, I have tested change from Warren Block, done in my tree.

87–108

As above, fix long line. Waiting on input from Warren Block on how to fix this one, the same fix used for the other does not work here.

rgrimes marked 3 inline comments as done.Mar 7 2018, 4:57 AM
grehan accepted this revision.Mar 7 2018, 5:52 AM

so I do not see why running a bunch of tests would yield.

This change only makes sense for Windows desktop guests where sockets are artificially limited to 1 or 2. No other guest cares.

How about you at least test with a version of that and give the results ?

I can cover the rest of the testing.

This revision is now accepted and ready to land.Mar 7 2018, 5:52 AM
bcr added a subscriber: bcr.Mar 7 2018, 2:17 PM

Found a small whitespace nit in the man page. Also, you need to bump the .Dd to the date of the commit (when it is ready).

usr.sbin/bhyve/bhyve.8
106

There is a trailing whitespace here, detected by textproc/igor.

rgrimes marked an inline comment as done.Mar 7 2018, 3:17 PM
In D9930#306720, @bcr wrote:

Found a small whitespace nit in the man page. Also, you need to bump the .Dd to the date of the commit (when it is ready).

I have fixed both of those in my local copy, must of snuck in while I was playing with long line problems, as I ran igor on an earlier version.
Igor is now run as part of my build script.
Thanks for the catch.

usr.sbin/bhyvectl/bhyvectl.c
192

The coma at the end of this line should of been removed, this has been done in my local copy, about to push that.

rgrimes updated this revision to Diff 40037.Mar 7 2018, 3:23 PM

Fix white space nit in usr.sbin/bhyve/bhyve.8
Remove extra comma in usr.sbin/bhyvectl/bhyvectl.c causing build failure

This revision now requires review to proceed.Mar 7 2018, 3:23 PM
rgrimes marked 2 inline comments as done.Mar 7 2018, 3:26 PM
bcr accepted this revision.Mar 7 2018, 10:42 PM

OK from manpages.

This revision is now accepted and ready to land.Mar 7 2018, 10:42 PM
novel added a subscriber: novel.Mar 10 2018, 3:43 PM
rgrimes updated this revision to Diff 40131.Mar 10 2018, 4:39 PM

Update bhyve -h usage output to include new cpu topology options.
Pointed out by Roman Bogorodskiy (novel@).

This revision now requires review to proceed.Mar 10 2018, 4:39 PM
olevole_olevole.ru added a subscriber: olevole_olevole.ru.Mar 20 2018, 12:33 PM
rgrimes edited the summary of this revision. (Show Details)Mar 20 2018, 3:26 PM
rgrimes added a reviewer: bhyve.
rgrimes edited the summary of this revision. (Show Details)
rgrimes added a comment.Mar 29 2018, 5:06 PM

The issue causing the cpu topology to come out wrong on Intel cpus has been found, it is a misplaced call to vm_get_topology that is only executed for the cpuid 0xb, ecx=0 case, causing the ecx=1 case to use uninitialized values.

sys/amd64/vmm/x86.c
400

This call needs to be moved before the if, otherwise sockets, cores and threads are used unitialized in the other cases of *ecx.

rgrimes updated this revision to Diff 40887.Mar 29 2018, 5:08 PM
rgrimes edited the summary of this revision. (Show Details)
rgrimes marked an inline comment as done.
rgrimes updated this revision to Diff 40892.Mar 29 2018, 6:56 PM

Make cpu topology sysctl's conditional on FreeBSD_version so that this patch can be merged to stable/11 and make these sysctl obsolete in >120057

rgrimes updated this revision to Diff 41196.Apr 6 2018, 7:16 PM

Clean up the topology_parse() routing to catch more errors, and to properly match the documented syntax, update the bhyve.8 manual page to reflect the actual syntax implemented.

bcr added a comment.Apr 6 2018, 8:55 PM

I think we're still good on the man page!

rgrimes added a comment.Apr 6 2018, 9:09 PM
In D9930#315522, @bcr wrote:

I think we're still good on the man page!

Can you smash the accepted (manpages) button for me :-)

bcr accepted this revision.Apr 6 2018, 9:13 PM

*smash*

This revision is now accepted and ready to land.Apr 6 2018, 9:13 PM
Closed by commit rS332298: Add the ability to control the CPU topology of created VMs (authored by rgrimes). · Explain WhyApr 8 2018, 7:25 PM
This revision was automatically updated to reflect the committed changes.
Herald added a subscriber: imp. · View Herald TranscriptApr 8 2018, 7:25 PM

Revision Contents
Changeset List

PathSize
lib/
libvmmapi/
6 lines
34 lines
sys/
amd64/
include/
4 lines
15 lines
vmm/
42 lines
12 lines
39 lines
usr.sbin/
bhyve/
36 lines
68 lines
bhyvectl/
13 lines

Diff 40892

View Options

lib/libvmmapi/vmmapi.h

Loading...
View Options

lib/libvmmapi/vmmapi.c

Loading...
View Options

sys/amd64/include/vmm.h

Loading...
View Options

sys/amd64/include/vmm_dev.h

Loading...
View Options

sys/amd64/vmm/vmm.c

Loading...
View Options

sys/amd64/vmm/vmm_dev.c

Loading...
View Options

sys/amd64/vmm/x86.c

Loading...
View Options

usr.sbin/bhyve/bhyve.8

Loading...
View Options

usr.sbin/bhyve/bhyverun.c

Loading...
View Options

usr.sbin/bhyvectl/bhyvectl.c

Loading...