Patchwork [v3,7/9] Add qemu_put_buffer_no_copy

login
register
mail settings
Submitter Orit Wasserman
Date March 21, 2013, 4:05 p.m.
Message ID <1363881940-27505-8-git-send-email-owasserm@redhat.com>
Download mbox | patch
Permalink /patch/229757/
State New
Headers show

Comments

Orit Wasserman - March 21, 2013, 4:05 p.m.
This allow us to add a buffer to the iovec to send without copying it
into the static buffer.

Signed-off-by: Orit Wasserman <owasserm@redhat.com>
---
 include/migration/qemu-file.h |  5 +++++
 savevm.c                      | 37 ++++++++++++++++++++++++++++---------
 2 files changed, 33 insertions(+), 9 deletions(-)
Juan Quintela - March 21, 2013, 5:34 p.m.
Orit Wasserman <owasserm@redhat.com> wrote:
> This allow us to add a buffer to the iovec to send without copying it
> into the static buffer.
>
> Signed-off-by: Orit Wasserman <owasserm@redhat.com>
> ---
>  include/migration/qemu-file.h |  5 +++++
>  savevm.c                      | 37 ++++++++++++++++++++++++++++---------
>  2 files changed, 33 insertions(+), 9 deletions(-)
>
> diff --git a/include/migration/qemu-file.h b/include/migration/qemu-file.h
> index 8d3da9b..5168be2 100644
> --- a/include/migration/qemu-file.h
> +++ b/include/migration/qemu-file.h
> @@ -75,6 +75,11 @@ int qemu_fclose(QEMUFile *f);
>  int64_t qemu_ftell(QEMUFile *f);
>  void qemu_put_buffer(QEMUFile *f, const uint8_t *buf, int size);
>  void qemu_put_byte(QEMUFile *f, int v);
> +/*
> + * put_buffer without copying the buffer.
> + * The buffer should be available till it is sent.
> + */
> +void qemu_put_buffer_no_copy(QEMUFile *f, const uint8_t *buf, int size);
>  
>  static inline void qemu_put_ubyte(QEMUFile *f, unsigned int v)
>  {
> diff --git a/savevm.c b/savevm.c
> index 83aa9e7..fbfb9e3 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -621,6 +621,30 @@ int qemu_fclose(QEMUFile *f)
>      return ret;
>  }
>  
> +
> +void qemu_put_buffer_no_copy(QEMUFile *f, const uint8_t *buf, int size)
> +{
> +    if (f->last_error) {
> +        return;
> +    }
> +
> +    if (f->is_write == 0 && f->buf_index > 0) {
> +        fprintf(stderr,
> +                "Attempted to write to buffer while read buffer is not empty\n");
> +        abort();
> +    }

I don't understand this test at all (yes, I know that the test already
existed).

We shouldn't never arrived qemu_put_buffer() with a QEMUFile with
f->is_write == 0.

Change it for one assert()?

> +    f->iov[f->iovcnt].iov_base = f->buf + f->buf_index;
> +    f->iov[f->iovcnt++].iov_len = size;

This is clearly wrong, or I have completely missunderstood it (I will
give a 50% to each posiblitiy).

Here we should be using "buf" and "size" passed as paramenters, f->buf
and f->buf_index shouldn't be used, no?

> +
> +    f->is_write = 1;

is_write is completely redundant, and should just be a:

f->ops->put_buffer test (or now with writev).  We only set it up when
there is anything to be written?

But again, this is independent of this series.
Orit Wasserman - March 21, 2013, 5:39 p.m.
On 03/21/2013 07:34 PM, Juan Quintela wrote:
> Orit Wasserman <owasserm@redhat.com> wrote:
>> This allow us to add a buffer to the iovec to send without copying it
>> into the static buffer.
>>
>> Signed-off-by: Orit Wasserman <owasserm@redhat.com>
>> ---
>>  include/migration/qemu-file.h |  5 +++++
>>  savevm.c                      | 37 ++++++++++++++++++++++++++++---------
>>  2 files changed, 33 insertions(+), 9 deletions(-)
>>
>> diff --git a/include/migration/qemu-file.h b/include/migration/qemu-file.h
>> index 8d3da9b..5168be2 100644
>> --- a/include/migration/qemu-file.h
>> +++ b/include/migration/qemu-file.h
>> @@ -75,6 +75,11 @@ int qemu_fclose(QEMUFile *f);
>>  int64_t qemu_ftell(QEMUFile *f);
>>  void qemu_put_buffer(QEMUFile *f, const uint8_t *buf, int size);
>>  void qemu_put_byte(QEMUFile *f, int v);
>> +/*
>> + * put_buffer without copying the buffer.
>> + * The buffer should be available till it is sent.
>> + */
>> +void qemu_put_buffer_no_copy(QEMUFile *f, const uint8_t *buf, int size);
>>  
>>  static inline void qemu_put_ubyte(QEMUFile *f, unsigned int v)
>>  {
>> diff --git a/savevm.c b/savevm.c
>> index 83aa9e7..fbfb9e3 100644
>> --- a/savevm.c
>> +++ b/savevm.c
>> @@ -621,6 +621,30 @@ int qemu_fclose(QEMUFile *f)
>>      return ret;
>>  }
>>  
>> +
>> +void qemu_put_buffer_no_copy(QEMUFile *f, const uint8_t *buf, int size)
>> +{
>> +    if (f->last_error) {
>> +        return;
>> +    }
>> +
>> +    if (f->is_write == 0 && f->buf_index > 0) {
>> +        fprintf(stderr,
>> +                "Attempted to write to buffer while read buffer is not empty\n");
>> +        abort();
>> +    }
> 
> I don't understand this test at all (yes, I know that the test already
> existed).
> 
> We shouldn't never arrived qemu_put_buffer() with a QEMUFile with
> f->is_write == 0.
> 
> Change it for one assert()?
> 
>> +    f->iov[f->iovcnt].iov_base = f->buf + f->buf_index;
>> +    f->iov[f->iovcnt++].iov_len = size;
> 
> This is clearly wrong, or I have completely missunderstood it (I will
> give a 50% to each posiblitiy).
> 
> Here we should be using "buf" and "size" passed as paramenters, f->buf
> and f->buf_index shouldn't be used, no?
you are right, it somehow moved to the next patch :(
I will send a fixed v4 after you finish review the series ..
> 
>> +
>> +    f->is_write = 1;
> 
> is_write is completely redundant, and should just be a:
> 
> f->ops->put_buffer test (or now with writev).  We only set it up when
> there is anything to be written?
> 
> But again, this is independent of this series.
> 
I agree, maybe we need a cleanup series and remove it.
It will certainly simplify this code

Orit

Patch

diff --git a/include/migration/qemu-file.h b/include/migration/qemu-file.h
index 8d3da9b..5168be2 100644
--- a/include/migration/qemu-file.h
+++ b/include/migration/qemu-file.h
@@ -75,6 +75,11 @@  int qemu_fclose(QEMUFile *f);
 int64_t qemu_ftell(QEMUFile *f);
 void qemu_put_buffer(QEMUFile *f, const uint8_t *buf, int size);
 void qemu_put_byte(QEMUFile *f, int v);
+/*
+ * put_buffer without copying the buffer.
+ * The buffer should be available till it is sent.
+ */
+void qemu_put_buffer_no_copy(QEMUFile *f, const uint8_t *buf, int size);
 
 static inline void qemu_put_ubyte(QEMUFile *f, unsigned int v)
 {
diff --git a/savevm.c b/savevm.c
index 83aa9e7..fbfb9e3 100644
--- a/savevm.c
+++ b/savevm.c
@@ -621,6 +621,30 @@  int qemu_fclose(QEMUFile *f)
     return ret;
 }
 
+
+void qemu_put_buffer_no_copy(QEMUFile *f, const uint8_t *buf, int size)
+{
+    if (f->last_error) {
+        return;
+    }
+
+    if (f->is_write == 0 && f->buf_index > 0) {
+        fprintf(stderr,
+                "Attempted to write to buffer while read buffer is not empty\n");
+        abort();
+    }
+
+    f->iov[f->iovcnt].iov_base = f->buf + f->buf_index;
+    f->iov[f->iovcnt++].iov_len = size;
+
+    f->is_write = 1;
+    f->bytes_xfer += size;
+
+    if (f->buf_index >= IO_BUF_SIZE || f->iovcnt >= MAX_IOV_SIZE) {
+        qemu_fflush(f);
+    }
+}
+
 void qemu_put_buffer(QEMUFile *f, const uint8_t *buf, int size)
 {
     int l;
@@ -640,19 +664,14 @@  void qemu_put_buffer(QEMUFile *f, const uint8_t *buf, int size)
         if (l > size)
             l = size;
         memcpy(f->buf + f->buf_index, buf, l);
-        f->iov[f->iovcnt].iov_base = f->buf + f->buf_index;
-        f->iov[f->iovcnt++].iov_len = l;
         f->is_write = 1;
         f->buf_index += l;
-        f->bytes_xfer += l;
+        qemu_put_buffer_no_copy(f, f->buf + (f->buf_index - l), l);
+        if (qemu_file_get_error(f)) {
+            break;
+        }
         buf += l;
         size -= l;
-        if (f->buf_index >= IO_BUF_SIZE || f->iovcnt >= MAX_IOV_SIZE) {
-            qemu_fflush(f);
-            if (qemu_file_get_error(f)) {
-                break;
-            }
-        }
     }
 }