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.
Tags
None
Referenced Files
Unknown Object (File)
Feb 12 2024, 1:31 PM
Unknown Object (File)
Dec 24 2023, 10:02 PM
Unknown Object (File)
Dec 20 2023, 2:27 AM
Unknown Object (File)
Dec 11 2023, 12:58 AM
Unknown Object (File)
Dec 10 2023, 9:56 PM
Unknown Object (File)
Dec 9 2023, 10:25 PM
Unknown Object (File)
Dec 4 2023, 10:45 PM
Unknown Object (File)
Dec 4 2023, 8:17 AM

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

Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

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!

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.
rgrimes edited the summary of this revision. (Show Details)

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

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.

I think using an accessor is a good path forward.

sys/amd64/vmm/vmm.c
469

I think you can drop this line.

506

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 added inline comments.
sys/amd64/vmm/vmm.c
469

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

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.

sys/amd64/vmm/vmm.c
469

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

rgrimes added inline comments.
sys/amd64/vmm/vmm.c
469

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.

sys/amd64/vmm/vmm.c
469

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.

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
sys/amd64/vmm/vmm.c
469

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

506

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?

sys/amd64/vmm/vmm.c
469

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.

469

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

506

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.

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.

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

This revision is now accepted and ready to land.Feb 27 2019, 10:33 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.