Patchwork [v6,6/6] virtio-console: Throttle virtio-serial-bus if we can't consume any more guest data

login
register
mail settings
Submitter Amit Shah
Date May 4, 2010, 6:23 p.m.
Message ID <1272997439-9675-7-git-send-email-amit.shah@redhat.com>
Download mbox | patch
Permalink /patch/51663/
State New
Headers show

Comments

Amit Shah - May 4, 2010, 6:23 p.m.
If the char device we're connected to is overwhelmed with data and it
can't accept any more, signal to the virtio-serial-bus to stop sending
us more data till we tell otherwise.

If the current buffer being processed hasn't been completely written out
to the char device, we have to keep it around and re-try sending it
since the virtio-serial-bus code assumes we consume the entire buffer.

Allow the chardev backends to return -EAGAIN; we're ready with a
callback handler that will flush the remainder of the buffer.

Also register with savevm so that we save/restore such a buffer across
migration.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 hw/virtio-console.c |  126 +++++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 123 insertions(+), 3 deletions(-)
Juan Quintela - May 4, 2010, 8:28 p.m.
Amit Shah <amit.shah@redhat.com> wrote:
> If the char device we're connected to is overwhelmed with data and it
> can't accept any more, signal to the virtio-serial-bus to stop sending
> us more data till we tell otherwise.
>
> If the current buffer being processed hasn't been completely written out
> to the char device, we have to keep it around and re-try sending it
> since the virtio-serial-bus code assumes we consume the entire buffer.
>
> Allow the chardev backends to return -EAGAIN; we're ready with a
> callback handler that will flush the remainder of the buffer.
>
> Also register with savevm so that we save/restore such a buffer across
> migration.
>
> Signed-off-by: Amit Shah <amit.shah@redhat.com>
> ---
>  hw/virtio-console.c |  126 +++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 files changed, 123 insertions(+), 3 deletions(-)
>
> diff --git a/hw/virtio-console.c b/hw/virtio-console.c
> index 749ed59..7eb6aa1 100644
> --- a/hw/virtio-console.c
> +++ b/hw/virtio-console.c
> @@ -13,18 +13,92 @@
>  #include "qemu-char.h"
>  #include "virtio-serial.h"
>  
> +typedef struct Buffer {
> +    uint8_t *buf;
> +    size_t rem_len;
> +    size_t offset;
> +} Buffer;
> +
>  typedef struct VirtConsole {
>      VirtIOSerialPort port;
>      CharDriverState *chr;
> +    Buffer *unflushed_buf;

This part of teh struct can be static instead of a pointer, and adjust
rest of code accordingly.  for the rest, it looks ok.

For this to work, I think that you will have to change rem_len for
total_len (or whatever) and adjust code around.  This makes trivial to
check for buffer used/no, just see if len = 0.

> +static int buffered_write_to_chardev(VirtConsole *vcon, const uint8_t *buf,
> +                                     size_t len)
> +{
> +    size_t written;
> +    ssize_t ret;
> +
> +    written = 0;
> +    do {
> +        ret = qemu_chr_write_nb(vcon->chr, buf + written, len - written);
> +        if (ret < 0) {
> +            if (vcon->unflushed_buf) {
> +                vcon->unflushed_buf->offset += written;
> +                vcon->unflushed_buf->rem_len -= written;
> +            } else {
> +                virtio_serial_throttle_port(&vcon->port, true);
> +                add_unflushed_buf(vcon, buf + written, len - written);
> +            }
> +
> +            return -EAGAIN;

You can "return ret" instead of "inventing" a new error :)

> +static void virtio_console_port_save(QEMUFile *f, void *opaque)
> +{
> +    VirtConsole *vcon = opaque;
> +    uint32_t have_buffer;
> +
> +    have_buffer = vcon->unflushed_buf ? true : false;

So, here you can just write vcon->unflushed_buf.len if you go with my
proposal, what will make VMState transition easier.


Rest of patch is good.  I also like the series, specially the change of
qemu_chr_add_handlers() to use one struct to pass its arguments.

Later, Juan.

Patch

diff --git a/hw/virtio-console.c b/hw/virtio-console.c
index 749ed59..7eb6aa1 100644
--- a/hw/virtio-console.c
+++ b/hw/virtio-console.c
@@ -13,18 +13,92 @@ 
 #include "qemu-char.h"
 #include "virtio-serial.h"
 
+typedef struct Buffer {
+    uint8_t *buf;
+    size_t rem_len;
+    size_t offset;
+} Buffer;
+
 typedef struct VirtConsole {
     VirtIOSerialPort port;
     CharDriverState *chr;
+    Buffer *unflushed_buf;
 } VirtConsole;
 
+static void add_unflushed_buf(VirtConsole *vcon, const uint8_t *buf, size_t len)
+{
+    vcon->unflushed_buf = qemu_malloc(sizeof(Buffer));
+    vcon->unflushed_buf->buf = qemu_malloc(len);
+
+    memcpy(vcon->unflushed_buf->buf, buf, len);
+    vcon->unflushed_buf->rem_len = len;
+    vcon->unflushed_buf->offset = 0;
+}
+
+static void free_unflushed_buf(VirtConsole *vcon)
+{
+    if (vcon->unflushed_buf) {
+        qemu_free(vcon->unflushed_buf->buf);
+        qemu_free(vcon->unflushed_buf);
+        vcon->unflushed_buf = NULL;
+    }
+}
+
+static int buffered_write_to_chardev(VirtConsole *vcon, const uint8_t *buf,
+                                     size_t len)
+{
+    size_t written;
+    ssize_t ret;
+
+    written = 0;
+    do {
+        ret = qemu_chr_write_nb(vcon->chr, buf + written, len - written);
+        if (ret < 0) {
+            if (vcon->unflushed_buf) {
+                vcon->unflushed_buf->offset += written;
+                vcon->unflushed_buf->rem_len -= written;
+            } else {
+                virtio_serial_throttle_port(&vcon->port, true);
+                add_unflushed_buf(vcon, buf + written, len - written);
+            }
+
+            return -EAGAIN;
+        }
+
+        written += ret;
+    } while (written != len);
+
+    return 0;
+}
+
+/* Callback function called when the chardev can accept more data */
+static void chr_write_unblocked(void *opaque)
+{
+    VirtConsole *vcon = opaque;
+
+    if (vcon->unflushed_buf) {
+        int ret;
+
+        ret = buffered_write_to_chardev(vcon, vcon->unflushed_buf->buf
+                                              + vcon->unflushed_buf->offset,
+                                        vcon->unflushed_buf->rem_len);
+        if (ret < 0) {
+            return;
+        }
+        free_unflushed_buf(vcon);
+    }
+    virtio_serial_throttle_port(&vcon->port, false);
+}
 
 /* Callback function that's called when the guest sends us data */
 static void flush_buf(VirtIOSerialPort *port, const uint8_t *buf, size_t len)
 {
     VirtConsole *vcon = DO_UPCAST(VirtConsole, port, port);
 
-    qemu_chr_write(vcon->chr, buf, len);
+    /* If a previous write was incomplete, we should've been throttled. */
+    assert(!vcon->unflushed_buf);
+
+    buffered_write_to_chardev(vcon, buf, len);
 }
 
 /* Readiness of the guest to accept data on a port */
@@ -48,19 +122,62 @@  static void chr_event(void *opaque, int event)
     VirtConsole *vcon = opaque;
 
     switch (event) {
-    case CHR_EVENT_OPENED: {
+    case CHR_EVENT_OPENED:
         virtio_serial_open(&vcon->port);
         break;
-    }
+
     case CHR_EVENT_CLOSED:
+        if (vcon->unflushed_buf) {
+            free_unflushed_buf(vcon);
+        }
         virtio_serial_close(&vcon->port);
         break;
     }
 }
 
+static void virtio_console_port_save(QEMUFile *f, void *opaque)
+{
+    VirtConsole *vcon = opaque;
+    uint32_t have_buffer;
+
+    have_buffer = vcon->unflushed_buf ? true : false;
+
+    qemu_put_be32s(f, &have_buffer);
+    if (have_buffer) {
+        qemu_put_be64s(f, &vcon->unflushed_buf->rem_len);
+        qemu_put_buffer(f, vcon->unflushed_buf->buf
+                           + vcon->unflushed_buf->offset,
+                        vcon->unflushed_buf->rem_len);
+    }
+}
+
+static int virtio_console_port_load(QEMUFile *f, void *opaque, int version_id)
+{
+    VirtConsole *vcon = opaque;
+    uint32_t have_buffer;
+
+    if (version_id > 1) {
+        return -EINVAL;
+    }
+
+    qemu_get_be32s(f, &have_buffer);
+    if (have_buffer) {
+        vcon->unflushed_buf = qemu_mallocz(sizeof(Buffer));
+
+        qemu_get_be64s(f, &vcon->unflushed_buf->rem_len);
+        vcon->unflushed_buf->buf = qemu_malloc(vcon->unflushed_buf->rem_len);
+        vcon->unflushed_buf->offset = 0;
+
+        qemu_get_buffer(f, vcon->unflushed_buf->buf,
+                        vcon->unflushed_buf->rem_len);
+    }
+    return 0;
+}
+
 static QemuChrHandlers chr_handlers = {
     .fd_can_read = chr_can_read,
     .fd_read = chr_read,
+    .fd_write_unblocked = chr_write_unblocked,
     .fd_event = chr_event,
 };
 
@@ -72,6 +189,8 @@  static int generic_port_init(VirtConsole *vcon, VirtIOSerialDevice *dev)
         qemu_chr_add_handlers(vcon->chr, &chr_handlers, vcon);
         vcon->port.info->have_data = flush_buf;
     }
+    register_savevm("virtio-console-ports", -1, 1, virtio_console_port_save,
+		    virtio_console_port_load, vcon);
     return 0;
 }
 
@@ -93,6 +212,7 @@  static int virtconsole_exitfn(VirtIOSerialDevice *dev)
     if (vcon->chr) {
         port->info->have_data = NULL;
         qemu_chr_close(vcon->chr);
+        free_unflushed_buf(vcon);
     }
 
     return 0;