From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([208.118.235.92]:52902)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <pbonzini@redhat.com>) id 1UIfno-0000Ke-Fs
	for qemu-devel@nongnu.org; Thu, 21 Mar 2013 09:45:10 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <pbonzini@redhat.com>) id 1UIfnm-0008BM-KV
	for qemu-devel@nongnu.org; Thu, 21 Mar 2013 09:45:08 -0400
Received: from mx1.redhat.com ([209.132.183.28]:8089)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <pbonzini@redhat.com>) id 1UIfnm-0008Ab-CU
	for qemu-devel@nongnu.org; Thu, 21 Mar 2013 09:45:06 -0400
Message-ID: <514B0EDB.10808@redhat.com>
Date: Thu, 21 Mar 2013 14:44:59 +0100
From: Paolo Bonzini <pbonzini@redhat.com>
MIME-Version: 1.0
References: <1363872230-15081-1-git-send-email-owasserm@redhat.com>
	<1363872230-15081-6-git-send-email-owasserm@redhat.com>
In-Reply-To: <1363872230-15081-6-git-send-email-owasserm@redhat.com>
Content-Type: text/plain; charset=ISO-8859-15
Content-Transfer-Encoding: 7bit
Subject: Re: [Qemu-devel] [PATCH v2 5/8] Use writev ops if available
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Orit Wasserman <owasserm@redhat.com>
Cc: mst@redhat.com, chegu_vinod@hp.com, qemu-devel@nongnu.org, quintela@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);
>      }
>