[v3,8/9] Use qemu_put_buffer_no_copy for guest memory pages

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

Details

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

Commit Message

Orit Wasserman March 21, 2013, 4:05 p.m.
This will remove an unneeded copy of guest memory pages.
For the page header and device state we still copy the data to the
static buffer the other option is to allocate the memory on demand
which is more expensive.

Signed-off-by: Orit Wasserman <owasserm@redhat.com>
---
 arch_init.c | 2 +-
 savevm.c    | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

Comments

Michael S. Tsirkin March 21, 2013, 4:14 p.m.
On Thu, Mar 21, 2013 at 06:05:39PM +0200, Orit Wasserman wrote:
> This will remove an unneeded copy of guest memory pages.
> For the page header and device state we still copy the data to the
> static buffer the other option is to allocate the memory on demand
> which is more expensive.
> 
> Signed-off-by: Orit Wasserman <owasserm@redhat.com>

Okay so with this, the _nocopy can be later rewritten to do vmsplice to
save another copy? Cool.

> ---
>  arch_init.c | 2 +-
>  savevm.c    | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch_init.c b/arch_init.c
> index 98e2bc6..27b53eb 100644
> --- a/arch_init.c
> +++ b/arch_init.c
> @@ -481,7 +481,7 @@ static int ram_save_block(QEMUFile *f, bool last_stage)
>              /* XBZRLE overflow or normal page */
>              if (bytes_sent == -1) {
>                  bytes_sent = save_block_hdr(f, block, offset, cont, RAM_SAVE_FLAG_PAGE);
> -                qemu_put_buffer(f, p, TARGET_PAGE_SIZE);
> +                qemu_put_buffer_no_copy(f, p, TARGET_PAGE_SIZE);
>                  bytes_sent += TARGET_PAGE_SIZE;
>                  acct_info.norm_pages++;
>              }
> diff --git a/savevm.c b/savevm.c
> index fbfb9e3..50e8fb2 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -634,7 +634,7 @@ void qemu_put_buffer_no_copy(QEMUFile *f, const uint8_t *buf, int size)
>          abort();
>      }
>  
> -    f->iov[f->iovcnt].iov_base = f->buf + f->buf_index;
> +    f->iov[f->iovcnt].iov_base = (uint8_t *)buf;
>      f->iov[f->iovcnt++].iov_len = size;
>  
>      f->is_write = 1;
> -- 
> 1.7.11.7
Juan Quintela March 21, 2013, 5:37 p.m.
Orit Wasserman <owasserm@redhat.com> wrote:
> This will remove an unneeded copy of guest memory pages.
> For the page header and device state we still copy the data to the
> static buffer the other option is to allocate the memory on demand
> which is more expensive.
>
> Signed-off-by: Orit Wasserman <owasserm@redhat.com>
> ---
>  arch_init.c | 2 +-
>  savevm.c    | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch_init.c b/arch_init.c
> index 98e2bc6..27b53eb 100644
> --- a/arch_init.c
> +++ b/arch_init.c
> @@ -481,7 +481,7 @@ static int ram_save_block(QEMUFile *f, bool last_stage)
>              /* XBZRLE overflow or normal page */
>              if (bytes_sent == -1) {
>                  bytes_sent = save_block_hdr(f, block, offset, cont, RAM_SAVE_FLAG_PAGE);
> -                qemu_put_buffer(f, p, TARGET_PAGE_SIZE);
> +                qemu_put_buffer_no_copy(f, p, TARGET_PAGE_SIZE);
>                  bytes_sent += TARGET_PAGE_SIZE;
>                  acct_info.norm_pages++;
>              }

Once here, shouldn't we also change:

block-migration.c::blk_send()

    qemu_put_buffer(f, blk->buf, BLOCK_SIZE);

to nocopy?

Again, this can be an additional patch.


> diff --git a/savevm.c b/savevm.c
> index fbfb9e3..50e8fb2 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -634,7 +634,7 @@ void qemu_put_buffer_no_copy(QEMUFile *f, const uint8_t *buf, int size)
>          abort();
>      }
>  
> -    f->iov[f->iovcnt].iov_base = f->buf + f->buf_index;
> +    f->iov[f->iovcnt].iov_base = (uint8_t *)buf;
>      f->iov[f->iovcnt++].iov_len = size;

This bit should be in the previous patch, the error that I pointed?

Later, Juan.
Orit Wasserman March 21, 2013, 6:08 p.m.
On 03/21/2013 07:37 PM, Juan Quintela wrote:
> Orit Wasserman <owasserm@redhat.com> wrote:
>> This will remove an unneeded copy of guest memory pages.
>> For the page header and device state we still copy the data to the
>> static buffer the other option is to allocate the memory on demand
>> which is more expensive.
>>
>> Signed-off-by: Orit Wasserman <owasserm@redhat.com>
>> ---
>>  arch_init.c | 2 +-
>>  savevm.c    | 2 +-
>>  2 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch_init.c b/arch_init.c
>> index 98e2bc6..27b53eb 100644
>> --- a/arch_init.c
>> +++ b/arch_init.c
>> @@ -481,7 +481,7 @@ static int ram_save_block(QEMUFile *f, bool last_stage)
>>              /* XBZRLE overflow or normal page */
>>              if (bytes_sent == -1) {
>>                  bytes_sent = save_block_hdr(f, block, offset, cont, RAM_SAVE_FLAG_PAGE);
>> -                qemu_put_buffer(f, p, TARGET_PAGE_SIZE);
>> +                qemu_put_buffer_no_copy(f, p, TARGET_PAGE_SIZE);
>>                  bytes_sent += TARGET_PAGE_SIZE;
>>                  acct_info.norm_pages++;
>>              }
> 
> Once here, shouldn't we also change:
> 
> block-migration.c::blk_send()
> 
>     qemu_put_buffer(f, blk->buf, BLOCK_SIZE);
> 
> to nocopy?
Sadly no, I looked at the code and I saw the buffer is freed immediately
after call blk_send :(. look at mig_save_device_dirty.

Orit
> 
> Again, this can be an additional patch.
> 
> 
>> diff --git a/savevm.c b/savevm.c
>> index fbfb9e3..50e8fb2 100644
>> --- a/savevm.c
>> +++ b/savevm.c
>> @@ -634,7 +634,7 @@ void qemu_put_buffer_no_copy(QEMUFile *f, const uint8_t *buf, int size)
>>          abort();
>>      }
>>  
>> -    f->iov[f->iovcnt].iov_base = f->buf + f->buf_index;
>> +    f->iov[f->iovcnt].iov_base = (uint8_t *)buf;
>>      f->iov[f->iovcnt++].iov_len = size;
> 
> This bit should be in the previous patch, the error that I pointed?
> 
> Later, Juan.
>
Michael S. Tsirkin March 21, 2013, 6:21 p.m.
On Thu, Mar 21, 2013 at 08:08:58PM +0200, Orit Wasserman wrote:
> On 03/21/2013 07:37 PM, Juan Quintela wrote:
> > Orit Wasserman <owasserm@redhat.com> wrote:
> >> This will remove an unneeded copy of guest memory pages.
> >> For the page header and device state we still copy the data to the
> >> static buffer the other option is to allocate the memory on demand
> >> which is more expensive.
> >>
> >> Signed-off-by: Orit Wasserman <owasserm@redhat.com>
> >> ---
> >>  arch_init.c | 2 +-
> >>  savevm.c    | 2 +-
> >>  2 files changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/arch_init.c b/arch_init.c
> >> index 98e2bc6..27b53eb 100644
> >> --- a/arch_init.c
> >> +++ b/arch_init.c
> >> @@ -481,7 +481,7 @@ static int ram_save_block(QEMUFile *f, bool last_stage)
> >>              /* XBZRLE overflow or normal page */
> >>              if (bytes_sent == -1) {
> >>                  bytes_sent = save_block_hdr(f, block, offset, cont, RAM_SAVE_FLAG_PAGE);
> >> -                qemu_put_buffer(f, p, TARGET_PAGE_SIZE);
> >> +                qemu_put_buffer_no_copy(f, p, TARGET_PAGE_SIZE);
> >>                  bytes_sent += TARGET_PAGE_SIZE;
> >>                  acct_info.norm_pages++;
> >>              }
> > 
> > Once here, shouldn't we also change:
> > 
> > block-migration.c::blk_send()
> > 
> >     qemu_put_buffer(f, blk->buf, BLOCK_SIZE);
> > 
> > to nocopy?
> Sadly no, I looked at the code and I saw the buffer is freed immediately
> after call blk_send :(. look at mig_save_device_dirty.
> 
> Orit

Hmm if _nocopy also implies it is not safe to free the
buffer immediately, then I'd rather put this in a name
e.g. _async instead of _nocopy.

> > 
> > Again, this can be an additional patch.
> > 
> > 
> >> diff --git a/savevm.c b/savevm.c
> >> index fbfb9e3..50e8fb2 100644
> >> --- a/savevm.c
> >> +++ b/savevm.c
> >> @@ -634,7 +634,7 @@ void qemu_put_buffer_no_copy(QEMUFile *f, const uint8_t *buf, int size)
> >>          abort();
> >>      }
> >>  
> >> -    f->iov[f->iovcnt].iov_base = f->buf + f->buf_index;
> >> +    f->iov[f->iovcnt].iov_base = (uint8_t *)buf;
> >>      f->iov[f->iovcnt++].iov_len = size;
> > 
> > This bit should be in the previous patch, the error that I pointed?
> > 
> > Later, Juan.
> >

Patch hide | download patch | download mbox

diff --git a/arch_init.c b/arch_init.c
index 98e2bc6..27b53eb 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -481,7 +481,7 @@  static int ram_save_block(QEMUFile *f, bool last_stage)
             /* XBZRLE overflow or normal page */
             if (bytes_sent == -1) {
                 bytes_sent = save_block_hdr(f, block, offset, cont, RAM_SAVE_FLAG_PAGE);
-                qemu_put_buffer(f, p, TARGET_PAGE_SIZE);
+                qemu_put_buffer_no_copy(f, p, TARGET_PAGE_SIZE);
                 bytes_sent += TARGET_PAGE_SIZE;
                 acct_info.norm_pages++;
             }
diff --git a/savevm.c b/savevm.c
index fbfb9e3..50e8fb2 100644
--- a/savevm.c
+++ b/savevm.c
@@ -634,7 +634,7 @@  void qemu_put_buffer_no_copy(QEMUFile *f, const uint8_t *buf, int size)
         abort();
     }
 
-    f->iov[f->iovcnt].iov_base = f->buf + f->buf_index;
+    f->iov[f->iovcnt].iov_base = (uint8_t *)buf;
     f->iov[f->iovcnt++].iov_len = size;
 
     f->is_write = 1;