[PATCH v3] KVM: async_pf: rcu irq exit if not triggered from idle task

[PATCH v3] KVM: async_pf: rcu irq exit if not triggered from idle task

[Wanpeng Li]

From: Wanpeng Li <wanpeng.li@hotmail.com>

 WARNING: CPU: 5 PID: 1242 at kernel/rcu/tree_plugin.h:323 rcu_note_context_switch+0x207/0x6b0
 CPU: 5 PID: 1242 Comm: unity-settings- Not tainted 4.13.0-rc2+ #1
 RIP: 0010:rcu_note_context_switch+0x207/0x6b0
 Call Trace:
  __schedule+0xda/0xba0
  ? kvm_async_pf_task_wait+0x1b2/0x270
  schedule+0x40/0x90
  kvm_async_pf_task_wait+0x1cc/0x270
  ? prepare_to_swait+0x22/0x70
  do_async_page_fault+0x77/0xb0
  ? do_async_page_fault+0x77/0xb0
  async_page_fault+0x28/0x30
 RIP: 0010:__d_lookup_rcu+0x90/0x1e0

I encounter this when trying to stress the async page fault in L1 guest w/ 
L2 guests running.

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 invokes rcu irq exit if it is not triggered from idle task.

Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Radim Krčmář <rkrcmar@redhat.com>
Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com>
---
v2 -> v3: 
 * add the warning log
v1 -> v2:
 * update patch description

 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 71c17a5..c6a7667 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);
-- 
2.7.4

Re: [PATCH v3] KVM: async_pf: rcu irq exit if not triggered from idle task

[Paolo Bonzini]

On 27/07/2017 11:05, Wanpeng Li wrote:
> 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 is true, but I think the patch is making things more complex where
it could be simplifying them.  Right now, the "native_safe_halt" branch
is calling rcu_irq_exit/enter but the "schedule" branch is not.  Could
you just pull rcu_irq_exit/enter outside the "if", so that you inform
the RCU subsystem even in the !n.halted case?

Paolo

Re: [PATCH v3] KVM: async_pf: rcu irq exit if not triggered from idle task

[Wanpeng Li]

2017-08-01 18:37 GMT+08:00 Paolo Bonzini <pbonzini@redhat.com>:
> On 27/07/2017 11:05, Wanpeng Li wrote:
>> 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 is true, but I think the patch is making things more complex where
> it could be simplifying them.  Right now, the "native_safe_halt" branch
> is calling rcu_irq_exit/enter but the "schedule" branch is not.  Could
> you just pull rcu_irq_exit/enter outside the "if", so that you inform
> the RCU subsystem even in the !n.halted case?

How about something like this?

diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
index 71c17a5..d04e30e 100644
--- a/arch/x86/kernel/kvm.c
+++ b/arch/x86/kernel/kvm.c
@@ -151,6 +151,8 @@ void kvm_async_pf_task_wait(u32 token)
         if (hlist_unhashed(&n.link))
             break;

+        rcu_irq_exit();
+
         if (!n.halted) {
             local_irq_enable();
             schedule();
@@ -159,11 +161,11 @@ void kvm_async_pf_task_wait(u32 token)
             /*
              * We cannot reschedule. So halt.
              */
-            rcu_irq_exit();
             native_safe_halt();
             local_irq_disable();
-            rcu_irq_enter();
         }
+
+        rcu_irq_enter();
     }
     if (!n.halted)
         finish_swait(&n.wq, &wait);

Regards,
Wanpeng Li

Re: [PATCH v3] KVM: async_pf: rcu irq exit if not triggered from idle task

[Paolo Bonzini]

On 01/08/2017 13:13, Wanpeng Li wrote:
> 2017-08-01 18:37 GMT+08:00 Paolo Bonzini <pbonzini@redhat.com>:
>> On 27/07/2017 11:05, Wanpeng Li wrote:
>>> 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 is true, but I think the patch is making things more complex where
>> it could be simplifying them.  Right now, the "native_safe_halt" branch
>> is calling rcu_irq_exit/enter but the "schedule" branch is not.  Could
>> you just pull rcu_irq_exit/enter outside the "if", so that you inform
>> the RCU subsystem even in the !n.halted case?
> 
> How about something like this?

If it works, it's perfect. :)

Please add Cc for stable kernels too in v4.

Paolo

> diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
> index 71c17a5..d04e30e 100644
> --- a/arch/x86/kernel/kvm.c
> +++ b/arch/x86/kernel/kvm.c
> @@ -151,6 +151,8 @@ void kvm_async_pf_task_wait(u32 token)
>          if (hlist_unhashed(&n.link))
>              break;
> 
> +        rcu_irq_exit();
> +
>          if (!n.halted) {
>              local_irq_enable();
>              schedule();
> @@ -159,11 +161,11 @@ void kvm_async_pf_task_wait(u32 token)
>              /*
>               * We cannot reschedule. So halt.
>               */
> -            rcu_irq_exit();
>              native_safe_halt();
>              local_irq_disable();
> -            rcu_irq_enter();
>          }
> +
> +        rcu_irq_enter();
>      }
>      if (!n.halted)
>          finish_swait(&n.wq, &wait);
> 
> Regards,
> Wanpeng Li
>