Page MenuHomeFreeBSD

bhyve vmm statistics tables need to be sized relative to VM_MAXCPU
Needs RevisionPublic

Authored by rgrimes on Jan 11 2019, 1:53 AM.

Details

Reviewers
jhb
tychon
pmooney_pfmooney.com
Group Reviewers
bhyve
Summary

You well get message(s) about the in ability to add vmm statistics if you increase VM_MAXCPU as the tables are statically sized at 64, correct that to be (48+VM_MAXCPU) so they scale as you increase this.

This is more of a marker so that the places that depend on VM_MAXCPU can be identified in the process of making it a run time per vm variable.

Test Plan

Load vmm.ko without this change and VM_MAXCPU set ot 24, notice console message, apply this fix and repeat, notice no console message.

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

rgrimes created this revision.Jan 11 2019, 1:53 AM
rgrimes updated this revision to Diff 52832.Jan 14 2019, 5:52 PM

Correct spelling of VM_MAXCPU

araujo requested changes to this revision.Jan 15 2019, 12:19 AM

It doesn't looks quite right! I need more time to articulate better arguments, but this patch as it is, must not be committed in.

This revision now requires changes to proceed.Jan 15 2019, 12:19 AM

Since the statistic tables are allocated on a per-vCPU basis already, it doesn't make sense to me why the tables themselves need to be expanded like this.

Since the statistic tables are allocated on a per-vCPU basis already, it doesn't make sense to me why the tables themselves need to be expanded like this.

Let me look at this again, but iirc the failure I saw was at vmm.ko load time when it hit an assert that says the data would not fit in the structure sized by these constants.

Since the statistic tables are allocated on a per-vCPU basis already, it doesn't make sense to me why the tables themselves need to be expanded like this.

vmm_stat.c:static struct vmm_stat_type *vsttab[MAX_VMM_STAT_ELEMS];

Actually they are allocated from a static global table that needs to be big enough to hold all of the statistics for a VM, including the per vCPU stats.

That stores the type definitions for the statistics. Storage of the actual statistic data should still be per-vcpu, hung off of struct vcpu`stats. Requiring a type definition per-vcpu doesn't make sense in that case.

That stores the type definitions for the statistics. Storage of the actual statistic data should still be per-vcpu, hung off of struct vcpu`stats. Requiring a type definition per-vcpu doesn't make sense in that case.

Per a side band communications it was found that there is
io/vlapic.c:static VMM_STAT_ARRAY(IPIS_SENT, VM_MAXCPU, "ipis sent to vcpu");
which means these tables do need to be sized based on VM_MAXCPU.

I am revising this patch to indicate this fact and marking that fact as XXX since this is going to make dyanamic sizing more work, and it may just be easier to discard these statistics as not useful.

jhb added a comment.Feb 19 2019, 5:51 PM

Full context diffs would really be useful in the future.

sys/amd64/include/vmm_dev.h
160

This can't become variable in the future as the type has to be static. I think instead we will want to keep this type alone and add new ioctls to handle larger sizes of stats. I can see two approaches, one would be to add a new 'vm_stats' like type that has a new 'base' field that sets the initial stat number (so the first call you set base to 0, the next call you set base to MAX_VM_STATS to get stats 64-127, etc. You would just keep calling it until the 'out' wasn't MAX_VM_STATS to discover what the number of stats were). The second approach would be that you would have a variant where 'statbuf' is a pointer to an array instead of an inline array. In this approach, if 'statbuf' were NULL, the ioctl wouldn't store any stats but would store the number of stats in 'num_entries' as a way to size the array. When you invoked the ioctl with a non-NULL statbuf, 'num_entries' would be an 'in / out' parameter giving the number of allocated stats on input and the number of populated stats on output.

sys/amd64/vmm/vmm_stat.h
39

Here I think the solution is to make this constant private to vmm_stat.c.

The reason for the CTASSERT in vmm_dev.c is to make vmm_stat_copy simpler to implement.

I think given that code that the first approach I described above is the better approach (vs a dynamic statbuf) and that you would instead need to modify vmm_stat_copy() to act as if it is working with the new version of the API that takes a base and an entry count. The legacy ioctl could then just do:

error = vmm_stat_copy(sv->cm, vmstats->cupid, 0, nitems(vmstats->statbuf),
    &vmstats->num_entries, vmstats->statbuf);

vmm_stat_copy() would then have to change to do bounds checking with respect to the new 'base' and 'count' arguments and only return the correct subset of stats.

The review does nothing about addressing your other issues, it simply corrects a current state of affairs that is wrong if you raise VM_MAXCPU above 24.

jhb added a comment.Feb 19 2019, 6:46 PM

Pasting a reply from Rod:

I am just trying to fix the current bug that this code does not
work correctly if you raise VM_MAXCPU, it starts to give you
problems at something like 24 due to the stats array not having
space for the ipi table that tries to get added.

This also documents the size dependancy and removes the
false /* arbitrary */ comment.

jhb added a comment.Feb 19 2019, 6:48 PM
In D18816#411818, @jhb wrote:

Pasting a reply from Rod:
I am just trying to fix the current bug that this code does not
work correctly if you raise VM_MAXCPU, it starts to give you
problems at something like 24 due to the stats array not having
space for the ipi table that tries to get added.
This also documents the size dependancy and removes the
false /* arbitrary */ comment.

The problem is you can't change it. Changing the constant in vmm_dev.h changes the ABI since the size of the structure is assumed in vmm_stat_copy()'s current API and that is also encoded in the ioctl value since the size of the structure is part of the constant. One of the approaches I described above is the only way you can do this without breaking the ABI.

In D18816#411819, @jhb wrote:
In D18816#411818, @jhb wrote:

Pasting a reply from Rod:
I am just trying to fix the current bug that this code does not
work correctly if you raise VM_MAXCPU, it starts to give you
problems at something like 24 due to the stats array not having
space for the ipi table that tries to get added.
This also documents the size dependancy and removes the
false /* arbitrary */ comment.

The problem is you can't change it. Changing the constant in vmm_dev.h changes the ABI since the size of the structure is assumed in vmm_stat_copy()'s current API and that is also encoded in the ioctl value since the size of the structure is part of the constant. One of the approaches I described above is the only way you can do this without breaking the ABI.

Then I think we should just go with pmooney's suggest of dropping the ipi's from the stats, as an API change would make this non mergable to 12, and this really does need to get back to 12.

jhb added a comment.Feb 19 2019, 10:02 PM
In D18816#411819, @jhb wrote:
In D18816#411818, @jhb wrote:

Pasting a reply from Rod:
I am just trying to fix the current bug that this code does not
work correctly if you raise VM_MAXCPU, it starts to give you
problems at something like 24 due to the stats array not having
space for the ipi table that tries to get added.
This also documents the size dependancy and removes the
false /* arbitrary */ comment.

The problem is you can't change it. Changing the constant in vmm_dev.h changes the ABI since the size of the structure is assumed in vmm_stat_copy()'s current API and that is also encoded in the ioctl value since the size of the structure is part of the constant. One of the approaches I described above is the only way you can do this without breaking the ABI.

Then I think we should just go with pmooney's suggest of dropping the ipi's from the stats, as an API change would make this non mergable to 12, and this really does need to get back to 12.

A new ioctl is MFC'able, and it's the proper way to fix this going forward to allow an arbitrary CPU count.

araujo removed a reviewer: araujo.Feb 22 2019, 1:23 AM