diff mbox

KVM: async_pf: Fix rcu_irq_enter/exit() usage

Message ID 1496837040-8200-1-git-send-email-wanpeng.li@hotmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Wanpeng Li June 7, 2017, 12:04 p.m. UTC
From: Wanpeng Li <wanpeng.li@hotmail.com>

Commit 9b132fbe5419 (Add rcu user eqs exception hooks for async page fault) 
adds rcu_irq_enter/exit() to kvm_async_pf_task_wait() to exit cpu idle eqs 
when needed, to protect the code that needs use rcu. There is no need to call 
this pairs if async page fault is not triggered from idle task.

This patch fixes it by rcu irq exit if it is not triggered from idle task to 
avoid rcu hang after schedule() in the for loop.

Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Radim Krčmář <rkrcmar@redhat.com>
Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com>
---
 arch/x86/kernel/kvm.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Comments

Paolo Bonzini June 7, 2017, 1:05 p.m. UTC | #1
On 07/06/2017 14:04, Wanpeng Li wrote:
> From: Wanpeng Li <wanpeng.li@hotmail.com>
> 
> Commit 9b132fbe5419 (Add rcu user eqs exception hooks for async page fault) 
> adds rcu_irq_enter/exit() to kvm_async_pf_task_wait() to exit cpu idle eqs 
> when needed, to protect the code that needs use rcu. There is no need to call 
> this pairs if async page fault is not triggered from idle task.
> 
> This patch fixes it by rcu irq exit if it is not triggered from idle task to 
> avoid rcu hang after schedule() in the for loop.

How does the bug manifest?

Paolo

> 
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Radim Krčmář <rkrcmar@redhat.com>
> Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com>
> ---
>  arch/x86/kernel/kvm.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
> index 43e10d6..e70ed72 100644
> --- a/arch/x86/kernel/kvm.c
> +++ b/arch/x86/kernel/kvm.c
> @@ -141,6 +141,8 @@ void kvm_async_pf_task_wait(u32 token)
>  	n.token = token;
>  	n.cpu = smp_processor_id();
>  	n.halted = is_idle_task(current) || preempt_count() > 1;
> +	if (!n.halted)
> +		rcu_irq_exit();
>  	init_swait_queue_head(&n.wq);
>  	hlist_add_head(&n.link, &b->list);
>  	raw_spin_unlock(&b->lock);
> @@ -167,8 +169,9 @@ void kvm_async_pf_task_wait(u32 token)
>  	}
>  	if (!n.halted)
>  		finish_swait(&n.wq, &wait);
> +	else
> +		rcu_irq_exit();
>  
> -	rcu_irq_exit();
>  	return;
>  }
Wanpeng Li June 7, 2017, 2:10 p.m. UTC | #2
2017-06-07 21:05 GMT+08:00 Paolo Bonzini <pbonzini@redhat.com>:
>
>
> On 07/06/2017 14:04, Wanpeng Li wrote:
>> From: Wanpeng Li <wanpeng.li@hotmail.com>
>>
>> Commit 9b132fbe5419 (Add rcu user eqs exception hooks for async page fault)
>> adds rcu_irq_enter/exit() to kvm_async_pf_task_wait() to exit cpu idle eqs
>> when needed, to protect the code that needs use rcu. There is no need to call
>> this pairs if async page fault is not triggered from idle task.
>>
>> This patch fixes it by rcu irq exit if it is not triggered from idle task to
>> avoid rcu hang after schedule() in the for loop.
>
> How does the bug manifest?

Just by codes review.

Regards,
Wanpeng Li
Paolo Bonzini June 7, 2017, 2:24 p.m. UTC | #3
On 07/06/2017 16:10, Wanpeng Li wrote:
> 2017-06-07 21:05 GMT+08:00 Paolo Bonzini <pbonzini@redhat.com>:
>>
>>
>> On 07/06/2017 14:04, Wanpeng Li wrote:
>>> From: Wanpeng Li <wanpeng.li@hotmail.com>
>>>
>>> Commit 9b132fbe5419 (Add rcu user eqs exception hooks for async page fault)
>>> adds rcu_irq_enter/exit() to kvm_async_pf_task_wait() to exit cpu idle eqs
>>> when needed, to protect the code that needs use rcu. There is no need to call
>>> this pairs if async page fault is not triggered from idle task.
>>>
>>> This patch fixes it by rcu irq exit if it is not triggered from idle task to
>>> avoid rcu hang after schedule() in the for loop.
>>
>> How does the bug manifest?
> 
> Just by codes review.

So it's just code cleanup?

Paolo
Wanpeng Li June 8, 2017, 8 a.m. UTC | #4
2017-06-07 22:24 GMT+08:00 Paolo Bonzini <pbonzini@redhat.com>:
>
>
> On 07/06/2017 16:10, Wanpeng Li wrote:
>> 2017-06-07 21:05 GMT+08:00 Paolo Bonzini <pbonzini@redhat.com>:
>>>
>>>
>>> On 07/06/2017 14:04, Wanpeng Li wrote:
>>>> From: Wanpeng Li <wanpeng.li@hotmail.com>
>>>>
>>>> Commit 9b132fbe5419 (Add rcu user eqs exception hooks for async page fault)
>>>> adds rcu_irq_enter/exit() to kvm_async_pf_task_wait() to exit cpu idle eqs
>>>> when needed, to protect the code that needs use rcu. There is no need to call
>>>> this pairs if async page fault is not triggered from idle task.
>>>>
>>>> This patch fixes it by rcu irq exit if it is not triggered from idle task to
>>>> avoid rcu hang after schedule() in the for loop.
>>>
>>> How does the bug manifest?
>>
>> Just by codes review.
>
> So it's just code cleanup?

Yeah, this should be a cleanup and I will update the patch
description. Actually, I observe a hang due to async pf injected to L2
which should be injected to L1, then tasks hang in L1 due to missing
PAGE_READY async_pfs.

Regards,
Wanpeng Li
diff mbox

Patch

diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
index 43e10d6..e70ed72 100644
--- a/arch/x86/kernel/kvm.c
+++ b/arch/x86/kernel/kvm.c
@@ -141,6 +141,8 @@  void kvm_async_pf_task_wait(u32 token)
 	n.token = token;
 	n.cpu = smp_processor_id();
 	n.halted = is_idle_task(current) || preempt_count() > 1;
+	if (!n.halted)
+		rcu_irq_exit();
 	init_swait_queue_head(&n.wq);
 	hlist_add_head(&n.link, &b->list);
 	raw_spin_unlock(&b->lock);
@@ -167,8 +169,9 @@  void kvm_async_pf_task_wait(u32 token)
 	}
 	if (!n.halted)
 		finish_swait(&n.wq, &wait);
+	else
+		rcu_irq_exit();
 
-	rcu_irq_exit();
 	return;
 }
 EXPORT_SYMBOL_GPL(kvm_async_pf_task_wait);