[Qemu-devel] [PATCH 0/8] (v2) chardev, virtio-console: flow control, error handling, fixes

[Qemu-devel] [PATCH 0/8] (v2) chardev, virtio-console: flow control, error handling, fixes

[Amit Shah]

Hello,

This patchset introduces flow control to virtio-console and
chardev-based virtio serial ports. This series is based on the
previous series I sent on Mar 31st (00/17: v4: virtio-serial fixes,
new abi for port discovery)

The qemu chardevs can now return -EAGAIN when a non-blocking remote
isn't ready to accept more data.

Comments?

Changes from v1:
- Remove poll() usage
- Add fixes for virtio-serial throttling

Amit Shah (8):
  virtio-serial: throttling: check for throttled status before sending
    any data
  virtio-serial: Unthrottle ports once they're closed
  virtio-serial: Discard unconsumed data before sending port close
    event
  virtio-serial: Bus info message for showing port's throttled status
  char: Let writers know how much data was written in case of errors
  char: unix: For files that are nonblocking, report -EAGAIN to calling
    functions
  virtio-console: Factor out common init between console and generic
    ports
  virtio-console: Throttle virtio-serial-bus if we can't consume any
    more guest data

 hw/virtio-console.c    |  156 ++++++++++++++++++++++++++++++++++++++++++------
 hw/virtio-serial-bus.c |   23 +++++---
 qemu-char.c            |   21 ++++++-
 3 files changed, 172 insertions(+), 28 deletions(-)

[Qemu-devel] [PATCH 1/8] virtio-serial: throttling: check for throttled status before sending any data

[Amit Shah]

We were assuming that once unthrottled, ports could accept any amount of
data without getting throttled again.

Fix this assumption.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 hw/virtio-serial-bus.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
index 65ab253..5df9b6b 100644
--- a/hw/virtio-serial-bus.c
+++ b/hw/virtio-serial-bus.c
@@ -118,7 +118,7 @@ static void flush_queued_data(VirtIOSerialPort *port, bool discard)
     VirtQueueElement elem;
 
     vq = port->ovq;
-    while (virtqueue_pop(vq, &elem)) {
+    while ((discard || !port->throttled) && virtqueue_pop(vq, &elem)) {
         uint8_t *buf;
         size_t ret, buf_size;
 
-- 
1.6.2.5

[Qemu-devel] [PATCH 2/8] virtio-serial: Unthrottle ports once they're closed

[Amit Shah]

Disable throttling once a port is closed (and we discard all the
unconsumed buffers in the vq).

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 hw/virtio-serial-bus.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
index 5df9b6b..8d77c94 100644
--- a/hw/virtio-serial-bus.c
+++ b/hw/virtio-serial-bus.c
@@ -190,8 +190,9 @@ int virtio_serial_close(VirtIOSerialPort *port)
 
     /*
      * If there's any data the guest sent which the app didn't
-     * consume, discard it.
+     * consume, reset the throttling flag and discard the data.
      */
+    port->throttled = false;
     flush_queued_data(port, true);
     return 0;
 }
-- 
1.6.2.5

[Qemu-devel] [PATCH 3/8] virtio-serial: Discard unconsumed data before sending port close event

[Amit Shah]

The guest kernel can reclaim the buffers when it receives the port close
event or when a port is being removed. Ensure we free up the buffers
before we send out any events to the guest.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 hw/virtio-serial-bus.c |   13 +++++++++----
 1 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
index 8d77c94..c0044b3 100644
--- a/hw/virtio-serial-bus.c
+++ b/hw/virtio-serial-bus.c
@@ -186,14 +186,15 @@ int virtio_serial_open(VirtIOSerialPort *port)
 int virtio_serial_close(VirtIOSerialPort *port)
 {
     port->host_connected = false;
-    send_control_event(port, VIRTIO_CONSOLE_PORT_OPEN, 0);
-
     /*
      * If there's any data the guest sent which the app didn't
      * consume, reset the throttling flag and discard the data.
      */
     port->throttled = false;
     flush_queued_data(port, true);
+
+    send_control_event(port, VIRTIO_CONSOLE_PORT_OPEN, 0);
+
     return 0;
 }
 
@@ -631,13 +632,17 @@ static void add_port(VirtIOSerial *vser, uint32_t port_id)
 
 static void remove_port(VirtIOSerial *vser, uint32_t port_id)
 {
+    VirtIOSerialPort *port;
     unsigned int i;
 
     i = port_id / 32;
     vser->ports_map[i] &= ~(1U << (port_id % 32));
 
-    send_control_event(find_port_by_id(vser, port_id),
-                       VIRTIO_CONSOLE_PORT_REMOVE, 1);
+    port = find_port_by_id(vser, port_id);
+    /* Flush out any unconsumed buffers first */
+    flush_queued_data(port, true);
+
+    send_control_event(port, VIRTIO_CONSOLE_PORT_REMOVE, 1);
 }
 
 static int virtser_port_qdev_init(DeviceState *qdev, DeviceInfo *base)
-- 
1.6.2.5

[Qemu-devel] [PATCH 4/8] virtio-serial: Bus info message for showing port's throttled status

[Amit Shah]

Show whether a port is throttled in 'info qtree'.

Also reduce LOC by 1 by assigning 'throttled' status just once.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 hw/virtio-serial-bus.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
index c0044b3..9c3a913 100644
--- a/hw/virtio-serial-bus.c
+++ b/hw/virtio-serial-bus.c
@@ -240,12 +240,11 @@ void virtio_serial_throttle_port(VirtIOSerialPort *port, bool throttle)
         return;
     }
 
+    port->throttled = throttle;
     if (throttle) {
-        port->throttled = true;
         return;
     }
 
-    port->throttled = false;
     flush_queued_data(port, false);
 }
 
@@ -595,6 +594,8 @@ static void virtser_bus_dev_print(Monitor *mon, DeviceState *qdev, int indent)
                    indent, "", port->guest_connected);
     monitor_printf(mon, "%*s dev-prop-int: host_connected: %d\n",
                    indent, "", port->host_connected);
+    monitor_printf(mon, "%*s dev-prop-int: throttled: %d\n",
+                   indent, "", port->throttled);
 }
 
 /* This function is only used if a port id is not provided by the user */
-- 
1.6.2.5

[Qemu-devel] [PATCH 5/8] char: Let writers know how much data was written in case of errors

[Amit Shah]

On writing errors, we just returned -1 even if some bytes were already
written out. Ensure we return the number of bytes written before we
return the error (on a subsequent call to qemu_chr_write()).

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 qemu-char.c |    9 +++++++--
 1 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/qemu-char.c b/qemu-char.c
index 048da3f..208466f 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -502,8 +502,13 @@ static int unix_write(int fd, const uint8_t *buf, int len1)
     while (len > 0) {
         ret = write(fd, buf, len);
         if (ret < 0) {
-            if (errno != EINTR && errno != EAGAIN)
-                return -1;
+            if (errno != EINTR && errno != EAGAIN) {
+                if (len1 - len) {
+                    return len1 - len;
+                } else {
+                    return -1;
+                }
+            }
         } else if (ret == 0) {
             break;
         } else {
-- 
1.6.2.5

[Qemu-devel] [PATCH 6/8] char: unix: For files that are nonblocking, report -EAGAIN to calling functions

[Amit Shah]

If the chardev we're writing to is nonblocking, just report -EAGAIN to
the caller so that the caller can take any further action if it so
wishes.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 qemu-char.c |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/qemu-char.c b/qemu-char.c
index 208466f..57e9af3 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -497,11 +497,23 @@ int send_all(int fd, const void *buf, int len1)
 static int unix_write(int fd, const uint8_t *buf, int len1)
 {
     int ret, len;
+    int flags;
+    bool nonblock;
+
+    flags = fcntl(fd, F_GETFL);
+    if (flags == -1) {
+        flags = 0;
+    }
+
+    nonblock = flags & O_NONBLOCK;
 
     len = len1;
     while (len > 0) {
         ret = write(fd, buf, len);
         if (ret < 0) {
+            if (errno == EAGAIN && nonblock) {
+                return -EAGAIN;
+            }
             if (errno != EINTR && errno != EAGAIN) {
                 if (len1 - len) {
                     return len1 - len;
-- 
1.6.2.5

[Qemu-devel] [PATCH 7/8] virtio-console: Factor out common init between console and generic ports

[Amit Shah]

The initialisation for generic ports and console ports is similar.
Factor out the parts that are the same in a different function that can
be called from each of the initfns.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 hw/virtio-console.c |   31 ++++++++++++++-----------------
 1 files changed, 14 insertions(+), 17 deletions(-)

diff --git a/hw/virtio-console.c b/hw/virtio-console.c
index caea11f..d7fe68b 100644
--- a/hw/virtio-console.c
+++ b/hw/virtio-console.c
@@ -58,24 +58,28 @@ static void chr_event(void *opaque, int event)
     }
 }
 
-/* Virtio Console Ports */
-static int virtconsole_initfn(VirtIOSerialDevice *dev)
+static int generic_port_init(VirtConsole *vcon, VirtIOSerialDevice *dev)
 {
-    VirtIOSerialPort *port = DO_UPCAST(VirtIOSerialPort, dev, &dev->qdev);
-    VirtConsole *vcon = DO_UPCAST(VirtConsole, port, port);
-
-    port->info = dev->info;
-
-    port->is_console = true;
+    vcon->port.info = dev->info;
 
     if (vcon->chr) {
         qemu_chr_add_handlers(vcon->chr, chr_can_read, chr_read, chr_event,
                               vcon);
-        port->info->have_data = flush_buf;
+        vcon->port.info->have_data = flush_buf;
     }
     return 0;
 }
 
+/* Virtio Console Ports */
+static int virtconsole_initfn(VirtIOSerialDevice *dev)
+{
+    VirtIOSerialPort *port = DO_UPCAST(VirtIOSerialPort, dev, &dev->qdev);
+    VirtConsole *vcon = DO_UPCAST(VirtConsole, port, port);
+
+    port->is_console = true;
+    return generic_port_init(vcon, dev);
+}
+
 static int virtconsole_exitfn(VirtIOSerialDevice *dev)
 {
     VirtIOSerialPort *port = DO_UPCAST(VirtIOSerialPort, dev, &dev->qdev);
@@ -115,14 +119,7 @@ static int virtserialport_initfn(VirtIOSerialDevice *dev)
     VirtIOSerialPort *port = DO_UPCAST(VirtIOSerialPort, dev, &dev->qdev);
     VirtConsole *vcon = DO_UPCAST(VirtConsole, port, port);
 
-    port->info = dev->info;
-
-    if (vcon->chr) {
-        qemu_chr_add_handlers(vcon->chr, chr_can_read, chr_read, chr_event,
-                              vcon);
-        port->info->have_data = flush_buf;
-    }
-    return 0;
+    return generic_port_init(vcon, dev);
 }
 
 static VirtIOSerialPortInfo virtserialport_info = {
-- 
1.6.2.5

[Qemu-devel] [PATCH 8/8] virtio-console: Throttle virtio-serial-bus if we can't consume any more guest data

[Amit Shah]

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.

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 |  129 +++++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 126 insertions(+), 3 deletions(-)

diff --git a/hw/virtio-console.c b/hw/virtio-console.c
index d7fe68b..1418a97 100644
--- a/hw/virtio-console.c
+++ b/hw/virtio-console.c
@@ -13,18 +13,93 @@
 #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;
+    QEMUTimer *host_timer;
+    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(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);
+            }
+
+            qemu_mod_timer(vcon->host_timer,
+                           qemu_get_clock(host_clock) + (int64_t) 100000);
+            return -EAGAIN;
+        }
+
+        written += ret;
+    } while (written != len);
+
+    return 0;
+}
+
+/* Callback function when the timer expires */
+static void unthrottle_port(VirtConsole *vcon)
+{
+    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,16 +123,59 @@ 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) {
+            qemu_del_timer(vcon->host_timer);
+            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 int generic_port_init(VirtConsole *vcon, VirtIOSerialDevice *dev)
 {
     vcon->port.info = dev->info;
@@ -67,6 +185,9 @@ static int generic_port_init(VirtConsole *vcon, VirtIOSerialDevice *dev)
                               vcon);
         vcon->port.info->have_data = flush_buf;
     }
+    vcon->host_timer = qemu_new_timer(host_clock, (void *)unthrottle_port, vcon);
+    register_savevm("virtio-console-ports", -1, 1, virtio_console_port_save,
+		    virtio_console_port_load, vcon);
     return 0;
 }
 
@@ -88,7 +209,9 @@ static int virtconsole_exitfn(VirtIOSerialDevice *dev)
     if (vcon->chr) {
         port->info->have_data = NULL;
         qemu_chr_close(vcon->chr);
+        free_unflushed_buf(vcon);
     }
+    qemu_free_timer(vcon->host_timer);
 
     return 0;
 }
-- 
1.6.2.5

[Qemu-devel] Re: [PATCH 0/8] (v2) chardev, virtio-console: flow control, error handling, fixes

[Paul Brook]

> Hello,
> 
> This patchset introduces flow control to virtio-console and
> chardev-based virtio serial ports. This series is based on the
> previous series I sent on Mar 31st (00/17: v4: virtio-serial fixes,
> new abi for port discovery)
> 
> The qemu chardevs can now return -EAGAIN when a non-blocking remote
> isn't ready to accept more data.
> 
> Comments?

This is a major change in semantics. Are you sure all users handle this 
correctly? My guess is that most of the devices don't.
EAGAIN isn't really a useful response unless you have some way of notifying 
the device that it can send more data.

Paul

[Qemu-devel] Re: [PATCH 0/8] (v2) chardev, virtio-console: flow control, error handling, fixes

[Gerd Hoffmann]

On 04/08/10 00:58, Paul Brook wrote:
>> Hello,
>>
>> This patchset introduces flow control to virtio-console and
>> chardev-based virtio serial ports. This series is based on the
>> previous series I sent on Mar 31st (00/17: v4: virtio-serial fixes,
>> new abi for port discovery)
>>
>> The qemu chardevs can now return -EAGAIN when a non-blocking remote
>> isn't ready to accept more data.
>>
>> Comments?
>
> This is a major change in semantics. Are you sure all users handle this
> correctly? My guess is that most of the devices don't.

I don't expect trouble here.  EAGAIN is returned only for file handles 
in non-blocking mode.  I doubt existing users use non-blocking I/O as 
this makes the current unix_write() code go busy-loop in case the 
outgoing pipe is full.

> EAGAIN isn't really a useful response unless you have some way of notifying
> the device that it can send more data.

This is a valid point though.

cheers,
   Gerd

Re: [Qemu-devel] Re: [PATCH 0/8] (v2) chardev, virtio-console: flow control, error handling, fixes

[Paul Brook]

> >> The qemu chardevs can now return -EAGAIN when a non-blocking remote
> >> isn't ready to accept more data.
> >>
> >> Comments?
> >
> > This is a major change in semantics. Are you sure all users handle this
> > correctly? My guess is that most of the devices don't.
> 
> I don't expect trouble here.  EAGAIN is returned only for file handles
> in non-blocking mode.  I doubt existing users use non-blocking I/O as
> this makes the current unix_write() code go busy-loop in case the
> outgoing pipe is full.

I'm pretty sure existing devices emulation assumes qemu_chr_write either fully 
completes or fails with an error. Whether a FD is nonblocking is determined by 
the backend used to implement the chardev, not the emulated device using the 
chardev.

For the device emulation side of the API, unix write() semantics are IMO a bad 
solution.  What we really want is an API for posting asynchronous writes.

Paul