Re: Bug inkvm_set_irq
Le 28/02/2011 11:11, Michael S. Tsirkin a écrit : On Mon, Feb 28, 2011 at 09:56:46AM +0100, Jean-Philippe Menil wrote: Le 27/02/2011 18:00, Michael S. Tsirkin a écrit : On Fri, Feb 25, 2011 at 10:07:22AM +0100, Jean-Philippe Menil wrote: Hi, Each time i try tou use vhost_net, i'm facing a kernel bug. I do a modprobe vhost_net, and start guest whith vhost=on. Following is a trace with a kernel 2.6.37, but i had the same problem with 2.6.36 (cf https://lkml.org/lkml/2010/11/30/29). 2.6.36 had a theorectical race that could explain this, but it should be ok in 2.6.37. The bug only occurs whith vhost_net charged, so i don't know if this is a bug in kvm module code or in the vhost_net code. It could be a bug in eventfd which is the interface used by both kvm and vhost_net. Just for fun, you can try 3.6.38 - eventfd code has been changed a lot in 2.6.38 and if it does not trigger there it's a hint that irqfd is the reason. Feb 23 13:56:19 ayrshire.u06.univ-nantes.prive kernel: [ 685.243100] BUG: unable to handle kernel paging request at 2458 Feb 23 13:56:19 ayrshire.u06.univ-nantes.prive kernel: [ 685.243250] IP: [a041aa8a] kvm_set_irq+0x2a/0x130 [kvm] Could you run markup_oops/ ksymoops on this please? As far as I can see kvm_set_irq can only get a wrong kvm pointer. Unless there's some general memory corruption, I'd guess You can also try comparing the irqfd-kvm pointer in kvm_irqfd_assign irqfd_wakeup and kvm_set_irq in virt/kvm/eventfd.c. Feb 23 13:56:19 ayrshire.u06.univ-nantes.prive kernel: [ 685.243378] PGD 45d363067 PUD 45e77a067 PMD 0 Feb 23 13:56:19 ayrshire.u06.univ-nantes.prive kernel: [ 685.243556] Oops: [#1] SMP Feb 23 13:56:19 ayrshire.u06.univ-nantes.prive kernel: [ 685.243692] last sysfs file: /sys/devices/pci:00/:00:0d.0/:05:00.0/:06:00.0/irq Feb 23 13:56:19 ayrshire.u06.univ-nantes.prive kernel: [ 685.243777] CPU 0 Feb 23 13:56:19 ayrshire.u06.univ-nantes.prive kernel: [ 685.243820] Modules linked in: vhost_net macvtap macvlan tun powernow_k8 mperf cpufreq_userspace cpufreq_stats cpufreq_powersave cpufreq_ondemand fre q_table cpufreq_conservative fuse xt_physdev ip6t_LOG ip6table_filter ip6_tables ipt_LOG xt_multiport xt_limit xt_tcpudp xt_state iptable_filter ip_tables x_tables nf_conntrack_tftp nf_conntrack_ftp nf_connt rack_ipv4 nf_defrag_ipv4 8021q bridge stp ext2 mbcache dm_round_robin dm_multipath nf_conntrack_ipv6 nf_conntrack nf_defrag_ipv6 kvm_amd kvm ipv6 snd_pcm snd_timer snd soundcore snd_page_alloc tpm_tis tpm ps mouse dcdbas tpm_bios processor i2c_nforce2 shpchp pcspkr ghes serio_raw joydev evdev pci_hotplug i2c_core hed button thermal_sys xfs exportfs dm_mod sg sr_mod cdrom usbhid hid usb_storage ses sd_mod enclosu re megaraid_sas ohci_hcd lpfc scsi_transport_fc scsi_tgt bnx2 scsi_mod ehci_hcd [last unloaded: scsi_wait_scan] Feb 23 13:56:19 ayrshire.u06.univ-nantes.prive kernel: [ 685.246123] Feb 23 13:56:19 ayrshire.u06.univ-nantes.prive kernel: [ 685.246123] Pid: 10, comm: kworker/0:1 Not tainted 2.6.37-dsiun-110105 #17 0K543T/PowerEdge M605 Feb 23 13:56:19 ayrshire.u06.univ-nantes.prive kernel: [ 685.246123] RIP: 0010:[a041aa8a] [a041aa8a] kvm_set_irq+0x2a/0x130 [kvm] Feb 23 13:56:19 ayrshire.u06.univ-nantes.prive kernel: [ 685.246123] RSP: 0018:88045fc89d30 EFLAGS: 00010246 Feb 23 13:56:19 ayrshire.u06.univ-nantes.prive kernel: [ 685.246123] RAX: RBX: 001a RCX: 0001 Feb 23 13:56:19 ayrshire.u06.univ-nantes.prive kernel: [ 685.246123] RDX: RSI: RDI: Feb 23 13:56:19 ayrshire.u06.univ-nantes.prive kernel: [ 685.246123] RBP: R08: 0001 R09: 880856a91e48 Feb 23 13:56:19 ayrshire.u06.univ-nantes.prive kernel: [ 685.246123] R10: R11: R12: Feb 23 13:56:19 ayrshire.u06.univ-nantes.prive kernel: [ 685.246123] R13: 0001 R14: R15: Feb 23 13:56:19 ayrshire.u06.univ-nantes.prive kernel: [ 685.246123] FS: 7f617986c710() GS:88007f80() knlGS: Feb 23 13:56:19 ayrshire.u06.univ-nantes.prive kernel: [ 685.246123] CS: 0010 DS: ES: CR0: 8005003b Feb 23 13:56:19 ayrshire.u06.univ-nantes.prive kernel: [ 685.246123] CR2: 2458 CR3: 00045d197000 CR4: 06f0 Feb 23 13:56:19 ayrshire.u06.univ-nantes.prive kernel: [ 685.246123] DR0: DR1: DR2: Feb 23 13:56:19 ayrshire.u06.univ-nantes.prive kernel: [ 685.246123] DR3: DR6: 0ff0 DR7: 0400 Feb 23 13:56:19 ayrshire.u06.univ-nantes.prive kernel: [ 685.246123] Process kworker/0:1 (pid: 10, threadinfo 88045fc88000, task 88085fc53c30) Feb 23 13:56:19 ayrshire.u06.univ-nantes.prive kernel: [
Re: Bug inkvm_set_irq
On Mon, Feb 28, 2011 at 11:40:43AM +0100, Jean-Philippe Menil wrote: Le 28/02/2011 11:11, Michael S. Tsirkin a écrit : On Mon, Feb 28, 2011 at 09:56:46AM +0100, Jean-Philippe Menil wrote: Le 27/02/2011 18:00, Michael S. Tsirkin a écrit : On Fri, Feb 25, 2011 at 10:07:22AM +0100, Jean-Philippe Menil wrote: Hi, Each time i try tou use vhost_net, i'm facing a kernel bug. I do a modprobe vhost_net, and start guest whith vhost=on. Following is a trace with a kernel 2.6.37, but i had the same problem with 2.6.36 (cf https://lkml.org/lkml/2010/11/30/29). 2.6.36 had a theorectical race that could explain this, but it should be ok in 2.6.37. The bug only occurs whith vhost_net charged, so i don't know if this is a bug in kvm module code or in the vhost_net code. It could be a bug in eventfd which is the interface used by both kvm and vhost_net. Just for fun, you can try 3.6.38 - eventfd code has been changed a lot in 2.6.38 and if it does not trigger there it's a hint that irqfd is the reason. Feb 23 13:56:19 ayrshire.u06.univ-nantes.prive kernel: [ 685.243100] BUG: unable to handle kernel paging request at 2458 Feb 23 13:56:19 ayrshire.u06.univ-nantes.prive kernel: [ 685.243250] IP: [a041aa8a] kvm_set_irq+0x2a/0x130 [kvm] Could you run markup_oops/ ksymoops on this please? As far as I can see kvm_set_irq can only get a wrong kvm pointer. Unless there's some general memory corruption, I'd guess You can also try comparing the irqfd-kvm pointer in kvm_irqfd_assign irqfd_wakeup and kvm_set_irq in virt/kvm/eventfd.c. Feb 23 13:56:19 ayrshire.u06.univ-nantes.prive kernel: [ 685.243378] PGD 45d363067 PUD 45e77a067 PMD 0 Feb 23 13:56:19 ayrshire.u06.univ-nantes.prive kernel: [ 685.243556] Oops: [#1] SMP Feb 23 13:56:19 ayrshire.u06.univ-nantes.prive kernel: [ 685.243692] last sysfs file: /sys/devices/pci:00/:00:0d.0/:05:00.0/:06:00.0/irq Feb 23 13:56:19 ayrshire.u06.univ-nantes.prive kernel: [ 685.243777] CPU 0 Feb 23 13:56:19 ayrshire.u06.univ-nantes.prive kernel: [ 685.243820] Modules linked in: vhost_net macvtap macvlan tun powernow_k8 mperf cpufreq_userspace cpufreq_stats cpufreq_powersave cpufreq_ondemand fre q_table cpufreq_conservative fuse xt_physdev ip6t_LOG ip6table_filter ip6_tables ipt_LOG xt_multiport xt_limit xt_tcpudp xt_state iptable_filter ip_tables x_tables nf_conntrack_tftp nf_conntrack_ftp nf_connt rack_ipv4 nf_defrag_ipv4 8021q bridge stp ext2 mbcache dm_round_robin dm_multipath nf_conntrack_ipv6 nf_conntrack nf_defrag_ipv6 kvm_amd kvm ipv6 snd_pcm snd_timer snd soundcore snd_page_alloc tpm_tis tpm ps mouse dcdbas tpm_bios processor i2c_nforce2 shpchp pcspkr ghes serio_raw joydev evdev pci_hotplug i2c_core hed button thermal_sys xfs exportfs dm_mod sg sr_mod cdrom usbhid hid usb_storage ses sd_mod enclosu re megaraid_sas ohci_hcd lpfc scsi_transport_fc scsi_tgt bnx2 scsi_mod ehci_hcd [last unloaded: scsi_wait_scan] Feb 23 13:56:19 ayrshire.u06.univ-nantes.prive kernel: [ 685.246123] Feb 23 13:56:19 ayrshire.u06.univ-nantes.prive kernel: [ 685.246123] Pid: 10, comm: kworker/0:1 Not tainted 2.6.37-dsiun-110105 #17 0K543T/PowerEdge M605 Feb 23 13:56:19 ayrshire.u06.univ-nantes.prive kernel: [ 685.246123] RIP: 0010:[a041aa8a] [a041aa8a] kvm_set_irq+0x2a/0x130 [kvm] Feb 23 13:56:19 ayrshire.u06.univ-nantes.prive kernel: [ 685.246123] RSP: 0018:88045fc89d30 EFLAGS: 00010246 Feb 23 13:56:19 ayrshire.u06.univ-nantes.prive kernel: [ 685.246123] RAX: RBX: 001a RCX: 0001 Feb 23 13:56:19 ayrshire.u06.univ-nantes.prive kernel: [ 685.246123] RDX: RSI: RDI: Feb 23 13:56:19 ayrshire.u06.univ-nantes.prive kernel: [ 685.246123] RBP: R08: 0001 R09: 880856a91e48 Feb 23 13:56:19 ayrshire.u06.univ-nantes.prive kernel: [ 685.246123] R10: R11: R12: Feb 23 13:56:19 ayrshire.u06.univ-nantes.prive kernel: [ 685.246123] R13: 0001 R14: R15: Feb 23 13:56:19 ayrshire.u06.univ-nantes.prive kernel: [ 685.246123] FS: 7f617986c710() GS:88007f80() knlGS: Feb 23 13:56:19 ayrshire.u06.univ-nantes.prive kernel: [ 685.246123] CS: 0010 DS: ES: CR0: 8005003b Feb 23 13:56:19 ayrshire.u06.univ-nantes.prive kernel: [ 685.246123] CR2: 2458 CR3: 00045d197000 CR4: 06f0 Feb 23 13:56:19 ayrshire.u06.univ-nantes.prive kernel: [ 685.246123] DR0: DR1: DR2: Feb 23 13:56:19 ayrshire.u06.univ-nantes.prive kernel: [ 685.246123] DR3: DR6: 0ff0 DR7: 0400 Feb 23 13:56:19 ayrshire.u06.univ-nantes.prive kernel: [ 685.246123] Process kworker/0:1 (pid: 10, threadinfo 88045fc88000, task
Re: Bug inkvm_set_irq
Le 28/02/2011 12:39, Michael S. Tsirkin a écrit : On Mon, Feb 28, 2011 at 11:40:43AM +0100, Jean-Philippe Menil wrote: Le 28/02/2011 11:11, Michael S. Tsirkin a écrit : On Mon, Feb 28, 2011 at 09:56:46AM +0100, Jean-Philippe Menil wrote: Le 27/02/2011 18:00, Michael S. Tsirkin a écrit : On Fri, Feb 25, 2011 at 10:07:22AM +0100, Jean-Philippe Menil wrote: Hi, Each time i try tou use vhost_net, i'm facing a kernel bug. I do a modprobe vhost_net, and start guest whith vhost=on. Following is a trace with a kernel 2.6.37, but i had the same problem with 2.6.36 (cf https://lkml.org/lkml/2010/11/30/29). 2.6.36 had a theorectical race that could explain this, but it should be ok in 2.6.37. The bug only occurs whith vhost_net charged, so i don't know if this is a bug in kvm module code or in the vhost_net code. It could be a bug in eventfd which is the interface used by both kvm and vhost_net. Just for fun, you can try 3.6.38 - eventfd code has been changed a lot in 2.6.38 and if it does not trigger there it's a hint that irqfd is the reason. Feb 23 13:56:19 ayrshire.u06.univ-nantes.prive kernel: [ 685.243100] BUG: unable to handle kernel paging request at 2458 Feb 23 13:56:19 ayrshire.u06.univ-nantes.prive kernel: [ 685.243250] IP: [a041aa8a] kvm_set_irq+0x2a/0x130 [kvm] Could you run markup_oops/ ksymoops on this please? As far as I can see kvm_set_irq can only get a wrong kvm pointer. Unless there's some general memory corruption, I'd guess You can also try comparing the irqfd-kvm pointer in kvm_irqfd_assign irqfd_wakeup and kvm_set_irq in virt/kvm/eventfd.c. Feb 23 13:56:19 ayrshire.u06.univ-nantes.prive kernel: [ 685.243378] PGD 45d363067 PUD 45e77a067 PMD 0 Feb 23 13:56:19 ayrshire.u06.univ-nantes.prive kernel: [ 685.243556] Oops: [#1] SMP Feb 23 13:56:19 ayrshire.u06.univ-nantes.prive kernel: [ 685.243692] last sysfs file: /sys/devices/pci:00/:00:0d.0/:05:00.0/:06:00.0/irq Feb 23 13:56:19 ayrshire.u06.univ-nantes.prive kernel: [ 685.243777] CPU 0 Feb 23 13:56:19 ayrshire.u06.univ-nantes.prive kernel: [ 685.243820] Modules linked in: vhost_net macvtap macvlan tun powernow_k8 mperf cpufreq_userspace cpufreq_stats cpufreq_powersave cpufreq_ondemand fre q_table cpufreq_conservative fuse xt_physdev ip6t_LOG ip6table_filter ip6_tables ipt_LOG xt_multiport xt_limit xt_tcpudp xt_state iptable_filter ip_tables x_tables nf_conntrack_tftp nf_conntrack_ftp nf_connt rack_ipv4 nf_defrag_ipv4 8021q bridge stp ext2 mbcache dm_round_robin dm_multipath nf_conntrack_ipv6 nf_conntrack nf_defrag_ipv6 kvm_amd kvm ipv6 snd_pcm snd_timer snd soundcore snd_page_alloc tpm_tis tpm ps mouse dcdbas tpm_bios processor i2c_nforce2 shpchp pcspkr ghes serio_raw joydev evdev pci_hotplug i2c_core hed button thermal_sys xfs exportfs dm_mod sg sr_mod cdrom usbhid hid usb_storage ses sd_mod enclosu re megaraid_sas ohci_hcd lpfc scsi_transport_fc scsi_tgt bnx2 scsi_mod ehci_hcd [last unloaded: scsi_wait_scan] Feb 23 13:56:19 ayrshire.u06.univ-nantes.prive kernel: [ 685.246123] Feb 23 13:56:19 ayrshire.u06.univ-nantes.prive kernel: [ 685.246123] Pid: 10, comm: kworker/0:1 Not tainted 2.6.37-dsiun-110105 #17 0K543T/PowerEdge M605 Feb 23 13:56:19 ayrshire.u06.univ-nantes.prive kernel: [ 685.246123] RIP: 0010:[a041aa8a] [a041aa8a] kvm_set_irq+0x2a/0x130 [kvm] Feb 23 13:56:19 ayrshire.u06.univ-nantes.prive kernel: [ 685.246123] RSP: 0018:88045fc89d30 EFLAGS: 00010246 Feb 23 13:56:19 ayrshire.u06.univ-nantes.prive kernel: [ 685.246123] RAX: RBX: 001a RCX: 0001 Feb 23 13:56:19 ayrshire.u06.univ-nantes.prive kernel: [ 685.246123] RDX: RSI: RDI: Feb 23 13:56:19 ayrshire.u06.univ-nantes.prive kernel: [ 685.246123] RBP: R08: 0001 R09: 880856a91e48 Feb 23 13:56:19 ayrshire.u06.univ-nantes.prive kernel: [ 685.246123] R10: R11: R12: Feb 23 13:56:19 ayrshire.u06.univ-nantes.prive kernel: [ 685.246123] R13: 0001 R14: R15: Feb 23 13:56:19 ayrshire.u06.univ-nantes.prive kernel: [ 685.246123] FS: 7f617986c710() GS:88007f80() knlGS: Feb 23 13:56:19 ayrshire.u06.univ-nantes.prive kernel: [ 685.246123] CS: 0010 DS: ES: CR0: 8005003b Feb 23 13:56:19 ayrshire.u06.univ-nantes.prive kernel: [ 685.246123] CR2: 2458 CR3: 00045d197000 CR4: 06f0 Feb 23 13:56:19 ayrshire.u06.univ-nantes.prive kernel: [ 685.246123] DR0: DR1: DR2: Feb 23 13:56:19 ayrshire.u06.univ-nantes.prive kernel: [ 685.246123] DR3: DR6: 0ff0 DR7: 0400 Feb 23 13:56:19 ayrshire.u06.univ-nantes.prive kernel: [ 685.246123] Process kworker/0:1 (pid: 10,
Re: [PATCH 1/6] Staging: hv: Unify hyper-v device abstractions
Hi! Just an FYI, When sending out patch series, could you send out a [PATCH 0/6] first, and have all other patches a reply to that patch. Both git and quilt have ways to do this. The reason is that the linux kernel mailing list gets over 600 emails a day, and for those of us that skim through, we like to hit [make thread as read] when we are not interested in the thread or patch set. When people send out a set of patches that are all individual, this means I need to hit this command for every patch. Also do not think I'm just picking on you. I've complained to several people about doing this. One just recently sent out 21 patches. Thanks, -- Steve ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
Re: Bug inkvm_set_irq
Hi, here is another trace with kvm.ko compiled with debug flags. the bug: [12099.503414] BUG: unable to handle kernel paging request at 0b6635e9 [12099.503462] IP: [a03ee877] kvm_set_irq+0x37/0x140 [kvm] [12099.503521] PGD 45d8d2067 PUD 45d58e067 PMD 0 [12099.503560] Oops: [#1] SMP [12099.503591] last sysfs file: /sys/devices/system/cpu/cpu11/cache/index2/shared_cpu_map [12099.503641] CPU 0 [12099.503648] Modules linked in: netconsole configfs vhost_net macvtap macvlan tun veth powernow_k8 mperf cpufreq_userspace cpufreq_stats cpufreq_powersave cpufreq_ondemand freq_table cpufreq_conservative fuse xt_physdev ip6t_LOG ip6table_filter ip6_tables ipt_LOG xt_multiport xt_limit xt_tcpudp xt_state iptable_filter ip_tables x_tables nf_conntrack_tftp nf_conntrack_ftp nf_conntrack_ipv4 nf_defrag_ipv4 8021q bridge stp ext2 mbcache dm_round_robin dm_multipath nf_conntrack_ipv6 nf_conntrack nf_defrag_ipv6 kvm_amd kvm ipv6 snd_pcm snd_timer snd soundcore snd_page_alloc shpchp pci_hotplug tpm_tis i2c_nforce2 tpm i2c_core pcspkr evdev psmouse joydev tpm_bios processor ghes dcdbas hed button serio_raw thermal_sys xfs exportfs dm_mod sg sr_mod cdrom usbhid hid usb_storage ses sd_mod enclosure megaraid_sas ohci_hcd lpfc scsi_transport_fc bnx2 scsi_tgt scsi_mod ehci_hcd [last unloaded: scsi_wait_scan] [12099.504277] [12099.504302] Pid: 1742, comm: kworker/0:2 Not tainted 2.6.37.2-dsiun-110105+ #2 Dell Inc. PowerEdge M605/0K543T [12099.504373] RIP: 0010:[a03ee877] [a03ee877] kvm_set_irq+0x37/0x140 [kvm] [12099.50] RSP: 0018:88045e013d00 EFLAGS: 00010246 [12099.504474] RAX: 0b6634c1 RBX: 0018 RCX: 0001 [12099.504508] RDX: RSI: RDI: 880419b600c0 [12099.504541] RBP: 88045e013dd0 R08: 88045e012000 R09: [12099.504575] R10: R11: R12: 880419b600c0 [12099.504609] R13: 880419b600c0 R14: a03efaa0 R15: 0001 [12099.504643] FS: 7f3abaa05710() GS:88007f80() knlGS: [12099.504693] CS: 0010 DS: ES: CR0: 8005003b [12099.504724] CR2: 0b6635e9 CR3: 00045e2bc000 CR4: 06f0 [12099.504757] DR0: DR1: DR2: [12099.504791] DR3: DR6: 0ff0 DR7: 0400 [12099.504825] Process kworker/0:2 (pid: 1742, threadinfo ffkvm_set_irqff88045e012000, task 88045ffb0d60) [12099.504874] Stack: [12099.504897] 000119c0 000119c0 000119c0 88045ffb0d60 [12099.504953] 88045ffb1010 88045e013fd8 88045ffb1018 88045e012010 [12099.505009] 000119c0 88045e013fd8 000119c0 000119c0 [12099.505065] Call Trace: [12099.505099] [813818ce] ? common_interrupt+0xe/0x13 [12099.505145] [a03efaa0] ? irqfd_inject+0x0/0x50 [kvm] [12099.505145] [a03efaca] irqfd_inject+0x2a/0x50 [kvm] [12099.505145] [8106b7bb] process_one_work+0x11b/0x450 [12099.505145] [8106bf37] worker_thread+0x157/0x410 [12099.505145] [8103a569] ? __wake_up_common+0x59/0x90 [12099.505145] [8106bde0] ? worker_thread+0x0/0x410 [12099.505145] [8106f996] kthread+0x96/0xa0 [12099.505145] [81003c64] kernel_thread_helper+0x4/0x10 [12099.505145] [8106f900] ? kthread+0x0/0xa0 [12099.505145] [81003c60] ? kernel_thread_helper+0x0/0x10 [12099.505145] Code: 55 49 89 fd 41 54 53 89 d3 48 81 ec a8 00 00 00 8b 15 a6 75 03 00 89 b5 3c ff ff ff 85 d2 0f 85 d5 00 00 00 49 8b 85 58 24 00 00 3b 98 28 01 00 00 73 61 89 db 48 8b 84 d8 30 01 00 00 48 85 c0 [12099.505145] RIP [a03ee877] kvm_set_irq+0x37/0x140 [kvm] [12099.505145] RSP 88045e013d00 [12099.505145] CR2: 0b6635e9 markup_oops result: root@ayrshire:~# cat bug.txt | perl markup_oops.pl -m /lib/modules/2.6.37.2-dsiun-110105+/kernel/arch/x86/kvm/kvm.ko /boot/vmlinuz-2.6.37.2-dsiun-110105+ vmaoffset = 18446744072103034880 a03ee841: 48 89 e5 mov%rsp,%rbp a03ee844: 41 57 push %r15 a03ee846: 41 89 cf mov%ecx,%r15d | %r15 = 1 %ecx = 1 a03ee849: 41 56 push %r14| %r14 = a03efaa0 a03ee84b: 41 55 push %r13 a03ee84d: 49 89 fd mov%rdi,%r13 | %edi = 880419b600c0 %r13 = 880419b600c0 a03ee850: 41 54 push %r12| %r12 = 880419b600c0 a03ee852: 53 push %rbx a03ee853: 89 d3 mov%edx,%ebx | %ebx = 18 a03ee855: 48 81 ec a8 00 00 00sub$0xa8,%rsp a03ee85c: 8b 15 00 00 00 00 mov0x0(%rip),%edx# a03ee862 kvm_set_irq+0x22 a03ee862: 89 b5 3c
Re: [PATCH 2/6] Staging: hv: Rename vm_device to hyperv_device
On Fri, Feb 25, 2011 at 06:06:03PM -0800, K. Y. Srinivasan wrote:
Rename the vm_device abstraction as hyperv_device.
That's a nice name, but it's the first one with a hyperv_ prefix in
this subsystem. Ok, second, but hyperv_service_context doesn't really
count as it's not really used.
Everything else is named hv_ here. Which is it going to be, hv_ or
hyperv_?
Either is fine, just be aware of the work involved if you pick
hyperv_...
You also rename vm_device_info to hyperv_device_info here, doing more
than one thing in a single patch.
Please don't do that, this should be 2 patches.
Signed-off-by: K. Y. Srinivasan k...@microsoft.com
Signed-off-by: Haiyang Zhang haiya...@microsoft.com
Signed-off-by: Hank Janssen hjans...@microsoft.com
---
drivers/staging/hv/blkvsc.c|4 +-
drivers/staging/hv/blkvsc_drv.c|8 ++--
drivers/staging/hv/channel_mgmt.h |2 +-
drivers/staging/hv/netvsc.c| 56 +++
drivers/staging/hv/netvsc.h|2 +-
drivers/staging/hv/netvsc_api.h| 12
drivers/staging/hv/netvsc_drv.c| 14
drivers/staging/hv/rndis_filter.c | 18 ++--
drivers/staging/hv/storvsc.c | 36 ---
drivers/staging/hv/storvsc_api.h |4 +-
drivers/staging/hv/storvsc_drv.c | 10 +++---
drivers/staging/hv/vmbus.h |6 ++--
drivers/staging/hv/vmbus_api.h |8 ++--
drivers/staging/hv/vmbus_drv.c | 50
drivers/staging/hv/vmbus_private.h | 12
15 files changed, 124 insertions(+), 118 deletions(-)
diff --git a/drivers/staging/hv/blkvsc.c b/drivers/staging/hv/blkvsc.c
index ecface3..47ccec2 100644
--- a/drivers/staging/hv/blkvsc.c
+++ b/drivers/staging/hv/blkvsc.c
@@ -35,8 +35,8 @@ static const struct hv_guid g_blk_device_type = {
}
};
-static int blk_vsc_on_device_add(struct vm_device *device,
- void *additional_info)
+static int
+blk_vsc_on_device_add(struct hyperv_device *device, void *additional_info)
Ick, why break the formatting like this? Please keep the return type on
the same line as the function name.
You do that in a number of places, please don't.
thanks,
greg k-h
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
Re: [PATCH 3/6] Staging: hv: Cleanup hyperv_device variable names
On Fri, Feb 25, 2011 at 06:06:32PM -0800, K. Y. Srinivasan wrote:
Cleanup the names of variables that refer to the
hyperv_device abstraction.
Clean them up to be what? Shorter? Nice? Full of rounded edges so
that when we bump into them in the dark they don't poke us and cause us
to shreak in pain?
Signed-off-by: K. Y. Srinivasan k...@microsoft.com
Signed-off-by: K. Y. Srinivasan k...@microsoft.com
Sweet, you cloned yourself, I thought only Alan Cox had achieved that
goal...
Signed-off-by: Haiyang Zhang haiya...@microsoft.com
Signed-off-by: Hank Janssen hjans...@microsoft.com
---
drivers/staging/hv/blkvsc_drv.c | 12 ++--
drivers/staging/hv/netvsc.c |4 +-
drivers/staging/hv/netvsc_drv.c | 36
drivers/staging/hv/storvsc_drv.c | 44 +-
drivers/staging/hv/vmbus_drv.c | 164
+++---
5 files changed, 130 insertions(+), 130 deletions(-)
diff --git a/drivers/staging/hv/blkvsc_drv.c b/drivers/staging/hv/blkvsc_drv.c
index 58ab0e8..305a665 100644
--- a/drivers/staging/hv/blkvsc_drv.c
+++ b/drivers/staging/hv/blkvsc_drv.c
@@ -95,7 +95,7 @@ struct blkvsc_request {
/* Per device structure */
struct block_device_context {
/* point back to our device context */
- struct hyperv_device *device_ctx;
+ struct hyperv_device *device_obj;
Hey, I was right, it does have more rounded edges, nicely done.
-static int netvsc_device_add(struct hyperv_device *device,
- void *additional_info);
+static int
+netvsc_device_add(struct hyperv_device *device, void *additional_info);
Again with the function return value hiding. Please don't.
--- a/drivers/staging/hv/storvsc_drv.c
+++ b/drivers/staging/hv/storvsc_drv.c
@@ -43,7 +43,7 @@ struct host_device_context {
/* must be 1st field
* FIXME this is a bug */
/* point back to our device context */
- struct hyperv_device *device_ctx;
+ struct hyperv_device *device_obj;
I really don't understand this change at all. obj is just as vapid
and clueless as ctx is, and it seems very gratuitous to change this.
And I should know, I have made a lot of gratuitous renames in my time in
the kernel...
static int vmbus_uevent(struct device *device, struct kobj_uevent_env *env)
{
- struct hyperv_device *device_ctx = device_to_hyperv_device(device);
+ struct hyperv_device *device_obj = device_to_hyperv_device(device);
int ret;
DPRINT_INFO(VMBUS_DRV, generating uevent - VMBUS_DEVICE_CLASS_GUID={
%02x%02x%02x%02x-%02x%02x-%02x%02x-
%02x%02x%02x%02x%02x%02x%02x%02x},
- device_ctx-class_id.data[3], device_ctx-class_id.data[2],
- device_ctx-class_id.data[1], device_ctx-class_id.data[0],
- device_ctx-class_id.data[5], device_ctx-class_id.data[4],
- device_ctx-class_id.data[7], device_ctx-class_id.data[6],
- device_ctx-class_id.data[8], device_ctx-class_id.data[9],
- device_ctx-class_id.data[10],
- device_ctx-class_id.data[11],
- device_ctx-class_id.data[12],
- device_ctx-class_id.data[13],
- device_ctx-class_id.data[14],
- device_ctx-class_id.data[15]);
+ device_obj-class_id.data[3], device_obj-class_id.data[2],
+ device_obj-class_id.data[1], device_obj-class_id.data[0],
+ device_obj-class_id.data[5], device_obj-class_id.data[4],
+ device_obj-class_id.data[7], device_obj-class_id.data[6],
+ device_obj-class_id.data[8], device_obj-class_id.data[9],
+ device_obj-class_id.data[10],
+ device_obj-class_id.data[11],
+ device_obj-class_id.data[12],
+ device_obj-class_id.data[13],
+ device_obj-class_id.data[14],
+ device_obj-class_id.data[15]);
After seeing this stuff so many times I'm waiting for a helper function
to be added for it in this subsystem. I'm sure you really don't want to
edit GUID printk-like functions ever again, right?
static void vmbus_device_release(struct device *device)
{
- struct hyperv_device *device_ctx = device_to_hyperv_device(device);
+ struct hyperv_device *device_obj = device_to_hyperv_device(device);
- kfree(device_ctx);
+ kfree(device_obj);
- /* !!DO NOT REFERENCE device_ctx anymore at this point!! */
+ /* !!DO NOT REFERENCE device_obj anymore at this point!! */
}
I think by virtue of the kfree() right above this comment, it should be
deleted. Especially as it is at the end of the function, making it
pretty much impossible to make any sense here...
Come on, global search-and-replace needs to be done in a sane manner,
other wise you can just send me a vi macro to run on the code, it would
be the same thing in the end
Re: [PATCH 5/6] Staging: hv: Rename driver_context to hyperv_driver
On Fri, Feb 25, 2011 at 06:07:38PM -0800, K. Y. Srinivasan wrote:
The title says it all.
No it doesn't. You should have done this on the last patch to keep me
from complaining about that name.
Oh, and the extra space, drop it please.
--- a/drivers/staging/hv/blkvsc_drv.c
+++ b/drivers/staging/hv/blkvsc_drv.c
@@ -116,10 +116,10 @@ struct block_device_context {
};
/* Per driver */
-struct blkvsc_driver_context {
+struct blkvsc_hyperv_driver {
/* !! These must be the first 2 fields !! */
/* FIXME this is a bug! */
- struct driver_context drv_ctx;
+ struct hyperv_driver drv_ctx;
struct storvsc_driver_object drv_obj;
};
Hey look, that Subject: and changelog body actually lied. It didn't say
it all. In fact, it didn't say enough, or you just renamed a different
structure because it felt nice.
{sigh}
One thing per patch, with full description. I'm getting tired of
repeating Documentation/CodingStyle for this subsystem when patches are
submitted.
It's as if no one even listens to me...
{sniff}
greg k-h
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
Re: [PATCH 1/6] Staging: hv: Unify hyper-v device abstractions
On Fri, Feb 25, 2011 at 06:05:30PM -0800, K. Y. Srinivasan wrote:
Hyper-V drivers have supported two device abstractions.
This patch implements a single device abstraction.
This patch or This patch series?
This simplifies the code and avoids duplication
of state.
This patch is confusing, you are renaming structures (from hv_ to vm_)
which I didn't think you wanted to do.
Signed-off-by: K. Y. Srinivasan k...@microsoft.com
Signed-off-by: Haiyang Zhang haiya...@microsoft.com
Signed-off-by: Hank Janssen hjans...@microsoft.com
---
drivers/staging/hv/blkvsc.c| 17 ---
drivers/staging/hv/blkvsc_drv.c| 14 +++---
drivers/staging/hv/channel_mgmt.c |1 +
drivers/staging/hv/channel_mgmt.h |2 +-
drivers/staging/hv/netvsc.c| 55 ---
drivers/staging/hv/netvsc.h|2 +-
drivers/staging/hv/netvsc_api.h| 12 +++---
drivers/staging/hv/netvsc_drv.c| 28 +---
drivers/staging/hv/rndis_filter.c | 19
drivers/staging/hv/storvsc.c | 37
drivers/staging/hv/storvsc_api.h |4 +-
drivers/staging/hv/storvsc_drv.c | 21 -
drivers/staging/hv/vmbus.h | 13 +++---
drivers/staging/hv/vmbus_api.h | 29 ++--
drivers/staging/hv/vmbus_drv.c | 84
+++-
drivers/staging/hv/vmbus_private.h | 12 +++---
16 files changed, 157 insertions(+), 193 deletions(-)
diff --git a/drivers/staging/hv/blkvsc.c b/drivers/staging/hv/blkvsc.c
index 7c8729b..ecface3 100644
--- a/drivers/staging/hv/blkvsc.c
+++ b/drivers/staging/hv/blkvsc.c
@@ -35,7 +35,8 @@ static const struct hv_guid g_blk_device_type = {
}
};
-static int blk_vsc_on_device_add(struct hv_device *device, void
*additional_info)
+static int blk_vsc_on_device_add(struct vm_device *device,
+ void *additional_info)
Huh? What was this change for? 80 column issues for function
definitions is not a big deal, if any, and should not be burried in a
patch that claims to do something else.
Still totally confused,
greg k-h
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
Re: [PATCH 4/6] Staging: hv: Unify the hyperv driver abstractions
On Fri, Feb 25, 2011 at 06:07:03PM -0800, K. Y. Srinivasan wrote: This patch combines the two driver abstractions into a single driver abstraction. Ah, how sweet. Unfortunatly you don't say how you did this. Nor do you describe _what_ those two driver abstractions were. Are we talking i2c and usb abstractions? gpio and spi? Driver core and platform? We want to know exactly what is going on here. Think of writing something that when you look back, in 3 years, while staring at a Linux hyperv driver originally written for the 2.6.9 kernel, that somehow never got forward ported and you are tasked with doing this, that you can just do a simple 'git log drivers/staging/hv/' and instantly know just from the changelog comments exactly what you need to do to your driver to clean it up and properly get it to work on the new 8.2.2 kernel release. This changelog entry, would require you to go and dig through the guts of the patch itself, trying to figure out what abstractions you are talking about, and exactly how they were combined, all the while wondering _why_ they were combined. Please, think of your future self, you will thank him in the years to come by doing this properly. Not to mention making other's lives easier if you happen to have escaped this dire task by then. Oh, you have an extra space up there in the subject, please fix it next time. -int blk_vsc_initialize(struct hv_driver *driver) +int blk_vsc_initialize(struct driver_context *driver) struct driver_context? Oh please no. I realize that you are hopefully going to later rename this to something else, but remember, a few patches back you thought that the ctx name wasn't nice. And here you go resuscitating it from the graveyard of pointy bits. And what happens if your future patch is rejected? You are stuck with a driver_context structure in a subsystem? That's a pretty big abuse of the global namespace, don't you think? It sounds like something that should go into include/linux/device.h Please be careful about names, they mean things, even when you think they don't. thanks, greg k-h ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
Re: [PATCH 6/6] Staging: hv: Cleanup hyperv_driver variable names
On Fri, Feb 25, 2011 at 06:07:58PM -0800, K. Y. Srinivasan wrote: The title says it all. That's a horrible changelog comment. So bad that I would rather see an empty message than this one. Seriously, it give no description, and makes us think that the whole patch is obvious, when it really isn't. What did you change them to? What did you change them from? What was your motivation in changing them? How were you feeling when the names changed? Ok, maybe not the last one, but you get the idea. thanks, greg k-h ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
Re: Bug inkvm_set_irq
On Mon, Feb 28, 2011 at 11:34:16PM +0100, Jean-Philippe Menil wrote: Hi, here is another trace with kvm.ko compiled with debug flags. the bug: [12099.503414] BUG: unable to handle kernel paging request at 0b6635e9 [12099.503462] IP: [a03ee877] kvm_set_irq+0x37/0x140 [kvm] [12099.503521] PGD 45d8d2067 PUD 45d58e067 PMD 0 [12099.503560] Oops: [#1] SMP [12099.503591] last sysfs file: /sys/devices/system/cpu/cpu11/cache/index2/shared_cpu_map [12099.503641] CPU 0 [12099.503648] Modules linked in: netconsole configfs vhost_net macvtap macvlan tun veth powernow_k8 mperf cpufreq_userspace cpufreq_stats cpufreq_powersave cpufreq_ondemand freq_table cpufreq_conservative fuse xt_physdev ip6t_LOG ip6table_filter ip6_tables ipt_LOG xt_multiport xt_limit xt_tcpudp xt_state iptable_filter ip_tables x_tables nf_conntrack_tftp nf_conntrack_ftp nf_conntrack_ipv4 nf_defrag_ipv4 8021q bridge stp ext2 mbcache dm_round_robin dm_multipath nf_conntrack_ipv6 nf_conntrack nf_defrag_ipv6 kvm_amd kvm ipv6 snd_pcm snd_timer snd soundcore snd_page_alloc shpchp pci_hotplug tpm_tis i2c_nforce2 tpm i2c_core pcspkr evdev psmouse joydev tpm_bios processor ghes dcdbas hed button serio_raw thermal_sys xfs exportfs dm_mod sg sr_mod cdrom usbhid hid usb_storage ses sd_mod enclosure megaraid_sas ohci_hcd lpfc scsi_transport_fc bnx2 scsi_tgt scsi_mod ehci_hcd [last unloaded: scsi_wait_scan] [12099.504277] [12099.504302] Pid: 1742, comm: kworker/0:2 Not tainted 2.6.37.2-dsiun-110105+ #2 Dell Inc. PowerEdge M605/0K543T [12099.504373] RIP: 0010:[a03ee877] [a03ee877] kvm_set_irq+0x37/0x140 [kvm] [12099.50] RSP: 0018:88045e013d00 EFLAGS: 00010246 [12099.504474] RAX: 0b6634c1 RBX: 0018 RCX: 0001 [12099.504508] RDX: RSI: RDI: 880419b600c0 [12099.504541] RBP: 88045e013dd0 R08: 88045e012000 R09: [12099.504575] R10: R11: R12: 880419b600c0 [12099.504609] R13: 880419b600c0 R14: a03efaa0 R15: 0001 [12099.504643] FS: 7f3abaa05710() GS:88007f80() knlGS: [12099.504693] CS: 0010 DS: ES: CR0: 8005003b [12099.504724] CR2: 0b6635e9 CR3: 00045e2bc000 CR4: 06f0 [12099.504757] DR0: DR1: DR2: [12099.504791] DR3: DR6: 0ff0 DR7: 0400 [12099.504825] Process kworker/0:2 (pid: 1742, threadinfo ffkvm_set_irqff88045e012000, task 88045ffb0d60) [12099.504874] Stack: [12099.504897] 000119c0 000119c0 000119c0 88045ffb0d60 [12099.504953] 88045ffb1010 88045e013fd8 88045ffb1018 88045e012010 [12099.505009] 000119c0 88045e013fd8 000119c0 000119c0 [12099.505065] Call Trace: [12099.505099] [813818ce] ? common_interrupt+0xe/0x13 [12099.505145] [a03efaa0] ? irqfd_inject+0x0/0x50 [kvm] [12099.505145] [a03efaca] irqfd_inject+0x2a/0x50 [kvm] [12099.505145] [8106b7bb] process_one_work+0x11b/0x450 [12099.505145] [8106bf37] worker_thread+0x157/0x410 [12099.505145] [8103a569] ? __wake_up_common+0x59/0x90 [12099.505145] [8106bde0] ? worker_thread+0x0/0x410 [12099.505145] [8106f996] kthread+0x96/0xa0 [12099.505145] [81003c64] kernel_thread_helper+0x4/0x10 [12099.505145] [8106f900] ? kthread+0x0/0xa0 [12099.505145] [81003c60] ? kernel_thread_helper+0x0/0x10 [12099.505145] Code: 55 49 89 fd 41 54 53 89 d3 48 81 ec a8 00 00 00 8b 15 a6 75 03 00 89 b5 3c ff ff ff 85 d2 0f 85 d5 00 00 00 49 8b 85 58 24 00 00 3b 98 28 01 00 00 73 61 89 db 48 8b 84 d8 30 01 00 00 48 85 c0 [12099.505145] RIP [a03ee877] kvm_set_irq+0x37/0x140 [kvm] [12099.505145] RSP 88045e013d00 [12099.505145] CR2: 0b6635e9 markup_oops result: root@ayrshire:~# cat bug.txt | perl markup_oops.pl -m /lib/modules/2.6.37.2-dsiun-110105+/kernel/arch/x86/kvm/kvm.ko /boot/vmlinuz-2.6.37.2-dsiun-110105+ vmaoffset = 18446744072103034880 a03ee841:48 89 e5mov %rsp,%rbp a03ee844:41 57 push %r15 a03ee846:41 89 cfmov%ecx,%r15d | %r15 = 1 %ecx = 1 a03ee849:41 56 push %r14| %r14 = a03efaa0 a03ee84b:41 55 push %r13 a03ee84d:49 89 fdmov%rdi,%r13 | %edi = 880419b600c0 %r13 = 880419b600c0 a03ee850:41 54 push %r12| %r12 = 880419b600c0 a03ee852:53 push %rbx a03ee853:89 d3 mov%edx,%ebx | %ebx = 18 a03ee855:48 81 ec a8 00 00 00sub$0xa8,%rsp
