Page MenuHomeFreeBSD

Add accessor for vm->maxcpus in preperation for run time maxcpu setting
ClosedPublic

Authored by rgrimes on Jan 5 2019, 5:18 PM.

Details

Summary

In working on the dynamic VM_MAXCPU code it has become necessary to access information inside struct vm; from several source files. The original idea of exposing the struct outside vmm.c has been changed to add an accessor function vm_get_maxcpus()

Test Plan

Compile vmm.ko, bhyveload, and bhyve. Load a 16 vCPU guest FreeBSD 12.0-RELEASE test vm on both Intel and Amd systems.

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

rgrimes created this revision.Jan 5 2019, 5:18 PM

Up until now, those structs have been kept opaque to the rest of the system, with accessor functions to perform necessary tasks/queries against them. Could you go into more detail about how your plans for dynamic VM_MAXCPU are impeded by similar constraints?

Up until now, those structs have been kept opaque to the rest of the system, with accessor functions to perform necessary tasks/queries against them. Could you go into more detail about how your plans for dynamic VM_MAXCPU are impeded by similar constraints?

I thought about that, I could use the topology accessors to rewrite all the

if (vcpuid < 0 ||vcpuid >VM_MAXCPU)

or I could add a vmm_topology_rangecheck(vcpuid) function either of these add a function call. If we are really hung up on the idea of accessors and such I can do that, but 90% of these accesses to this value are in the vmm.c file, with 4 other filex wanting access to the values.

Add bhyve group, I thought I had that in the list when I created the review. I am soliciting feedback on if I should abandon this diff and re-write the code using an accessor function (called vmm_vm_maxcpus()) to vmm.c that does the vmx->vm->maxcpus dereference and call that. The most common use of this is in vmm.c itself, and would probably end up inlined by the compiler anyway, though there are at least 4 other files that have the need for this, and iirc there is userland code that uses the VM_MAXCPU constant that needs to NOT do that!

araujo added a subscriber: araujo.Jan 8 2019, 4:46 PM

Add bhyve group, I thought I had that in the list when I created the review. I am soliciting feedback on if I should abandon this diff and re-write the code using an accessor function (called vmm_vm_maxcpus()) to vmm.c that does the vmx->vm->maxcpus dereference and call that. The most common use of this is in vmm.c itself, and would probably end up inlined by the compiler anyway, though there are at least 4 other files that have the need for this, and iirc there is userland code that uses the VM_MAXCPU constant that needs to NOT do that!

Hi @rgrimes,

I'm not sure the impact of the changes you are intend to do related with the ACPI MADT table, I have this ticket under my hands and as you are working on these changes now, maybe you should take a double look on this ticket. This ticket also has couple links to some discussions regarding the bump of VM_MAXCPU.

Ticket: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=212782

...

Hi @rgrimes,
I'm not sure the impact of the changes you are intend to do related with the ACPI MADT table, I have this ticket under my hands and as you are working on these changes now, maybe you should take a double look on this ticket. This ticket also has couple links to some discussions regarding the bump of VM_MAXCPU.
Ticket: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=212782

I have taken that bug, I was aware of those issues and plan a seperate review for fixing the MADT table issue, which does not even reference VM_MAXCPU when it should. I may quickly add a CTASSERT(VM_MAXCPU>20) which iirc is where that table starts to run into issues, though that would stop pmooney who is using efi only and hence does not have the issue this table creates. Fixing the MADT is a rather easy thing once all the other bits are in place.

re-write the code using an accessor function (called vmm_vm_maxcpus()) to vmm.c that does the vmx->vm->maxcpus dereference and call that. The most common use of this is in vmm.c itself, and would probably end up inlined by the compiler anyway, though there are at least 4 other files that have the need for this, and iirc there is userland code that uses the VM_MAXCPU constant that needs to NOT do that!

I think there's precedent for consumers inside vmm.c to simply access the struct member, rather than needing to pass through the accessor. Adding the accessor would obviate the need to make the struct vm/struct vcpu definitions visible to the rest of the vmm module.

rgrimes retitled this revision from Relocate struct vm; and associated sub structures to a new header file to Add accessor for vm->maxcpus in preperation for run time maxcpu setting.Jan 10 2019, 4:46 PM
rgrimes edited the summary of this revision. (Show Details)
rgrimes updated this revision to Diff 52745.

Change from exposing struct vm to using an accessor function, this just adds the accessor.

rgrimes updated this revision to Diff 52899.Jan 16 2019, 9:39 PM

Add convertion of most VM_MAXCPU references to either vm->maxcpus (vmm.c) or to use accessor function vm_get_maxcpus().

re-write the code using an accessor function (called vmm_vm_maxcpus()) to vmm.c that does the vmx->vm->maxcpus dereference and call that. The most common use of this is in vmm.c itself, and would probably end up inlined by the compiler anyway, though there are at least 4 other files that have the need for this, and iirc there is userland code that uses the VM_MAXCPU constant that needs to NOT do that!

I think there's precedent for consumers inside vmm.c to simply access the struct member, rather than needing to pass through the accessor. Adding the accessor would obviate the need to make the struct vm/struct vcpu definitions visible to the rest of the vmm module.

I have rewritten my changes to use either the accessor if from external to vmm.c or using direct struct member reference if inside vmm.c. There are still a few left to fix that did not lend themselves to direct mechanical change that I am working on.

rgrimes edited the test plan for this revision. (Show Details)Jan 16 2019, 9:59 PM
jhb accepted this revision.Feb 19 2019, 5:32 PM

I think using an accessor is a good path forward.

sys/amd64/vmm/vmm.c
469 ↗(On Diff #52899)

I think you can drop this line.

506 ↗(On Diff #52899)

I think you can drop this line, especially given the XXX on line 499 after the maxcpus != 0 check.

This revision is now accepted and ready to land.Feb 19 2019, 5:32 PM
rgrimes marked 2 inline comments as done.Feb 19 2019, 5:45 PM
rgrimes added inline comments.
sys/amd64/vmm/vmm.c
469 ↗(On Diff #52899)

This line is needed for other work that checks this 0 value to mean "we have not allocated" data structures. Also I didnt touch this in this review, so that would be a change beyond the purpose of this code review.

506 ↗(On Diff #52899)

This and line 499 are handled in other code work, for now there is no need to change this here to make this patch do what it needs to do.

jhb added inline comments.Feb 19 2019, 7:00 PM
sys/amd64/vmm/vmm.c
469 ↗(On Diff #52899)

Usually I would wait to include that until the change that actually uses it. As it is, it looks odd in this change.

rgrimes marked 3 inline comments as done.Feb 19 2019, 7:04 PM
rgrimes added inline comments.
sys/amd64/vmm/vmm.c
469 ↗(On Diff #52899)

John, if you do NOT set this value here all the accessors used to replace VM_MAXCPU by the vm->maxcpus would get a value of 0 seriously breaking things.

jhb added inline comments.Feb 19 2019, 10:00 PM
sys/amd64/vmm/vmm.c
469 ↗(On Diff #52899)

Huh? The comment is about the 'maxcpus = 0' line that is pointless since the next line sets it to VM_MAXCPU.

rgrimes marked 2 inline comments as done.Feb 21 2019, 8:15 AM
rgrimes updated this revision to Diff 54175.

Update to incorporate feedback from bde@ (mentor), wrap long lines, sort prototypes within group vm_get* in sys/amd64/include/vmm.h.

This revision now requires review to proceed.Feb 21 2019, 8:15 AM
araujo removed a subscriber: araujo.Feb 22 2019, 1:22 AM
sys/amd64/vmm/vmm.c
469 ↗(On Diff #54175)

Nuke the 0 assignment so that maxcpus is never in what would now be an illegal state?

506 ↗(On Diff #54175)

Nuke the = maxcpus assignment so that a smaller provided value won't temporarily put the vm in an illegal state?

sys/amd64/vmm/vmm_dev.c
140 ↗(On Diff #54175)

Cache the max here?

159 ↗(On Diff #54175)

Cache the max?

206 ↗(On Diff #54175)

Cache the max - 1 once, instead of recalculating it all over the function?

rgrimes added inline comments.Feb 22 2019, 6:23 PM
sys/amd64/vmm/vmm.c
469 ↗(On Diff #54175)

Agreed, I need to update the diff and upload again.

506 ↗(On Diff #54175)

Agreed, I should of done this yesterday right after the call when it was fresh in my mind, I had meant to nuke this as per the call discussion.

469 ↗(On Diff #52899)

Let me try again. The original line of code that sets this to 0 and has the comment not implemented is still correct, in the since I have no reason to change it at this time. BUT if I do not change the value stored in this (the following line) the code would break bhyve in a very bad way, so I choose to leave the line that shall become valid later, and have its comment removed when it "is implemented", ie initing this to 0 is the correct long term value, and added a temporary line that shall be later deleted when the code can handle the fact that vm->maxcpus might be 0 and some mallocs need to be done. Since you have accepted the code anyway we should probably just drop this issue and move forward as this all cleans up when the final version is committed.

sys/amd64/vmm/vmm_dev.c
159 ↗(On Diff #54175)

I would hope the compiler would do that already without me having to explicitly code code this as
foo = vm_get_max()
for (; vcpu < foo); vcpu++)
Ah, bleh, it can not do that, these are not inlined so it does not know that there are no side effects from that function call. Ok, jhb@ do you agree I should probably cache these values locally?

Also doesn't this go away when we upstream the better locking implementation you have already done?

206 ↗(On Diff #54175)

Will do

sys/amd64/vmm/vmm_dev.c
159 ↗(On Diff #54175)

There would be some caching needed in vcpu_lock_all(), which would become part of the "write lock acquisition" path. It would be safe for the unlock path, since maxcpu would be effectively fixed while you held the write lock.

rgrimes updated this revision to Diff 54265.Feb 23 2019, 7:06 PM

Address review comments from Patrick Mooney. Found some additional places that caching would be helpful so update them too.

rgrimes marked 5 inline comments as done.Feb 23 2019, 7:09 PM
This revision is now accepted and ready to land.Feb 27 2019, 10:33 PM
jhb accepted this revision.Feb 27 2019, 11:09 PM
jhb added inline comments.
sys/amd64/vmm/vmm_dev.c
159 ↗(On Diff #54175)

You can tell the compiler that the return value is always the same for a given input by marking the function prototype with '__pure2' or some such in which case the compiler would cache the return value and only call it once. What you have is fine though.

This revision was automatically updated to reflect the committed changes.