Re: [Qemu-devel] [PATCH v2 5/8] Use writev ops if available

[Paolo Bonzini <pbonzini@redhat.com>]

Il 21/03/2013 14:23, Orit Wasserman ha scritto:
> Update qemu_fflush and stdio_close to use writev ops if they are available
> 
> Signed-off-by: Orit Wasserman <owasserm@redhat.com>
> ---
>  savevm.c | 30 ++++++++++++++++++++----------
>  1 file changed, 20 insertions(+), 10 deletions(-)
> 
> diff --git a/savevm.c b/savevm.c
> index ab81dd3..fde59d3 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -292,7 +292,7 @@ static int stdio_fclose(void *opaque)
>      QEMUFileStdio *s = opaque;
>      int ret = 0;
>  
> -    if (s->file->ops->put_buffer) {
> +    if (s->file->ops->put_buffer || s->file->ops->writev_buffer) {
>          int fd = fileno(s->stdio_file);
>          struct stat st;
>  
> @@ -515,24 +515,34 @@ static void qemu_file_set_error(QEMUFile *f, int ret)
>      }
>  }
>  
> -/** Flushes QEMUFile buffer
> +/**
> + * Flushes QEMUFile buffer
>   *
> + * If there is writev_buffer QEMUFileOps it uses it otherwise uses
> + * put_buffer ops.
>   */
>  static void qemu_fflush(QEMUFile *f)
>  {
>      int ret = 0;
>  
> -    if (!f->ops->put_buffer) {
> +    if (f->ops->writev_buffer) {
> +        if (f->is_write && f->iovcnt > 0) {
> +            ret = f->ops->writev_buffer(f->opaque, f->iov, f->iovcnt);

This needs to update f->pos.

> +        }
> +    } else if (f->ops->put_buffer) {
> +        if (f->is_write && f->buf_index > 0) {
> +            ret = f->ops->put_buffer(f->opaque, f->buf, f->pos, f->buf_index);
> +        }

I think this has to go through all the iovecs (i.e. never look at
f->buf_index, only f->iovcnt).  Otherwise RAM migration does not work,
because the page data is not copied to f->buf.  But that's pretty much
it, the series looks good.

However, I'd still prefer to have qemu_put_buffer_no_copy coalesce
adjacent iovecs.  Otherwise you might end up with only 3-400 bytes in
each sendmsg when serializing device data.

Paolo

> +    } else {
>          return;
>      }
> -    if (f->is_write && f->buf_index > 0) {
> -        ret = f->ops->put_buffer(f->opaque, f->buf, f->pos, f->buf_index);
> -        if (ret >= 0) {
> -            f->pos += f->buf_index;
> -        }
> -        f->buf_index = 0;
> -        f->iovcnt = 0;
> +
> +    if (ret >= 0) {
> +        f->pos += f->buf_index;
>      }
> +    f->buf_index = 0;
> +    f->iovcnt = 0;
> +
>      if (ret < 0) {
>          qemu_file_set_error(f, ret);
>      }
>