Patchwork [v2,5/8] Use writev ops if available

login
register
mail settings
Submitter Orit Wasserman
Date March 21, 2013, 1:23 p.m.
Message ID <1363872230-15081-6-git-send-email-owasserm@redhat.com>
Download mbox | patch
Permalink /patch/229673/
State New
Headers show

Comments

Orit Wasserman - March 21, 2013, 1:23 p.m.
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(-)
Paolo Bonzini - March 21, 2013, 1:44 p.m.
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);
>      }
>
Orit Wasserman - March 21, 2013, 1:52 p.m.
On 03/21/2013 03:44 PM, Paolo Bonzini wrote:
> 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.
You are right I will fix it.
> 
> 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.
Yes this will be helpful for the device state.

Orit
> 
> 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);
>>      }
>>
>

Patch

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);
+        }
+    } 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);
+        }
+    } 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);
     }