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

[Juan Quintela <quintela@redhat.com>]

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.