Message ID | 20090825041310.GA15313@amt.cnet (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
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?
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. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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)
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> -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html