kvm: use modern cpumask primitives, no cpumask_t on stack

PATCH In review Rusty Russell 3 participants 11 messages Dec 7, 2008 – Dec 8, 2008 KVM

lore

Share & tools

lore

Rusty Russell

author

2008-12-07 10:55 UTC #0 lore raw

PATCH 1/2 kvm: use modern cpumask primitives, no cpumask_t on stack

We're getting rid on on-stack cpumasks for large NR_CPUS.

1) Use cpumask_var_t and alloc_cpumask_var (a noop normally). Fallback code is inefficient but never happens in practice.
2) smp_call_function_mask -> smp_call_function_many
3) cpus_clear, cpus_empty, cpu_set -> cpumask_clear, cpumask_empty, cpumask_set_cpu.

Signed-off-by Rusty Russell <[email protected]>

Signed-off-by Mike Travis <[email protected]>

---
 virt/kvm/kvm_main.c |   45 +++++++++++++++++++++++++++++++++++----------
 1 file changed, 35 insertions(+), 10 deletions(-)

virt/kvm/kvm_main.c +35 ~~-10~~

@@ -358,11 +358,23 @@ static void ack_flush(void *_completed)
 void kvm_flush_remote_tlbs(struct kvm *kvm)
 {
 	int i, cpu, me;
-	cpumask_t cpus;
+	cpumask_var_t cpus;
 	struct kvm_vcpu *vcpu;
 
 	me = get_cpu();
-	cpus_clear(cpus);
+	if (!alloc_cpumask_var(&cpus, GFP_ATOMIC)) {
+		/* Slow path on failure.  Call everyone. */
+		for (i = 0; i < KVM_MAX_VCPUS; ++i) {
+			vcpu = kvm->vcpus[i];
+			if (vcpu)
+				set_bit(KVM_REQ_TLB_FLUSH, &vcpu->requests);
+		}
+		++kvm->stat.remote_tlb_flush;
+		smp_call_function_many(cpu_online_mask, ack_flush, NULL, 1);
+		put_cpu();
+		return;
+	}
+
 	for (i = 0; i < KVM_MAX_VCPUS; ++i) {
 		vcpu = kvm->vcpus[i];
 		if (!vcpu)

@@ -371,24 +383,36 @@ void kvm_flush_remote_tlbs(struct kvm *k
 			continue;
 		cpu = vcpu->cpu;
 		if (cpu != -1 && cpu != me)
-			cpu_set(cpu, cpus);
+			cpumask_set_cpu(cpu, cpus);
 	}
-	if (cpus_empty(cpus))
+	if (cpumask_empty(cpus))
 		goto out;
 	++kvm->stat.remote_tlb_flush;
-	smp_call_function_mask(cpus, ack_flush, NULL, 1);
+	smp_call_function_many(cpus, ack_flush, NULL, 1);
 out:
 	put_cpu();
+	free_cpumask_var(cpus);
 }
 
 void kvm_reload_remote_mmus(struct kvm *kvm)
 {
 	int i, cpu, me;
-	cpumask_t cpus;
+	cpumask_var_t cpus;
 	struct kvm_vcpu *vcpu;
 
 	me = get_cpu();
-	cpus_clear(cpus);
+	if (!alloc_cpumask_var(&cpus, GFP_ATOMIC)) {
+		/* Slow path on failure.  Call everyone. */
+		for (i = 0; i < KVM_MAX_VCPUS; ++i) {
+			vcpu = kvm->vcpus[i];
+			if (vcpu)
+				set_bit(KVM_REQ_MMU_RELOAD, &vcpu->requests);
+		}
+		smp_call_function_many(cpu_online_mask, ack_flush, NULL, 1);
+		put_cpu();
+		return;
+	}
+
 	for (i = 0; i < KVM_MAX_VCPUS; ++i) {
 		vcpu = kvm->vcpus[i];
 		if (!vcpu)

@@ -397,13 +421,14 @@ void kvm_reload_remote_mmus(struct kvm *
 			continue;
 		cpu = vcpu->cpu;
 		if (cpu != -1 && cpu != me)
-			cpu_set(cpu, cpus);
+			cpumask_set_cpu(cpu, cpus);
 	}
-	if (cpus_empty(cpus))
+	if (cpumask_empty(cpus))
 		goto out;
-	smp_call_function_mask(cpus, ack_flush, NULL, 1);
+	smp_call_function_many(cpus, ack_flush, NULL, 1);
 out:
 	put_cpu();
+	free_cpumask_var(cpus);
 }

Avi Kivity

2008-12-07 15:52 UTC #1 lore raw

Quoted 24 lines · Rusty Russell

We're getting rid on on-stack cpumasks for large NR_CPUS.
1) Use cpumask_var_t and alloc_cpumask_var (a noop normally). Fallback code is inefficient but never happens in practice.
2) smp_call_function_mask -> smp_call_function_many
3) cpus_clear, cpus_empty, cpu_set -> cpumask_clear, cpumask_empty, cpumask_set_cpu.
virt/kvm/kvm_main.c +5 -2
@@ -358,11 +358,23 @@ static void ack_flush(void *_completed)
 void kvm_flush_remote_tlbs(struct kvm *kvm)
 {
 	int i, cpu, me;
-	cpumask_t cpus;
+	cpumask_var_t cpus;
 	struct kvm_vcpu *vcpu;
 
 	me = get_cpu();
-	cpus_clear(cpus);
+	if (!alloc_cpumask_var(&cpus, GFP_ATOMIC)) {
+		/* Slow path on failure.  Call everyone. */
+		for (i = 0; i < KVM_MAX_VCPUS; ++i) {
+			vcpu = kvm->vcpus[i];

virt/kvm/kvm_main.c static void ack_flush(void *_completed) · line 378

Quoted 10 lines

virt/kvm/kvm_main.c +9 -0

+			if (vcpu)
+				set_bit(KVM_REQ_TLB_FLUSH, &vcpu->requests);
+		}
+		++kvm->stat.remote_tlb_flush;
+		smp_call_function_many(cpu_online_mask, ack_flush, NULL, 1);
+		put_cpu();
+		return;
+	}
+

Wow, code duplication from Rusty. Things must be bad.

Since we're in a get_cpu() here, how about a per_cpu static cpumask instead? I don't mind the inefficient fallback, just the duplication.

Signature · 1 line

error compiling committee.c: too many arguments to function

Avi Kivity

2008-12-07 16:14 UTC #2 lore raw

Quoted 10 lines · Avi Kivity

Quoted 4 lines · Rusty Russell
We're getting rid on on-stack cpumasks for large NR_CPUS.
1) Use cpumask_var_t and alloc_cpumask_var (a noop normally). Fallback code is inefficient but never happens in practice.
Wow, code duplication from Rusty. Things must be bad.
Since we're in a get_cpu() here, how about a per_cpu static cpumask instead? I don't mind the inefficient fallback, just the duplication.

Btw, for the general case, instead of forcing everyone to duplicate, how about:

cpumask_var_t cpus;

with_cpumask(cpus) {
... code to populate cpus
smp_call_function_some(...);
} end_with_cpumask(cpus);

Where with_cpumask() allocates cpus, and uses a mutex + static fallback on failure.

May need a couple of variants (spinlock + GFP_NOWAIT, mutex with sleeping allocation).

Signature · 1 line

error compiling committee.c: too many arguments to function

Rusty Russell

author

2008-12-08 06:08 UTC #3 lore raw

Quoted 18 lines · On Monday 08 December 2008 02:44:00 Avi Kivity wrote:

Quoted 11 lines · Avi Kivity
Quoted 4 lines · Rusty Russell
We're getting rid on on-stack cpumasks for large NR_CPUS.
1) Use cpumask_var_t and alloc_cpumask_var (a noop normally). Fallback code is inefficient but never happens in practice.
Wow, code duplication from Rusty. Things must be bad.
Since we're in a get_cpu() here, how about a per_cpu static cpumask instead? I don't mind the inefficient fallback, just the duplication.
Btw, for the general case, instead of forcing everyone to duplicate, how about:
cpumask_var_t cpus;

Quoted 7 lines

with_cpumask(cpus) {
... code to populate cpus
smp_call_function_some(...);
} end_with_cpumask(cpus);
Where with_cpumask() allocates cpus, and uses a mutex + static fallback on failure.

I'd prefer not to hide deadlocks that way :(

I'll re-battle with that code to neaten it. There are only a few places which have these kind of issues.

Thanks,
Rusty.

Avi Kivity

2008-12-08 09:49 UTC #4 lore raw

Quoted 9 lines · Rusty Russell

Quoted 9 lines
Btw, for the general case, instead of forcing everyone to duplicate, how about:
cpumask_var_t cpus;
with_cpumask(cpus) {
... code to populate cpus
smp_call_function_some(...);
} end_with_cpumask(cpus);

Quoted 9 lines

Quoted 2 lines
Where with_cpumask() allocates cpus, and uses a mutex + static fallback on failure.
I'd prefer not to hide deadlocks that way :(
I'll re-battle with that code to neaten it. There are only a few places which have these kind of issues.

cpuvar_get_maybe_mutex_lock(...);
...
cpuvar_put_maybe_mutex_unlock(...);

Signature · 2 lines

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

Rusty Russell

author

2008-12-08 11:55 UTC #5 lore raw

Quoted 17 lines · On Monday 08 December 2008 20:19:57 Avi Kivity wrote:

Quoted 16 lines · Rusty Russell
Quoted 5 lines
Btw, for the general case, instead of forcing everyone to duplicate, how about:
cpumask_var_t cpus;
Quoted 8 lines
with_cpumask(cpus) {
... code to populate cpus
smp_call_function_some(...);
} end_with_cpumask(cpus);
Where with_cpumask() allocates cpus, and uses a mutex + static fallback on failure.
I'd prefer not to hide deadlocks that way :(

Quoted 8 lines

Quoted 4 lines
I'll re-battle with that code to neaten it. There are only a few places which have these kind of issues.
cpuvar_get_maybe_mutex_lock(...);
...
cpuvar_put_maybe_mutex_unlock(...);

My thought was something like:

/* This is an empty struct for !CONFIG_CPUMASK_OFFSTACK. */ static struct cpuvar_with_mutex_fallback fallback;

...

	cpumask_var_t tmp;

	cpuvar_alloc_fallback(&tmp, &fallback);
	...
	cpuvar_free_fallback(tmp, &fallback);

We may get there eventually, but so far I've managed to produce less horrendous code in every case.

Cheers,
Rusty.

Rusty Russell

author

2008-12-08 09:56 UTC #6 lore raw

PATCH 1/1 kvm: Extract core of kvm_flush_remote_tlbs/kvm_reload_remote_mmus.

In reply to Avi Kivity

Quoted 1 line · Avi

Wow, code duplication from Rusty. Things must be bad.

Something about glass houses comes to mind. But instead, a patch.

Signed-off-by Rusty Russell <[email protected]>

---
 virt/kvm/kvm_main.c |   44 +++++++++++++++-----------------------------
 1 file changed, 15 insertions(+), 29 deletions(-)

virt/kvm/kvm_main.c +15 ~~-29~~

@@ -355,10 +355,11 @@ static void ack_flush(void *_completed)
 {
 }
 
-void kvm_flush_remote_tlbs(struct kvm *kvm)
+static bool make_all_cpus_request(struct kvm *kvm, unsigned int req)
 {
 	int i, cpu, me;
 	cpumask_t cpus;
+	bool called = false;
 	struct kvm_vcpu *vcpu;
 
 	me = get_cpu();

@@ -367,45 +368,30 @@ void kvm_flush_remote_tlbs(struct kvm *k
 		vcpu = kvm->vcpus[i];
 		if (!vcpu)
 			continue;
-		if (test_and_set_bit(KVM_REQ_TLB_FLUSH, &vcpu->requests))
+		if (test_and_set_bit(req, &vcpu->requests))
 			continue;
 		cpu = vcpu->cpu;
 		if (cpu != -1 && cpu != me)
 			cpu_set(cpu, cpus);
 	}
-	if (cpus_empty(cpus))
-		goto out;
-	++kvm->stat.remote_tlb_flush;
-	smp_call_function_mask(cpus, ack_flush, NULL, 1);
-out:
+	if (!cpus_empty(cpus)) {
+		smp_call_function_mask(cpus, ack_flush, NULL, 1);
+		called = true;
+	}
 	put_cpu();
+	return called;
+}
+
+void kvm_flush_remote_tlbs(struct kvm *kvm)
+{
+	if (make_all_cpus_request(kvm, KVM_REQ_TLB_FLUSH))
+		++kvm->stat.remote_tlb_flush;
 }
 
 void kvm_reload_remote_mmus(struct kvm *kvm)
 {
-	int i, cpu, me;
-	cpumask_t cpus;
-	struct kvm_vcpu *vcpu;
-
-	me = get_cpu();
-	cpus_clear(cpus);
-	for (i = 0; i < KVM_MAX_VCPUS; ++i) {
-		vcpu = kvm->vcpus[i];
-		if (!vcpu)
-			continue;
-		if (test_and_set_bit(KVM_REQ_MMU_RELOAD, &vcpu->requests))
-			continue;
-		cpu = vcpu->cpu;
-		if (cpu != -1 && cpu != me)
-			cpu_set(cpu, cpus);
-	}
-	if (cpus_empty(cpus))
-		goto out;
-	smp_call_function_mask(cpus, ack_flush, NULL, 1);
-out:
-	put_cpu();
+	make_all_cpus_request(kvm, KVM_REQ_MMU_RELOAD);
 }
-
 
 int kvm_vcpu_init(struct kvm_vcpu *vcpu, struct kvm *kvm, unsigned id)
 {

Rusty Russell

author

2008-12-08 09:58 UTC #7 lore raw

We're getting rid on on-stack cpumasks for large NR_CPUS.

1) Use cpumask_var_t/alloc_cpumask_var.
2) smp_call_function_mask -> smp_call_function_many
3) cpus_clear, cpus_empty, cpu_set -> cpumask_clear, cpumask_empty, cpumask_set_cpu.

This actually generates slightly smaller code than the old one with CONFIG_CPUMASKS_OFFSTACK=n. (gcc knows that cpus cannot be NULL in that case, where cpumask_var_t is cpumask_t[1]).

Signed-off-by Rusty Russell <[email protected]>

---
 virt/kvm/kvm_main.c |   23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

virt/kvm/kvm_main.c +14 -9

@@ -358,12 +358,14 @@ static bool make_all_cpus_request(struct
 static bool make_all_cpus_request(struct kvm *kvm, unsigned int req)
 {
 	int i, cpu, me;
-	cpumask_t cpus;
-	bool called = false;
+	cpumask_var_t cpus;
+	bool called = true;
 	struct kvm_vcpu *vcpu;
 
+	if (alloc_cpumask_var(&cpus, GFP_ATOMIC))
+		cpumask_clear(cpus);
+
 	me = get_cpu();
-	cpus_clear(cpus);
 	for (i = 0; i < KVM_MAX_VCPUS; ++i) {
 		vcpu = kvm->vcpus[i];
 		if (!vcpu)

@@ -371,14 +373,17 @@ static bool make_all_cpus_request(struct
 		if (test_and_set_bit(req, &vcpu->requests))
 			continue;
 		cpu = vcpu->cpu;
-		if (cpu != -1 && cpu != me)
-			cpu_set(cpu, cpus);
+		if (cpus != NULL && cpu != -1 && cpu != me)
+			cpumask_set_cpu(cpu, cpus);
 	}
-	if (!cpus_empty(cpus)) {
-		smp_call_function_mask(cpus, ack_flush, NULL, 1);
-		called = true;
-	}
+	if (unlikely(cpus == NULL))
+		smp_call_function_many(cpu_online_mask, ack_flush, NULL, 1);
+	else if (!cpumask_empty(cpus))
+		smp_call_function_many(cpus, ack_flush, NULL, 1);
+	else
+		called = false;
 	put_cpu();
+	free_cpumask_var(cpus);
 	return called;
 }

Avi Kivity

Applied

2008-12-08 12:00 UTC #8 lore raw

Re: [PATCH] kvm: Extract core of kvm_flush_remote_tlbs/kvm_reload_remote_mmus.

In reply to Rusty Russell

Quoted 7 lines · Rusty Russell

Quoted 2 lines · Avi
Wow, code duplication from Rusty. Things must be bad.
Something about glass houses comes to mind. But instead, a patch.

Er, right, eek.

I've applied all three patches, thanks.

Signature · 1 line

error compiling committee.c: too many arguments to function

Mike Travis

2008-12-08 14:32 UTC #9 lore raw

In reply to Avi Kivity

Quoted 38 lines · Avi Kivity

Quoted 24 lines · Rusty Russell
We're getting rid on on-stack cpumasks for large NR_CPUS.
1) Use cpumask_var_t and alloc_cpumask_var (a noop normally). Fallback code is inefficient but never happens in practice.
2) smp_call_function_mask -> smp_call_function_many
3) cpus_clear, cpus_empty, cpu_set -> cpumask_clear, cpumask_empty, cpumask_set_cpu.
virt/kvm/kvm_main.c +5 -2
@@ -358,11 +358,23 @@ static void ack_flush(void *_completed)
 void kvm_flush_remote_tlbs(struct kvm *kvm)
 {
     int i, cpu, me;
-    cpumask_t cpus;
+    cpumask_var_t cpus;
     struct kvm_vcpu *vcpu;
 
     me = get_cpu();
-    cpus_clear(cpus);
+    if (!alloc_cpumask_var(&cpus, GFP_ATOMIC)) {
+        /* Slow path on failure.  Call everyone. */
+        for (i = 0; i < KVM_MAX_VCPUS; ++i) {
+            vcpu = kvm->vcpus[i];
virt/kvm/kvm_main.c static void ack_flush(void *_completed) · line 378
Quoted 10 lines
virt/kvm/kvm_main.c +9 -0
+            if (vcpu)
+                set_bit(KVM_REQ_TLB_FLUSH, &vcpu->requests);
+        }
+        ++kvm->stat.remote_tlb_flush;
+        smp_call_function_many(cpu_online_mask, ack_flush, NULL, 1);
+        put_cpu();
+        return;
+    }
+
 
Wow, code duplication from Rusty. Things must be bad.

Quoted 3 lines

Since we're in a get_cpu() here, how about a per_cpu static cpumask instead? I don't mind the inefficient fallback, just the duplication.

One thing to note is that when CPUMASK_OFFSTACK=n, then alloc_cpumask_var returns a constant 1 and the duplicate code is not even compiled.

Avi Kivity

2008-12-08 14:56 UTC #10 lore raw

Quoted 7 lines · Mike Travis

Quoted 3 lines
Since we're in a get_cpu() here, how about a per_cpu static cpumask instead? I don't mind the inefficient fallback, just the duplication.
One thing to note is that when CPUMASK_OFFSTACK=n, then alloc_cpumask_var returns a constant 1 and the duplicate code is not even compiled.

I'm a lot more concerned about source duplication than binary duplication. Rusty's patches resulted in a net reduction in duplication, so perhaps I should keep quiet about it.

Signature · 1 line

error compiling committee.c: too many arguments to function