Patchwork [RFC,04/20] Make QEMUFile buf expandable, and introduce qemu_realloc_buffer() and qemu_clear_buffer().

login
register
mail settings
Submitter Yoshiaki Tamura
Date April 21, 2010, 5:57 a.m.
Message ID <1271829445-5328-5-git-send-email-tamura.yoshiaki@lab.ntt.co.jp>
Download mbox | patch
Permalink /patch/50631/
State New
Headers show

Comments

Yoshiaki Tamura - April 21, 2010, 5:57 a.m.
Currently buf size is fixed at 32KB.  It would be useful if it could
be flexible.

Signed-off-by: Yoshiaki Tamura <tamura.yoshiaki@lab.ntt.co.jp>
---
 hw/hw.h  |    2 ++
 savevm.c |   26 +++++++++++++++++++++++++-
 2 files changed, 27 insertions(+), 1 deletions(-)
Stefan Hajnoczi - April 21, 2010, 8:03 a.m.
On Wed, Apr 21, 2010 at 6:57 AM, Yoshiaki Tamura
<tamura.yoshiaki@lab.ntt.co.jp> wrote:
> @@ -454,6 +458,25 @@ void qemu_fflush(QEMUFile *f)
>     }
>  }
>
> +void *qemu_realloc_buffer(QEMUFile *f, int size)
> +{
> +    f->buf_max_size = size;
> +
> +    f->buf = qemu_realloc(f->buf, f->buf_max_size);
> +    if (f->buf == NULL) {
> +        fprintf(stderr, "qemu file buffer realloc failed\n");
> +        exit(1);
> +    }
> +
> +    return f->buf;
> +}
> +

qemu_realloc() will abort() if there was not enough memory to realloc.
 Just like qemu_malloc(), you don't need to check for NULL.

Stefan
Yoshiaki Tamura - April 21, 2010, 8:27 a.m.
2010/4/21 Stefan Hajnoczi <stefanha@gmail.com>:
> On Wed, Apr 21, 2010 at 6:57 AM, Yoshiaki Tamura
> <tamura.yoshiaki@lab.ntt.co.jp> wrote:
>> @@ -454,6 +458,25 @@ void qemu_fflush(QEMUFile *f)
>>     }
>>  }
>>
>> +void *qemu_realloc_buffer(QEMUFile *f, int size)
>> +{
>> +    f->buf_max_size = size;
>> +
>> +    f->buf = qemu_realloc(f->buf, f->buf_max_size);
>> +    if (f->buf == NULL) {
>> +        fprintf(stderr, "qemu file buffer realloc failed\n");
>> +        exit(1);
>> +    }
>> +
>> +    return f->buf;
>> +}
>> +
>
> qemu_realloc() will abort() if there was not enough memory to realloc.
>  Just like qemu_malloc(), you don't need to check for NULL.

Thanks for your comment.  I'll remove it.
If there is no objection, I would like to take out this patch from the series,
and post it by itself.

Yoshi

>
> Stefan
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
Avi Kivity - April 23, 2010, 9:53 a.m.
On 04/21/2010 08:57 AM, Yoshiaki Tamura wrote:
> Currently buf size is fixed at 32KB.  It would be useful if it could
> be flexible.
>
>    

Why is this needed?  The real buffering is in the kernel anyways; this 
is only used to reduce the number of write() syscalls.
Yoshiaki Tamura - April 23, 2010, 9:59 a.m.
Avi Kivity wrote:
> On 04/21/2010 08:57 AM, Yoshiaki Tamura wrote:
>> Currently buf size is fixed at 32KB. It would be useful if it could
>> be flexible.
>>
>
> Why is this needed? The real buffering is in the kernel anyways; this is
> only used to reduce the number of write() syscalls.

This was introduced to buffer the transfered guests image transaction ally on 
the receiver side.  The sender doesn't use it.
In case of intermediate state, we just discard this buffer.
Avi Kivity - April 23, 2010, 1:14 p.m.
On 04/23/2010 12:59 PM, Yoshiaki Tamura wrote:
> Avi Kivity wrote:
>> On 04/21/2010 08:57 AM, Yoshiaki Tamura wrote:
>>> Currently buf size is fixed at 32KB. It would be useful if it could
>>> be flexible.
>>>
>>
>> Why is this needed? The real buffering is in the kernel anyways; this is
>> only used to reduce the number of write() syscalls.
>
> This was introduced to buffer the transfered guests image transaction 
> ally on the receiver side.  The sender doesn't use it.
> In case of intermediate state, we just discard this buffer.

How large can it grow?

What's wrong with applying it (perhaps partially) to the guest state?  
The next state transfer will overwrite it completely, no?
Anthony Liguori - April 23, 2010, 1:26 p.m.
On 04/23/2010 04:53 AM, Avi Kivity wrote:
> On 04/21/2010 08:57 AM, Yoshiaki Tamura wrote:
>> Currently buf size is fixed at 32KB.  It would be useful if it could
>> be flexible.
>>
>
> Why is this needed?  The real buffering is in the kernel anyways; this 
> is only used to reduce the number of write() syscalls.

With vmstate, we really shouldn't need to do this magic anymore as an 
optimization.

Regards,

Anthony Liguori
Yoshiaki Tamura - April 26, 2010, 10:43 a.m.
Avi Kivity wrote:
> On 04/23/2010 12:59 PM, Yoshiaki Tamura wrote:
>> Avi Kivity wrote:
>>> On 04/21/2010 08:57 AM, Yoshiaki Tamura wrote:
>>>> Currently buf size is fixed at 32KB. It would be useful if it could
>>>> be flexible.
>>>>
>>>
>>> Why is this needed? The real buffering is in the kernel anyways; this is
>>> only used to reduce the number of write() syscalls.
>>
>> This was introduced to buffer the transfered guests image transaction
>> ally on the receiver side. The sender doesn't use it.
>> In case of intermediate state, we just discard this buffer.
>
> How large can it grow?

It really depends on what workload is running on the guest, but it should be as 
large as the guest ram size in the worst case.

> What's wrong with applying it (perhaps partially) to the guest state?
> The next state transfer will overwrite it completely, no?

AFAIK, the answer is no.
qemu_loadvm_state() calls load handlers of each device emulator, and they will 
update its state directly, which means even if the transaction was not complete, 
it's impossible to recover the previous state if we don't make a buffer.

I guess your concern is about consuming large size of ram, and I think having an 
option for writing the transaction to a temporal disk image should be effective.

Patch

diff --git a/hw/hw.h b/hw/hw.h
index 05131a0..fc9ed29 100644
--- a/hw/hw.h
+++ b/hw/hw.h
@@ -61,6 +61,8 @@  void qemu_fflush(QEMUFile *f);
 int qemu_fclose(QEMUFile *f);
 void qemu_put_buffer(QEMUFile *f, const uint8_t *buf, int size);
 void qemu_put_byte(QEMUFile *f, int v);
+void *qemu_realloc_buffer(QEMUFile *f, int size);
+void qemu_clear_buffer(QEMUFile *f);
 
 static inline void qemu_put_ubyte(QEMUFile *f, unsigned int v)
 {
diff --git a/savevm.c b/savevm.c
index 2fd3de6..490ab70 100644
--- a/savevm.c
+++ b/savevm.c
@@ -174,7 +174,8 @@  struct QEMUFile {
                            when reading */
     int buf_index;
     int buf_size; /* 0 when writing */
-    uint8_t buf[IO_BUF_SIZE];
+    int buf_max_size;
+    uint8_t *buf;
 
     int has_error;
 };
@@ -424,6 +425,9 @@  QEMUFile *qemu_fopen_ops(void *opaque, QEMUFilePutBufferFunc *put_buffer,
     f->get_rate_limit = get_rate_limit;
     f->is_write = 0;
 
+    f->buf_max_size = IO_BUF_SIZE;
+    f->buf = qemu_mallocz(sizeof(uint8_t) * f->buf_max_size);
+
     return f;
 }
 
@@ -454,6 +458,25 @@  void qemu_fflush(QEMUFile *f)
     }
 }
 
+void *qemu_realloc_buffer(QEMUFile *f, int size)
+{
+    f->buf_max_size = size;
+
+    f->buf = qemu_realloc(f->buf, f->buf_max_size);
+    if (f->buf == NULL) {
+        fprintf(stderr, "qemu file buffer realloc failed\n");
+        exit(1);
+    }
+
+    return f->buf;
+}
+
+void qemu_clear_buffer(QEMUFile *f)
+{
+    f->buf_size = f->buf_index = f->buf_offset = 0;
+    memset(f->buf, 0, f->buf_max_size);
+}
+
 static void qemu_fill_buffer(QEMUFile *f)
 {
     int len;
@@ -479,6 +502,7 @@  int qemu_fclose(QEMUFile *f)
     qemu_fflush(f);
     if (f->close)
         ret = f->close(f->opaque);
+    qemu_free(f->buf);
     qemu_free(f);
     return ret;
 }