Page MenuHomeFreeBSD
Differential D53728

kern: Introduce RLIMIT_VMM
Needs ReviewPublic
Actions

Authored by bnovkov on Thu, Nov 13, 11:13 AM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Nov 24, 4:33 PM
Unknown Object (File)
Sat, Nov 22, 9:51 AM
Unknown Object (File)
Wed, Nov 19, 11:56 PM
Unknown Object (File)
Wed, Nov 19, 11:56 PM
Unknown Object (File)
Wed, Nov 19, 11:56 PM
Unknown Object (File)
Wed, Nov 19, 11:55 PM
Unknown Object (File)
Wed, Nov 19, 4:38 PM
Unknown Object (File)
Sun, Nov 16, 6:03 AM
View All 19 Files
Subscribers
corvink
imp
olce

Details

Reviewers
markj
olce
corvink
Group Reviewers
bhyve
Summary

This change introduces a new per-UID limit for controlling the
number of vmm instances, in anticipation of unprivileged bhyve.
This limit is designed to prevent a user from creating an unlimited
amount of vmm instances.
The initial value of the limit was chosen arbitrarily and may be
adjusted in the future.

Sponsored by: The FreeBSD Foundation
Sponsored by: Klara, Inc.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 68594
Build 65477: arc lint + arc unit

Event Timeline

bnovkov created this revision.Thu, Nov 13, 11:13 AM
Herald added a reviewer: olce. · View Herald TranscriptThu, Nov 13, 11:13 AM
Herald added subscribers: olce, imp. · View Herald Transcript
bnovkov requested review of this revision.Thu, Nov 13, 11:13 AM
Harbormaster completed remote builds in B68594: Diff 166350.Thu, Nov 13, 11:13 AM
bnovkov added a child revision: D53729: vmm: Add ability to destroy VMs on close.Thu, Nov 13, 11:13 AM
corvink added a subscriber: corvink.Fri, Nov 14, 6:54 AM
corvink added inline comments.
sys/dev/vmm/vmm_dev.c
991

Why changing this to thread?

1190

It would be great to explain in the commit message why we want to restrict the number of vmms and especially, why we're choosing this default value.

bnovkov updated this revision to Diff 166564.Mon, Nov 17, 2:45 PM
bnovkov edited the summary of this revision. (Show Details)

Address @corvink 's comments.

bnovkov added inline comments.Mon, Nov 17, 2:47 PM
sys/dev/vmm/vmm_dev.c
991

Whoops, that was a leftover from an unrelated previous attempt, thanks for catching it!

1190

I've expanded the commit message, but the limit itself is fairly arbitrary. I'm open to suggestions if you think there's a more reasonable value.

corvink accepted this revision.Tue, Nov 18, 6:53 AM
corvink added inline comments.
sys/dev/vmm/vmm_dev.c
1190

I just prefer that we're adding a short reason why choosing this default limit. If it's just a random pick, it's fine but it's worth mentioning in the commit message that it's just a random pick. So, if someone ever wonders why we've chosen this limit he can check the commit message.

This revision is now accepted and ready to land.Tue, Nov 18, 6:53 AM
markj added a comment.Tue, Nov 18, 3:16 PM

usr.bin/procstat/procstat_rlimit.c should be updated too.

sys/dev/vmm/vmm_dev.c
20

This sorts after queue.h.

1190

I agree, there should be a comment.

Thinking a bit more, having a max of 1024 is a bit silly, it's possibly too low on large count-count machines. Maybe let's just have it be 4*mp_ncpus or 8*mp_ncpus?

Also this needs to be overridable with a tunable, like hw.vmm.maxcpu above.

sys/kern/kern_resource.c
1776
bnovkov updated this revision to Diff 166883.Fri, Nov 21, 10:07 AM
bnovkov edited the summary of this revision. (Show Details)

Address @markj 's comments

This revision now requires review to proceed.Fri, Nov 21, 10:07 AM
bnovkov marked 4 inline comments as done.Fri, Nov 21, 10:09 AM
markj added inline comments.Fri, Nov 21, 3:07 PM
sys/dev/vmm/vmm_dev.c
1012

You'd need to decrement the counter again in the error cases below. Better would be to postpone it.

bnovkov updated this revision to Diff 167033.Mon, Nov 24, 4:34 PM

Defer incrementing the vmm counter.

markj added inline comments.Mon, Nov 24, 5:04 PM
sys/dev/vmm/vmm_dev.c
1041

You need to destroy the cdev here too. vmmdev_destroy() won't do it for you.

bnovkov updated this revision to Diff 167036.Mon, Nov 24, 5:18 PM
bnovkov marked an inline comment as done.
bnovkov added inline comments.
sys/dev/vmm/vmm_dev.c
1041

whoops, that was silly, fixed!

olce added a comment.Mon, Nov 24, 5:49 PM

There are already inline comments asking for a comment on the current limit. If possible, I think it would be great also to explain the point of this resource limit. In particular, which resources a VMM consumes that is not already accounted into existing resource limits, such as for memory: RLIMIT_AS, RLIMIT_RSS, RLIMIT_STACK, or CPU: RLIMIT_CPU? I suspect some kernel memory? Are there hardware limitations to the number of VMs that can be created?

sys/dev/vmm/vmm_dev.c
906–907

Small in-passing question: Can sc->ucred really be NULL here? I'm asking to be able to verify that chgvmmcnt() calls are indeed paired correctly.

Revision Contents
Changeset List

PathSize
sys/
dev/
vmm/
25 lines
kern/
13 lines
sys/
4 lines
2 lines

Diff 166350

View Options

sys/dev/vmm/vmm_dev.c

Loading...
View Options

sys/kern/kern_resource.c

Loading...
View Options

sys/sys/resource.h

Loading...
View Options

sys/sys/resourcevar.h

Loading...