bhyve vmm statistics tables need to be sized relative to VM_MAXCPU
AbandonedPublic
Actions

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

Details

Reviewers

tychon
pmooney_pfmooney.com
rgrimes

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

Tests Skipped

Event Timeline

rgrimes created this revision.Jan 11 2019, 1:53 AM

Herald added a subscriber: imp. · View Herald TranscriptJan 11 2019, 1:53 AM

Correct spelling of VM_MAXCPU

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.

In D18816#404801, @pmooney_pfmooney.com wrote:

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.

In D18816#404801, @pmooney_pfmooney.com wrote:

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.

In D18816#404818, @pmooney_pfmooney.com wrote:

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.

rgrimes added a parent revision: D10070: vmm.ko: retire compile time VM_MAXCPU, replace with per VM topology maxcpus.Feb 19 2019, 5:00 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.

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.

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.

In D18816#411853, @rgrimes wrote:

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.