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

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

Comments

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

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;