[v3,7/9] Add qemu_put_buffer_no_copy

Submitted by Orit Wasserman on March 21, 2013, 4:05 p.m.

Details

Message ID 1363881940-27505-8-git-send-email-owasserm@redhat.com
State New
Headers show

Commit Message

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(-)

Comments

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 hide | download patch | download mbox

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