[PATCH] kvmppc_init_timing_stats: fix sleep with interrupts

[PATCH] kvmppc_init_timing_stats: fix sleep with interrupts

[Scott Wood]

It is not legal to call mutex_lock() with interrupts disabled.
This will assert with debug checks enabled.

If there's a real need to disable interrupts here, it could be done
after the mutex is acquired -- but I don't see why it's needed at all.

Signed-off-by: Scott Wood <scottwood@freescale.com>
---
 arch/powerpc/kvm/timing.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/kvm/timing.c b/arch/powerpc/kvm/timing.c
index 46fa04f..a021f58 100644
--- a/arch/powerpc/kvm/timing.c
+++ b/arch/powerpc/kvm/timing.c
@@ -35,7 +35,6 @@ void kvmppc_init_timing_stats(struct kvm_vcpu *vcpu)
 	int i;
 
 	/* pause guest execution to avoid concurrent updates */
-	local_irq_disable();
 	mutex_lock(&vcpu->mutex);
 
 	vcpu->arch.last_exit_type = 0xDEAD;
@@ -51,7 +50,6 @@ void kvmppc_init_timing_stats(struct kvm_vcpu *vcpu)
 	vcpu->arch.timing_last_enter.tv64 = 0;
 
 	mutex_unlock(&vcpu->mutex);
-	local_irq_enable();
 }
 
 static void add_exit_timing(struct kvm_vcpu *vcpu, u64 duration, int type)
-- 
1.7.0.4

Re: [PATCH] kvmppc_init_timing_stats: fix sleep with interrupts disabled

[Alexander Graf]

On 30.09.2010, at 21:28, Scott Wood wrote:

> It is not legal to call mutex_lock() with interrupts disabled.
> This will assert with debug checks enabled.
> 
> If there's a real need to disable interrupts here, it could be done
> after the mutex is acquired -- but I don't see why it's needed at all.

Christian, IIRC this code is yours. Any comments?

Alex

> 
> Signed-off-by: Scott Wood <scottwood@freescale.com>
> ---
> arch/powerpc/kvm/timing.c |    2 --
> 1 files changed, 0 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/powerpc/kvm/timing.c b/arch/powerpc/kvm/timing.c
> index 46fa04f..a021f58 100644
> --- a/arch/powerpc/kvm/timing.c
> +++ b/arch/powerpc/kvm/timing.c
> @@ -35,7 +35,6 @@ void kvmppc_init_timing_stats(struct kvm_vcpu *vcpu)
> 	int i;
> 
> 	/* pause guest execution to avoid concurrent updates */
> -	local_irq_disable();
> 	mutex_lock(&vcpu->mutex);
> 
> 	vcpu->arch.last_exit_type = 0xDEAD;
> @@ -51,7 +50,6 @@ void kvmppc_init_timing_stats(struct kvm_vcpu *vcpu)
> 	vcpu->arch.timing_last_enter.tv64 = 0;
> 
> 	mutex_unlock(&vcpu->mutex);
> -	local_irq_enable();
> }
> 
> static void add_exit_timing(struct kvm_vcpu *vcpu, u64 duration, int type)
> -- 
> 1.7.0.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] kvmppc_init_timing_stats: fix sleep with interrupts disabled

[Christian Ehrhardt]

On 10/01/2010 12:07 AM, Alexander Graf wrote:
>
> On 30.09.2010, at 21:28, Scott Wood wrote:
>
>> It is not legal to call mutex_lock() with interrupts disabled.
>> This will assert with debug checks enabled.
>>
>> If there's a real need to disable interrupts here, it could be done
>> after the mutex is acquired -- but I don't see why it's needed at all.
>
> Christian, IIRC this code is yours. Any comments?

Yes, It should be save to drop the irq_disable.
IIRC it was without the lock in early stages of development (pre 
upstream git, so you can't see it) which had some obvious races.
But the lock makes it mutually exclusive per vcpu and should ensure it 
isn't running - by that it should be fine.

Reviewed-by: Christian Ehrhardt <ehrhardt@linux.vnet.ibm.com>

> Alex
>
>>
>> Signed-off-by: Scott Wood<scottwood@freescale.com>
>> ---
>> arch/powerpc/kvm/timing.c |    2 --
>> 1 files changed, 0 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/powerpc/kvm/timing.c b/arch/powerpc/kvm/timing.c
>> index 46fa04f..a021f58 100644
>> --- a/arch/powerpc/kvm/timing.c
>> +++ b/arch/powerpc/kvm/timing.c
>> @@ -35,7 +35,6 @@ void kvmppc_init_timing_stats(struct kvm_vcpu *vcpu)
>> 	int i;
>>
>> 	/* pause guest execution to avoid concurrent updates */
>> -	local_irq_disable();
>> 	mutex_lock(&vcpu->mutex);
>>
>> 	vcpu->arch.last_exit_type = 0xDEAD;
>> @@ -51,7 +50,6 @@ void kvmppc_init_timing_stats(struct kvm_vcpu *vcpu)
>> 	vcpu->arch.timing_last_enter.tv64 = 0;
>>
>> 	mutex_unlock(&vcpu->mutex);
>> -	local_irq_enable();
>> }
>>
>> static void add_exit_timing(struct kvm_vcpu *vcpu, u64 duration, int type)
>> --
>> 1.7.0.4
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

-- 

Grüsse / regards, Christian Ehrhardt
IBM Linux Technology Center, System z Linux Performance

Re: [PATCH] kvmppc_init_timing_stats: fix sleep with interrupts disabled

[Alexander Graf]

On 04.10.2010, at 07:22, Christian Ehrhardt wrote:

> 
> On 10/01/2010 12:07 AM, Alexander Graf wrote:
>> 
>> On 30.09.2010, at 21:28, Scott Wood wrote:
>> 
>>> It is not legal to call mutex_lock() with interrupts disabled.
>>> This will assert with debug checks enabled.
>>> 
>>> If there's a real need to disable interrupts here, it could be done
>>> after the mutex is acquired -- but I don't see why it's needed at all.
>> 
>> Christian, IIRC this code is yours. Any comments?
> 
> Yes, It should be save to drop the irq_disable.
> IIRC it was without the lock in early stages of development (pre upstream git, so you can't see it) which had some obvious races.
> But the lock makes it mutually exclusive per vcpu and should ensure it isn't running - by that it should be fine.
> 
> Reviewed-by: Christian Ehrhardt <ehrhardt@linux.vnet.ibm.com>

Ok, applied on my kvm ppc branch.


Alex