Patchwork [RFC,10/12] Add qemu_put_buffer_no_copy

login
register
mail settings
Submitter Orit Wasserman
Date March 21, 2013, 9:09 a.m.
Message ID <1363856971-4601-11-git-send-email-owasserm@redhat.com>
Download mbox | patch
Permalink /patch/229593/
State New
Headers show

Comments

Orit Wasserman - March 21, 2013, 9:09 a.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                      | 42 ++++++++++++++++++++++++------------------
 2 files changed, 29 insertions(+), 18 deletions(-)
Paolo Bonzini - March 21, 2013, 9:25 a.m.
Il 21/03/2013 10:09, Orit Wasserman ha scritto:
> +    f->iov[f->iovcnt].iov_base = (uint8_t *)buf;
> +    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);
>      }

It should not be complex to check if f->iov[f->iovcnt - 1] can be
extended?  This could remove many system calls when you have many
consecutive zero pages.

Paolo
mrhines@linux.vnet.ibm.com - March 23, 2013, 4:27 p.m.
Can you add a "flag" or something to indicate that the iov pointer 
belongs to RAM and not to device state?

That way, I could re-use this code for RDMA - if I see this flag, I will 
know to send to RDMA.....

- Michael


On 03/21/2013 05:09 AM, Orit Wasserman 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                      | 42 ++++++++++++++++++++++++------------------
>   2 files changed, 29 insertions(+), 18 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 40d96f4..32a506e 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -629,6 +629,22 @@ void qemu_put_buffer(QEMUFile *f, const uint8_t *buf, int size)
>   {
>       int l;
>
> +    while (size > 0) {
> +        l = IO_BUF_SIZE - f->buf_index;
> +        if (l > size) {
> +            l = size;
> +        }
> +        memcpy(f->buf + f->buf_index, buf, l);
> +        f->buf_index += l;
> +        f->is_write = 1;
> +        qemu_put_buffer_no_copy(f, f->buf + (f->buf_index - l), l);
> +        buf += l;
> +        size -= l;
> +    }
> +}
> +
> +void qemu_put_buffer_no_copy(QEMUFile *f, const uint8_t *buf, int size)
> +{
>       if (f->last_error) {
>           return;
>       }
> @@ -639,24 +655,14 @@ void qemu_put_buffer(QEMUFile *f, const uint8_t *buf, int size)
>           abort();
>       }
>
> -    while (size > 0) {
> -        l = IO_BUF_SIZE - f->buf_index;
> -        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;
> -        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;
> -            }
> -        }
> +    f->iov[f->iovcnt].iov_base = (uint8_t *)buf;
> +    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);
>       }
>   }
>
Orit Wasserman - March 25, 2013, 8:11 a.m.
On 03/23/2013 06:27 PM, Michael R. Hines wrote:
> Can you add a "flag" or something to indicate that the iov pointer belongs to RAM and not to device state?
> 
> That way, I could re-use this code for RDMA - if I see this flag, I will know to send to RDMA.....
This function is called only for ram pages so no need for flag.

Orit
> 
> - Michael
> 
> 
> On 03/21/2013 05:09 AM, Orit Wasserman 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                      | 42 ++++++++++++++++++++++++------------------
>>   2 files changed, 29 insertions(+), 18 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 40d96f4..32a506e 100644
>> --- a/savevm.c
>> +++ b/savevm.c
>> @@ -629,6 +629,22 @@ void qemu_put_buffer(QEMUFile *f, const uint8_t *buf, int size)
>>   {
>>       int l;
>>
>> +    while (size > 0) {
>> +        l = IO_BUF_SIZE - f->buf_index;
>> +        if (l > size) {
>> +            l = size;
>> +        }
>> +        memcpy(f->buf + f->buf_index, buf, l);
>> +        f->buf_index += l;
>> +        f->is_write = 1;
>> +        qemu_put_buffer_no_copy(f, f->buf + (f->buf_index - l), l);
>> +        buf += l;
>> +        size -= l;
>> +    }
>> +}
>> +
>> +void qemu_put_buffer_no_copy(QEMUFile *f, const uint8_t *buf, int size)
>> +{
>>       if (f->last_error) {
>>           return;
>>       }
>> @@ -639,24 +655,14 @@ void qemu_put_buffer(QEMUFile *f, const uint8_t *buf, int size)
>>           abort();
>>       }
>>
>> -    while (size > 0) {
>> -        l = IO_BUF_SIZE - f->buf_index;
>> -        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;
>> -        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;
>> -            }
>> -        }
>> +    f->iov[f->iovcnt].iov_base = (uint8_t *)buf;
>> +    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);
>>       }
>>   }
>>
>
Paolo Bonzini - March 25, 2013, 1:05 p.m.
----- Messaggio originale -----
> Da: "Michael R. Hines" <mrhines@linux.vnet.ibm.com>
> A: "Orit Wasserman" <owasserm@redhat.com>
> Cc: qemu-devel@nongnu.org, pbonzini@redhat.com, mst@redhat.com, "chegu vinod" <chegu_vinod@hp.com>,
> quintela@redhat.com
> Inviato: Sabato, 23 marzo 2013 17:27:49
> Oggetto: Re: [Qemu-devel] [RFC 10/12] Add qemu_put_buffer_no_copy
> 
> Can you add a "flag" or something to indicate that the iov pointer
> belongs to RAM and not to device state?
> 
> That way, I could re-use this code for RDMA - if I see this flag, I
> will know to send to RDMA.....

I am not sure you can, because the function will be preceded by
a qemu_put_be64 to store the header.  That header is not sent in
the RDMA case, isn't it?

Paolo
mrhines@linux.vnet.ibm.com - March 25, 2013, 3:18 p.m.
Right, the header's not used - but, are we certain that 
put_buffer_copy() will *always* be used for RAM in the future?

- Michael

On 03/25/2013 09:05 AM, Paolo Bonzini wrote:
>
> ----- Messaggio originale -----
>> Da: "Michael R. Hines" <mrhines@linux.vnet.ibm.com>
>> A: "Orit Wasserman" <owasserm@redhat.com>
>> Cc: qemu-devel@nongnu.org, pbonzini@redhat.com, mst@redhat.com, "chegu vinod" <chegu_vinod@hp.com>,
>> quintela@redhat.com
>> Inviato: Sabato, 23 marzo 2013 17:27:49
>> Oggetto: Re: [Qemu-devel] [RFC 10/12] Add qemu_put_buffer_no_copy
>>
>> Can you add a "flag" or something to indicate that the iov pointer
>> belongs to RAM and not to device state?
>>
>> That way, I could re-use this code for RDMA - if I see this flag, I
>> will know to send to RDMA.....
> I am not sure you can, because the function will be preceded by
> a qemu_put_be64 to store the header.  That header is not sent in
> the RDMA case, isn't it?
>
> Paolo
>
Paolo Bonzini - March 25, 2013, 3:59 p.m.
> Right, the header's not used - but, are we certain that
> put_buffer_copy() will *always* be used for RAM in the future?

I think you should not make any assumption and proceed as if this
series didn't exist.

Paolo

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 40d96f4..32a506e 100644
--- a/savevm.c
+++ b/savevm.c
@@ -629,6 +629,22 @@  void qemu_put_buffer(QEMUFile *f, const uint8_t *buf, int size)
 {
     int l;
 
+    while (size > 0) {
+        l = IO_BUF_SIZE - f->buf_index;
+        if (l > size) {
+            l = size;
+        }
+        memcpy(f->buf + f->buf_index, buf, l);
+        f->buf_index += l;
+        f->is_write = 1;
+        qemu_put_buffer_no_copy(f, f->buf + (f->buf_index - l), l);
+        buf += l;
+        size -= l;
+    }
+}
+
+void qemu_put_buffer_no_copy(QEMUFile *f, const uint8_t *buf, int size)
+{
     if (f->last_error) {
         return;
     }
@@ -639,24 +655,14 @@  void qemu_put_buffer(QEMUFile *f, const uint8_t *buf, int size)
         abort();
     }
 
-    while (size > 0) {
-        l = IO_BUF_SIZE - f->buf_index;
-        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;
-        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;
-            }
-        }
+    f->iov[f->iovcnt].iov_base = (uint8_t *)buf;
+    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);
     }
 }