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

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

[Avi Kivity]

Rusty Russell wrote:
> 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.
>
> --- linux-2.6.orig/virt/kvm/kvm_main.c
> +++ linux-2.6/virt/kvm/kvm_main.c
> @@ -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;
> +	}
> +
>   

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.

-- 
error compiling committee.c: too many arguments to function

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

[Avi Kivity]

Avi Kivity wrote:
> Rusty Russell wrote:
>> 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).

-- 
error compiling committee.c: too many arguments to function

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

[Rusty Russell]

On Monday 08 December 2008 02:44:00 Avi Kivity wrote:
> Avi Kivity wrote:
> > Rusty Russell wrote:
> >> 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.

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.

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

[Avi Kivity]

Rusty Russell wrote:
>> 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.
>>     
>
> 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(...);

?

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

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

[Rusty Russell]

On Monday 08 December 2008 20:19:57 Avi Kivity wrote:
> Rusty Russell wrote:
> >> 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.
> >>     
> >
> > 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(...);

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.

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

[Rusty Russell]

Avi said:
> 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 <rusty@rustcorp.com.au>
---
 virt/kvm/kvm_main.c |   44 +++++++++++++++-----------------------------
 1 file changed, 15 insertions(+), 29 deletions(-)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -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)
 {

kvm: use modern cpumask primitives, no cpumask_t on stack

[Rusty Russell]

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 <rusty@rustcorp.com.au>
---
 virt/kvm/kvm_main.c |   23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -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;
 }
 

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

[Avi Kivity]

Rusty Russell wrote:
> Avi said:
>   
>> 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.

-- 
error compiling committee.c: too many arguments to function

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

[Mike Travis]

Avi Kivity wrote:
> Rusty Russell wrote:
>> 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.
>>
>> --- linux-2.6.orig/virt/kvm/kvm_main.c
>> +++ linux-2.6/virt/kvm/kvm_main.c
>> @@ -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;
>> +    }
>> +
>>   
> 
> 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.
> 

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.

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

[Avi Kivity]

Mike Travis wrote:
>> 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.

-- 
error compiling committee.c: too many arguments to function