| Message ID | 1304860165-28810-5-git-send-email-asias.hejun@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sun, 2011-05-08 at 21:09 +0800, Asias He wrote: > As virtio spec says: > > """ > Because this is high importance and low bandwidth, the current Linux > implementation polls for the buffer to be used, rather than waiting > for an interrupt, simplifying the implementation signicantly. > """ > > drivers/char/virtio_console.c > send_buf() { > ... > /* Tell Host to go! */ > virtqueue_kick(out_vq); > ... > while (!virtqueue_get_buf(out_vq, &len)) > cpu_relax(); > ... > } > > The console hangs can simply be reproduced by yes command which > gives tremendous console IOs and IRQs. Sasha, does this fix the hangs you were seeing? We should re-enable virtio console unconditionally if this does - that increases test coverage for virtio console. Pekka -- 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
On Sun, 2011-05-08 at 20:05 +0300, Pekka Enberg wrote: > On Sun, 2011-05-08 at 21:09 +0800, Asias He wrote: > > As virtio spec says: > > > > """ > > Because this is high importance and low bandwidth, the current Linux > > implementation polls for the buffer to be used, rather than waiting > > for an interrupt, simplifying the implementation signicantly. > > """ > > > > drivers/char/virtio_console.c > > send_buf() { > > ... > > /* Tell Host to go! */ > > virtqueue_kick(out_vq); > > ... > > while (!virtqueue_get_buf(out_vq, &len)) > > cpu_relax(); > > ... > > } > > > > The console hangs can simply be reproduced by yes command which > > gives tremendous console IOs and IRQs. > > Sasha, does this fix the hangs you were seeing? We should re-enable > virtio console unconditionally if this does - that increases test > coverage for virtio console. I'm seeing no more hangs, but why enable it unconditionally? Maybe enable it by default, but we shouldn't force the activation of virtio modules if the user doesn't want them.
On Sun, May 8, 2011 at 8:21 PM, Sasha Levin <levinsasha928@gmail.com> wrote: > I'm seeing no more hangs, but why enable it unconditionally? > Maybe enable it by default, but we shouldn't force the activation of > virtio modules if the user doesn't want them. I meant enabling the device on PCI bus like we did before. -- 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
On Sun, 2011-05-08 at 20:28 +0300, Pekka Enberg wrote: > On Sun, May 8, 2011 at 8:21 PM, Sasha Levin <levinsasha928@gmail.com> wrote: > > I'm seeing no more hangs, but why enable it unconditionally? > > Maybe enable it by default, but we shouldn't force the activation of > > virtio modules if the user doesn't want them. > > I meant enabling the device on PCI bus like we did before. Thats what I've meant too. virtio-console is the only device which got initialized even if it wasn't requested (even when '-c serial' was passed specifically).
On Sun, May 8, 2011 at 8:29 PM, Sasha Levin <levinsasha928@gmail.com> wrote: > On Sun, 2011-05-08 at 20:28 +0300, Pekka Enberg wrote: >> On Sun, May 8, 2011 at 8:21 PM, Sasha Levin <levinsasha928@gmail.com> wrote: >> > I'm seeing no more hangs, but why enable it unconditionally? >> > Maybe enable it by default, but we shouldn't force the activation of >> > virtio modules if the user doesn't want them. >> >> I meant enabling the device on PCI bus like we did before. > > Thats what I've meant too. virtio-console is the only device which got > initialized even if it wasn't requested (even when '-c serial' was > passed specifically). The more options we have, the more combinations we need to test. What's the downside of enabling virtio console by default? The upside is that it's less likely to break. Btw, we should probably do that for virtio rng as well. -- 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
On Sun, 2011-05-08 at 20:34 +0300, Pekka Enberg wrote: > On Sun, May 8, 2011 at 8:29 PM, Sasha Levin <levinsasha928@gmail.com> wrote: > > On Sun, 2011-05-08 at 20:28 +0300, Pekka Enberg wrote: > >> On Sun, May 8, 2011 at 8:21 PM, Sasha Levin <levinsasha928@gmail.com> wrote: > >> > I'm seeing no more hangs, but why enable it unconditionally? > >> > Maybe enable it by default, but we shouldn't force the activation of > >> > virtio modules if the user doesn't want them. > >> > >> I meant enabling the device on PCI bus like we did before. > > > > Thats what I've meant too. virtio-console is the only device which got > > initialized even if it wasn't requested (even when '-c serial' was > > passed specifically). > > The more options we have, the more combinations we need to test. > What's the downside of enabling virtio console by default? The upside > is that it's less likely to break. Btw, we should probably do that for > virtio rng as well. I fully support enabling it by default, I'm against enabling it even if the user asked to have it disabled.
On Sun, 2011-05-08 at 20:35 +0300, Sasha Levin wrote: > I fully support enabling it by default, I'm against enabling it even if > the user asked to have it disabled. Sure. -- 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/tools/kvm/virtio/console.c b/tools/kvm/virtio/console.c index f5449ba..f9031cb 100644 --- a/tools/kvm/virtio/console.c +++ b/tools/kvm/virtio/console.c @@ -166,13 +166,18 @@ static void virtio_console_handle_callback(struct kvm *self, void *param) vq = param; + /* + * The current Linux implementation polls for the buffer + * to be used, rather than waiting for an interrupt. + * So there is no need to inject an interrupt for the tx path. + */ + while (virt_queue__available(vq)) { head = virt_queue__get_iov(vq, iov, &out, &in, self); len = term_putc_iov(CONSOLE_VIRTIO, iov, out); virt_queue__set_used_elem(vq, head, len); } - virt_queue__trigger_irq(vq, virtio_console_pci_device.irq_line, &cdev.isr, self); } static bool virtio_console_pci_io_out(struct kvm *self, u16 port, void *data, int size, u32 count)
As virtio spec says: """ Because this is high importance and low bandwidth, the current Linux implementation polls for the buffer to be used, rather than waiting for an interrupt, simplifying the implementation signicantly. """ drivers/char/virtio_console.c send_buf() { ... /* Tell Host to go! */ virtqueue_kick(out_vq); ... while (!virtqueue_get_buf(out_vq, &len)) cpu_relax(); ... } The console hangs can simply be reproduced by yes command which gives tremendous console IOs and IRQs. [ 16.786440] irq 4: nobody cared (try booting with the "irqpoll" option) [ 16.786440] Pid: 1437, comm: yes Tainted: G W 2.6.39-rc6+ #56 [ 16.786440] Call Trace: [ 16.786440] [<c16578eb>] __report_bad_irq+0x30/0x89 [ 16.786440] [<c10980e6>] note_interrupt+0x118/0x17a [ 16.786440] [<c1096e7d>] handle_irq_event_percpu+0x168/0x179 [ 16.786440] [<c1096eba>] handle_irq_event+0x2c/0x46 [ 16.786440] [<c1098516>] ? unmask_irq+0x1e/0x1e [ 16.786440] [<c1098566>] handle_level_irq+0x50/0x6e [ 16.786440] <IRQ> [<c102fa69>] ? do_IRQ+0x35/0x7f [ 16.786440] [<c1665ea9>] ? common_interrupt+0x29/0x30 [ 16.786440] [<c16610d6>] ? _raw_spin_unlock_irqrestore+0x7/0x28 [ 16.786440] [<c1364f65>] ? hvc_write+0x88/0x9e [ 16.786440] [<c1355500>] ? do_output_char+0x88/0x18a [ 16.786440] [<c1355631>] ? process_output+0x2f/0x42 [ 16.786440] [<c1355af6>] ? n_tty_write+0x211/0x2dc [ 16.786440] [<c1059d77>] ? try_to_wake_up+0x226/0x226 [ 16.786440] [<c13534a4>] ? tty_write+0x15e/0x1d1 [ 16.786440] [<c12c1644>] ? security_file_permission+0x22/0x26 [ 16.786440] [<c13558e5>] ? process_echoes+0x241/0x241 [ 16.786440] [<c10dd9d2>] ? vfs_write+0x84/0xd7 [ 16.786440] [<c1353346>] ? tty_write_lock+0x3d/0x3d [ 16.786440] [<c10ddb92>] ? sys_write+0x3b/0x5d [ 16.786440] [<c166594c>] ? sysenter_do_call+0x12/0x22 [ 16.786440] handlers: [ 16.786440] [<c1351397>] (vp_interrupt+0x0/0x3a) [ 16.786440] Disabling IRQ #4 Signed-off-by: Asias He <asias.hejun@gmail.com> --- tools/kvm/virtio/console.c | 7 ++++++- 1 files changed, 6 insertions(+), 1 deletions(-)