Skip to contentSkip to conversation

reduce KVM stack usage

RFC PATCH In review Dave Hansen 3 participants 5 messages Jul 17, 2008 – Jul 19, 2008

Subsystems (3)
x86 KVM KVM x86
lore
Share & tools
lore

Dave Hansen

author
#0 lore raw

RFC PATCH 1/1 reduce KVM stack usage

KVM uses a lot of kernel stack on x86, especially with gcc 3.x. It likes to overflow it and make my poor machine go boom. This patch takes the worst stack users and makes them use kmalloc(). It also saves ~30 bytes in kvm_arch_vm_ioctl() by using a union.

I haven't tested this at all yet. Just posting to get some comments.

Avi, you said that one of these was a hot path. Which one is that? I kinda doubt you'll be able to see any impact anyway.

before:

0x000042d3 kvm_arch_vcpu_ioctl [kvm]: 2148
0x000012e3 kvm_vcpu_ioctl [kvm]: 1620
0x00004a83 kvm_arch_vm_ioctl [kvm]: 1332
0x0000d472 kvm_pv_mmu_op [kvm]: 536
0x0000d4eb kvm_pv_mmu_op [kvm]: 536
0x00000476 __kvm_set_memory_region [kvm]: 184
0x000007e3 __kvm_set_memory_region [kvm]: 184
0x00007eb3 kvm_task_switch_32 [kvm]: 128
0x0000b553 paging64_page_fault [kvm]: 112
0x0000bfa3 paging32_page_fault [kvm]: 112
0x0000e48b x86_emulate_insn [kvm]: 112
0x0000e785 x86_emulate_insn [kvm]: 112

after:

0x00000476 __kvm_set_memory_region [kvm]: 184
0x000007e3 __kvm_set_memory_region [kvm]: 184
0x00004c83 kvm_arch_vm_ioctl [kvm]: 164
0x000012e3 kvm_vcpu_ioctl [kvm]: 152
0x00008013 kvm_task_switch_32 [kvm]: 128
0x0000b6b3 paging64_page_fault [kvm]: 112
0x0000c103 paging32_page_fault [kvm]: 112
0x0000e5fb x86_emulate_insn [kvm]: 112
0x0000e8f5 x86_emulate_insn [kvm]: 112
0x00004353 kvm_arch_vcpu_ioctl [kvm]: 104

arch/x86/kvm/mmu.c +15 -8
index 7e7c396..f4e62f2 100644
@@ -2154,18 +2154,24 @@ int kvm_pv_mmu_op(struct kvm_vcpu *vcpu, unsigned long bytes,
 		  gpa_t addr, unsigned long *ret)
 {
 	int r;
-	struct kvm_pv_mmu_op_buffer buffer;
+	struct kvm_pv_mmu_op_buffer *buffer =
+		kmalloc(GFP_KERNEL, sizeof(struct kvm_pv_mmu_op_buffer));
 
-	buffer.ptr = buffer.buf;
-	buffer.len = min_t(unsigned long, bytes, sizeof buffer.buf);
-	buffer.processed = 0;
+	if (!buffer) {
+		*ret = 0;
+		return -ENOMEM;
+	}
+
+	buffer->ptr = buffer->buf;
+	buffer->len = min_t(unsigned long, bytes, sizeof buffer->buf);
+	buffer->processed = 0;
 
-	r = kvm_read_guest(vcpu->kvm, addr, buffer.buf, buffer.len);
+	r = kvm_read_guest(vcpu->kvm, addr, buffer->buf, buffer->len);
 	if (r)
 		goto out;
 
-	while (buffer.len) {
-		r = kvm_pv_mmu_op_one(vcpu, &buffer);
+	while (buffer->len) {
+		r = kvm_pv_mmu_op_one(vcpu, buffer);
 		if (r < 0)
 			goto out;
 		if (r == 0)

@@ -2174,7 +2180,8 @@ int kvm_pv_mmu_op(struct kvm_vcpu *vcpu, unsigned long bytes,
 
 	r = 1;
 out:
-	*ret = buffer.processed;
+	*ret = buffer->processed;
+	kfree(buffer);
 	return r;
 }
arch/x86/kvm/x86.c +83 -48

205 lines not on this page · raw message

virt/kvm/kvm_main.c +32 -18

94 lines not on this page · raw message

-- Dave

#1 lore raw

arch/x86/kvm/mmu.c int kvm_pv_mmu_op(struct kvm_vcpu *vcpu, unsigned long bytes, · line 2158

Quoted 2 lines
arch/x86/kvm/mmu.c +2 -0
+	struct kvm_pv_mmu_op_buffer *buffer =
+		kmalloc(GFP_KERNEL, sizeof(struct kvm_pv_mmu_op_buffer));

Surely this produces a warning? kmalloc takes (size, flags) -- you have them reversed here.

Review

Quoted 5 lines
(quoted diff) +5 -0
+		lapic = kzalloc(GFP_KERNEL, sizeof(*lapic));
+		lapic = kmalloc(GFP_KERNEL, sizeof(*lapic));
+	struct kvm_irqchip *chip = kmalloc(GFP_KERNEL, sizeof(*chip));
+		kvm_sregs = kmalloc(GFP_KERNEL, sizeof kvm_sregs);
+		fpu = kmalloc(GFP_KERNEL, sizeof(*fpu));

same for all of these places.

virt/kvm/kvm_main.c out_free2: · line 1095

Quoted 7 lines
virt/kvm/kvm_main.c +6 -0
+	if (lapic)
+		kfree(lapic);
 
+	if (fpu)
+		kfree(fpu);
+	if (kvm_sregs)
+		kfree(kvm_sregs);

kfree(NULL) is fine, so you can remove the if()s here.

author
#2 lore raw
Quoted 5 lines · Roland Dreier

Review

Quoted 2 lines
(quoted diff) +2 -0
+	struct kvm_pv_mmu_op_buffer *buffer =
+		kmalloc(GFP_KERNEL, sizeof(struct kvm_pv_mmu_op_buffer));

Surely this produces a warning? kmalloc takes (size, flags) -- you have them reversed here.

Heh. It actually doesn't.

Quoted 11 lines

Review

Quoted 5 lines
(quoted diff) +5 -0
+		lapic = kzalloc(GFP_KERNEL, sizeof(*lapic));
+		lapic = kmalloc(GFP_KERNEL, sizeof(*lapic));
+	struct kvm_irqchip *chip = kmalloc(GFP_KERNEL, sizeof(*chip));
+		kvm_sregs = kmalloc(GFP_KERNEL, sizeof kvm_sregs);
+		fpu = kmalloc(GFP_KERNEL, sizeof(*fpu));

same for all of these places.

Quoted 2 lines
(quoted diff) +2 -0
+	if (lapic)
+		kfree(lapic);
Quoted 6 lines

Review

Quoted 4 lines
(quoted diff) +4 -0
+	if (fpu)
+		kfree(fpu);
+	if (kvm_sregs)
+		kfree(kvm_sregs);

kfree(NULL) is fine, so you can remove the if()s here.

I know it is fine, but I kinda like putting the if()s, just to let people know that we don't always *expect* something to be in there. But, it doesn't matter to me too much either way.

Here's another try. This actually lets kvm come up for me.

arch/x86/kvm/mmu.c +15 -8
index 7e7c396..fd1f303 100644
@@ -2154,18 +2154,24 @@ int kvm_pv_mmu_op(struct kvm_vcpu *vcpu, unsigned long bytes,
 		  gpa_t addr, unsigned long *ret)
 {
 	int r;
-	struct kvm_pv_mmu_op_buffer buffer;
+	struct kvm_pv_mmu_op_buffer *buffer =
+		kmalloc(sizeof(struct kvm_pv_mmu_op_buffer), GFP_KERNEL);
 
-	buffer.ptr = buffer.buf;
-	buffer.len = min_t(unsigned long, bytes, sizeof buffer.buf);
-	buffer.processed = 0;
+	if (!buffer) {
+		*ret = 0;
+		return -ENOMEM;
+	}
+
+	buffer->ptr = buffer->buf;
+	buffer->len = min_t(unsigned long, bytes, sizeof buffer->buf);
+	buffer->processed = 0;
 
-	r = kvm_read_guest(vcpu->kvm, addr, buffer.buf, buffer.len);
+	r = kvm_read_guest(vcpu->kvm, addr, buffer->buf, buffer->len);
 	if (r)
 		goto out;
 
-	while (buffer.len) {
-		r = kvm_pv_mmu_op_one(vcpu, &buffer);
+	while (buffer->len) {
+		r = kvm_pv_mmu_op_one(vcpu, buffer);
 		if (r < 0)
 			goto out;
 		if (r == 0)

@@ -2174,7 +2180,8 @@ int kvm_pv_mmu_op(struct kvm_vcpu *vcpu, unsigned long bytes,
 
 	r = 1;
 out:
-	*ret = buffer.processed;
+	*ret = buffer->processed;
+	kfree(buffer);
 	return r;
 }
arch/x86/kvm/x86.c +85 -51

212 lines not on this page · raw message

virt/kvm/kvm_main.c +30 -18

92 lines not on this page · raw message

-- Dave

#3 lore raw
Quoted 6 lines
Quoted 4 lines

Review

Quoted 1 line
(quoted diff) +1 -0
+		kmalloc(GFP_KERNEL, sizeof(struct kvm_pv_mmu_op_buffer));

Surely this produces a warning? kmalloc takes (size, flags) -- you have them reversed here.

Heh. It actually doesn't.

Yeah, I guess you need sparse to catch the gfp_t mismatch.

Quoted 5 lines
Quoted 1 line

kfree(NULL) is fine, so you can remove the if()s here.

I know it is fine, but I kinda like putting the if()s, just to let people know that we don't always *expect* something to be in there. But, it doesn't matter to me too much either way.

It's not really a big deal, but "if (x) kfree(x);" does bloat the object code with the extra test of 'x'. I guess you could put a comment like /* free any temp structures we allocated */ or something like that.

- R.

#4 lore raw
In reply to Dave Hansen
Quoted 11 lines · Dave Hansen

KVM uses a lot of kernel stack on x86, especially with gcc 3.x. It likes to overflow it and make my poor machine go boom. This patch takes the worst stack users and makes them use kmalloc(). It also saves ~30 bytes in kvm_arch_vm_ioctl() by using a union.

I haven't tested this at all yet. Just posting to get some comments.

Avi, you said that one of these was a hot path. Which one is that? I kinda doubt you'll be able to see any impact anyway.

The pv_mmu_ops thing.

Quoted 37 lines
arch/x86/kvm/mmu.c +13 -7
index 7e7c396..f4e62f2 100644
@@ -2154,18 +2154,24 @@ int kvm_pv_mmu_op(struct kvm_vcpu *vcpu, unsigned long bytes,
 		  gpa_t addr, unsigned long *ret)
 {
 	int r;
-	struct kvm_pv_mmu_op_buffer buffer;
+	struct kvm_pv_mmu_op_buffer *buffer =
+		kmalloc(GFP_KERNEL, sizeof(struct kvm_pv_mmu_op_buffer));
 
-	buffer.ptr = buffer.buf;
-	buffer.len = min_t(unsigned long, bytes, sizeof buffer.buf);
-	buffer.processed = 0;
+	if (!buffer) {
+		*ret = 0;
+		return -ENOMEM;
+	}
+
+	buffer->ptr = buffer->buf;
+	buffer->len = min_t(unsigned long, bytes, sizeof buffer->buf);
+	buffer->processed = 0;
 
-	r = kvm_read_guest(vcpu->kvm, addr, buffer.buf, buffer.len);
+	r = kvm_read_guest(vcpu->kvm, addr, buffer->buf, buffer->len);
 	if (r)
 		goto out;
 
-	while (buffer.len) {
-		r = kvm_pv_mmu_op_one(vcpu, &buffer);
+	while (buffer->len) {
+		r = kvm_pv_mmu_op_one(vcpu, buffer);
 		if (r < 0)
 			goto out;
 		if (r == 0)

arch/x86/kvm/mmu.c int kvm_pv_mmu_op(struct kvm_vcpu *vcpu, unsigned long bytes, · line 2185

Quoted 7 lines
arch/x86/kvm/mmu.c +2 -1
@@ -2174,7 +2180,8 @@ int kvm_pv_mmu_op(struct kvm_vcpu *vcpu, unsigned long bytes,
 
 	r = 1;
 out:
-	*ret = buffer.processed;
+	*ret = buffer->processed;
+	kfree(buffer);

Please change this to store the buffer in vcpu->arch.something, and free it on vcpu destruction. If this path is called at all, it is called very often, so it's fine to "waste" those 512 bytes.

Might even allocate it as a field in vcpu->arch, since it's only 512 bytes.

arch/x86/kvm/mmu.c int kvm_pv_mmu_op(struct kvm_vcpu *vcpu, unsigned long bytes,

Quoted 8 lines
arch/x86/kvm/mmu.c +6 -0
+static int kvm_arch_vm_irqchip_ioctl(struct kvm *kvm, void *argp,
+				     unsigned int ioctl)
+{
+
+int x86_kvm_vm_ioctl_set_memory_region(struct kvm *kvm, void *argp)
+{

Please keep the code inlined in kvm_arch_vcpu_ioctl(), as this is a -stable candidate and I want to keep the patch obvious. A later patch can break it up.

Please separate the vm ioctl, vcpu ioctl, and pv_mmu_op thing into separate patches, for bisect goodness.

Signature · 2 lines
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.