KVM pvmmu: do not batch pte updates from interrupt context

KVM pvmmu: do not batch pte updates from interrupt context

[Marcelo Tosatti]

Commit b8bcfe997e4 made paravirt pte updates synchronous in interrupt
context.

Unfortunately the KVM pv mmu code caches the lazy/nonlazy mode
internally, so a pte update from interrupt context during a lazy mmu
operation can be batched while it should be performed synchronously.

https://bugzilla.redhat.com/show_bug.cgi?id=518022

Drop the internal mode variable and use paravirt_get_lazy_mode(), which
returns the correct state.

Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>

diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
index c664d51..63b0ec8 100644
--- a/arch/x86/kernel/kvm.c
+++ b/arch/x86/kernel/kvm.c
@@ -34,7 +34,6 @@
 struct kvm_para_state {
 	u8 mmu_queue[MMU_QUEUE_SIZE];
 	int mmu_queue_len;
-	enum paravirt_lazy_mode mode;
 };
 
 static DEFINE_PER_CPU(struct kvm_para_state, para_state);
@@ -77,7 +76,7 @@ static void kvm_deferred_mmu_op(void *buffer, int len)
 {
 	struct kvm_para_state *state = kvm_para_state();
 
-	if (state->mode != PARAVIRT_LAZY_MMU) {
+	if (paravirt_get_lazy_mode() != PARAVIRT_LAZY_MMU) {
 		kvm_mmu_op(buffer, len);
 		return;
 	}
@@ -185,10 +184,7 @@ static void kvm_release_pt(unsigned long pfn)
 
 static void kvm_enter_lazy_mmu(void)
 {
-	struct kvm_para_state *state = kvm_para_state();
-
 	paravirt_enter_lazy_mmu();
-	state->mode = paravirt_get_lazy_mode();
 }
 
 static void kvm_leave_lazy_mmu(void)
@@ -197,7 +193,6 @@ static void kvm_leave_lazy_mmu(void)
 
 	mmu_queue_flush(state);
 	paravirt_leave_lazy_mmu();
-	state->mode = paravirt_get_lazy_mode();
 }
 
 static void __init paravirt_ops_setup(void)

Re: KVM pvmmu: do not batch pte updates from interrupt context

[Avi Kivity]

On 08/25/2009 07:13 AM, Marcelo Tosatti wrote:
> Commit b8bcfe997e4 made paravirt pte updates synchronous in interrupt
> context.
>
> Unfortunately the KVM pv mmu code caches the lazy/nonlazy mode
> internally, so a pte update from interrupt context during a lazy mmu
> operation can be batched while it should be performed synchronously.
>
> https://bugzilla.redhat.com/show_bug.cgi?id=518022
>
> Drop the internal mode variable and use paravirt_get_lazy_mode(), which
> returns the correct state.
>
>    

It looks good and I'd like to get it into 2.6.31.  Do we have any 
reports it fixes the problem?

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

Re: KVM pvmmu: do not batch pte updates from interrupt context

[Marcelo Tosatti]

On Thu, Aug 27, 2009 at 11:11:11AM +0300, Avi Kivity wrote:
> On 08/25/2009 07:13 AM, Marcelo Tosatti wrote:
>> Commit b8bcfe997e4 made paravirt pte updates synchronous in interrupt
>> context.
>>
>> Unfortunately the KVM pv mmu code caches the lazy/nonlazy mode
>> internally, so a pte update from interrupt context during a lazy mmu
>> operation can be batched while it should be performed synchronously.
>>
>> https://bugzilla.redhat.com/show_bug.cgi?id=518022
>>
>> Drop the internal mode variable and use paravirt_get_lazy_mode(), which
>> returns the correct state.
>>
>>    
>
> It looks good and I'd like to get it into 2.6.31.  Do we have any  
> reports it fixes the problem?

No, it seems difficult to reproduce. If you look at the trace:

Process wpa_supplicant (pid: 192, ti=c2160000 task=f63cab80
task.ti=c2160000)
Stack:
 481d84b0 000005a8 000005a8 000005a8 c2161aec c0774a72 000005a8 00000001
<0> c2161ad8 f5ea8f00 fffb680c 00000000 000005a8 481d84b0 c2161b68
000005a8
<0> 00000000 c2161b04 f7d7af3c 000005a8 481d84b0 0000080c f62aa748
c2161b34
Call Trace:
 [<c0774a72>] ? skb_copy_bits+0x5e/0x1a0
 [<f7d7af3c>] ? xdr_skb_read_bits+0x34/0x60 [sunrpc]
 [<f7d7ad89>] ? xdr_partial_copy_from_skb+0x121/0x185 [sunrpc]
 [<f7d7af08>] ? xdr_skb_read_bits+0x0/0x60 [sunrpc]
 [<f7d7c700>] ? xs_tcp_data_recv+0x371/0x53b [sunrpc]
 [<f7d7af08>] ? xdr_skb_read_bits+0x0/0x60 [sunrpc]
 [<c07b6dbc>] ? tcp_read_sock+0x7d/0x193
 [<f7d7c38f>] ? xs_tcp_data_recv+0x0/0x53b [sunrpc]
 [<f7d7c36b>] ? xs_tcp_data_ready+0x6a/0x8e [sunrpc]
 [<c07be7db>] ? tcp_rcv_established+0x4fe/0x63a
 [<c07c5603>] ? tcp_v4_do_rcv+0x171/0x2c7
 [<c07c695c>] ? tcp_v4_rcv+0x3ea/0x5e2
 [<c07ab5b4>] ? ip_local_deliver_finish+0x13f/0x1ee
 [<c07ab6d7>] ? ip_local_deliver+0x74/0x8d
 [<c07ab05e>] ? ip_rcv_finish+0x31f/0x35a
 [<c07ab2bb>] ? ip_rcv+0x222/0x266
 [<c077df66>] ? netif_receive_skb+0x38e/0x3bf
 [<f7d39714>] ? virtnet_poll+0x4ca/0x633 [virtio_net]
 [<c077b9e0>] ? __rcu_read_lock+0x0/0x45
 [<c077e724>] ? net_rx_action+0xa7/0x1d3
 [<c044adbb>] ? __do_softirq+0x60/0x192
 [<c044ae23>] ? __do_softirq+0xc8/0x192
 [<c044af36>] ? do_softirq+0x49/0x7f
 [<c044b08a>] ? irq_exit+0x48/0x8c
 [<c0405705>] ? do_IRQ+0x92/0xb7
 [<c0404095>] ? common_interrupt+0x35/0x3c
 [<c04264c5>] ? paravirt_leave_lazy_mmu+0x0/0x22
 [<c042593d>] ? kvm_leave_lazy_mmu+0x5c/0x7e
 [<c04d74a5>] ? unmap_vmas+0x489/0x5c0
 [<c0446200>] ? task_unlock+0xe/0x34
 [<c04dc13f>] ? exit_mmap+0xb2/0x113
 [<c04421b3>] ? mmput+0x57/0xc0
 [<c0446492>] ? exit_mm+0xeb/0x104
 [<c0447fd0>] ? do_exit+0x19e/0x648
 [<c046f278>] ? trace_hardirqs_on_caller+0x122/0x155
 [<c04484ec>] ? do_group_exit+0x72/0x99
 [<c044853a>] ? sys_exit_group+0x27/0x3c
 [<c0403a50>] ? syscall_call+0x7/0xb

xdr_partial_copy_from_skb uses kmap_atomic.