Re: [Qemu-devel] [PATCH 1/4] ram: add free_space parameter to save_live functions

[Orit Wasserman]

On 01/18/2013 01:53 PM, Juan Quintela wrote:
> As we really know how much space we have free in the buffers, we can
> send that information instead of guessing how much we can sent each time.
> 
> Signed-off-by: Juan Quintela <address@hidden>
> ---
>  arch_init.c                 | 20 +++++++++-----------
>  block-migration.c           |  2 +-
>  include/migration/vmstate.h |  2 +-
>  include/sysemu/sysemu.h     |  2 +-
>  migration.c                 |  3 ++-
>  savevm.c                    | 10 +++++++---
>  6 files changed, 21 insertions(+), 18 deletions(-)
> 
> diff --git a/arch_init.c b/arch_init.c
> index dada6de..2792b76 100644
> --- a/arch_init.c
> +++ b/arch_init.c
> @@ -601,9 +601,12 @@ static int ram_save_setup(QEMUFile *f, void *opaque)
>      return 0;
>  }
> 
> -static int ram_save_iterate(QEMUFile *f, void *opaque)
> +/* Maximum size for a transmited page
> +                       header + len + idstr + page size */
> +#define MAX_PAGE_SIZE (8      + 1   + 256  + TARGET_PAGE_SIZE)
> +
> +static int ram_save_iterate(QEMUFile *f, void *opaque, uint64_t free_space)
>  {
> -    int ret;
>      int i;
>      int64_t t0;
>      int total_sent = 0;
> @@ -616,15 +619,15 @@ static int ram_save_iterate(QEMUFile *f, void *opaque)
> 
>      t0 = qemu_get_clock_ns(rt_clock);
>      i = 0;
> -    while ((ret = qemu_file_rate_limit(f)) == 0) {
> -        int bytes_sent;
> -
> -        bytes_sent = ram_save_block(f, false);
> +    /* We need space for at least one page and end of section marker */
> +    while (free_space > MAX_PAGE_SIZE + 8) {
Actually we may need more if we move to a new memory block we will need to add 
the block idstr
and may run of space (not talking about compression which requires less space 
and we may have it)....
Why not move this logic into ram_save_block?
> +        int bytes_sent = ram_save_block(f, false);
>          /* no more blocks to sent */
>          if (bytes_sent == 0) {
>              break;
>          }
>          total_sent += bytes_sent;
> +        free_space -= bytes_sent;
>          acct_info.iterations++;
>          /* we want to check in the 1st loop, just in case it was the 1st time
>             and we had to sync the dirty bitmap.
> @@ -644,11 +647,6 @@ static int ram_save_iterate(QEMUFile *f, void *opaque)
> 
>      qemu_mutex_unlock_ramlist();
> 
> -    if (ret < 0) {
> -        bytes_transferred += total_sent;
> -        return ret;
> -    }
> -
don't we need to return negative to release the lock sometimes?
>      qemu_put_be64(f, RAM_SAVE_FLAG_EOS);
>      total_sent += 8;
>      bytes_transferred += total_sent;
> diff --git a/block-migration.c b/block-migration.c
> index 6acf3e1..0c3157a 100644
> --- a/block-migration.c
> +++ b/block-migration.c
> @@ -535,7 +535,7 @@ static int block_save_setup(QEMUFile *f, void *opaque)
>      return 0;
>  }
> 
> -static int block_save_iterate(QEMUFile *f, void *opaque)
> +static int block_save_iterate(QEMUFile *f, void *opaque, uint64_t free_space)
>  {
>      int ret;
> 
> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> index f27276c..0b55cf4 100644
> --- a/include/migration/vmstate.h
> +++ b/include/migration/vmstate.h
> @@ -33,7 +33,7 @@ typedef struct SaveVMHandlers {
>      void (*set_params)(const MigrationParams *params, void * opaque);
>      SaveStateHandler *save_state;
>      int (*save_live_setup)(QEMUFile *f, void *opaque);
> -    int (*save_live_iterate)(QEMUFile *f, void *opaque);
> +    int (*save_live_iterate)(QEMUFile *f, void *opaque, uint64_t free_space);
>      int (*save_live_complete)(QEMUFile *f, void *opaque);
>      uint64_t (*save_live_pending)(QEMUFile *f, void *opaque, uint64_t 
> max_size);
>      void (*cancel)(void *opaque);
> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
> index d65a9f1..3ff043c 100644
> --- a/include/sysemu/sysemu.h
> +++ b/include/sysemu/sysemu.h
> @@ -75,7 +75,7 @@ void qemu_announce_self(void);
>  bool qemu_savevm_state_blocked(Error **errp);
>  int qemu_savevm_state_begin(QEMUFile *f,
>                              const MigrationParams *params);
> -int qemu_savevm_state_iterate(QEMUFile *f);
> +int qemu_savevm_state_iterate(QEMUFile *f, uint64_t free_space);
>  int qemu_savevm_state_complete(QEMUFile *f);
>  void qemu_savevm_state_cancel(void);
>  uint64_t qemu_savevm_state_pending(QEMUFile *f, uint64_t max_size);
> diff --git a/migration.c b/migration.c
> index 77c1971..e74ce49 100644
> --- a/migration.c
> +++ b/migration.c
> @@ -683,6 +683,7 @@ static void *buffered_file_thread(void *opaque)
>      while (true) {
>          int64_t current_time = qemu_get_clock_ms(rt_clock);
>          uint64_t pending_size;
> +        size_t free_space = s->buffer_capacity - s->buffer_size;
don't we need to take into consideration the rate_limit (xfer_limit)
otherwise we may send too much.
> 
>          qemu_mutex_lock_iothread();
>          if (s->state != MIG_STATE_ACTIVE) {
> @@ -699,7 +700,7 @@ static void *buffered_file_thread(void *opaque)
>              pending_size = qemu_savevm_state_pending(s->file, max_size);
>              DPRINTF("pending size %lu max %lu\n", pending_size, max_size);
>              if (pending_size && pending_size >= max_size) {
> -                ret = qemu_savevm_state_iterate(s->file);
> +                ret = qemu_savevm_state_iterate(s->file, free_space);
ret is never negative so the lock won't be released
>                  if (ret < 0) {
>                      qemu_mutex_unlock_iothread();
>                      break;
> diff --git a/savevm.c b/savevm.c
> index 913a623..3447f91 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -1609,10 +1609,11 @@ int qemu_savevm_state_begin(QEMUFile *f,
>   *   0 : We haven't finished, caller have to go again
>   *   1 : We have finished, we can go to complete phase
>   */
> -int qemu_savevm_state_iterate(QEMUFile *f)
> +int qemu_savevm_state_iterate(QEMUFile *f, uint64_t free_space)
>  {
>      SaveStateEntry *se;
>      int ret = 1;
> +    size_t remaining_space = free_space;
can't we add free_space variable the QemuFile and make 
all the qemu_put_byte .. qemu_put_buffer functions to update it?
this way we won't need to pass it to the iterate functions ...

Regards,
Orit
> 
>      QTAILQ_FOREACH(se, &savevm_handlers, entry) {
>          if (!se->ops || !se->ops->save_live_iterate) {
> @@ -1629,9 +1630,11 @@ int qemu_savevm_state_iterate(QEMUFile *f)
>          trace_savevm_section_start();
>          /* Section type */
>          qemu_put_byte(f, QEMU_VM_SECTION_PART);
> +        remaining_space -= 1;
>          qemu_put_be32(f, se->section_id);
> +        remaining_space -= 4;
> 
> -        ret = se->ops->save_live_iterate(f, se->opaque);
> +        ret = se->ops->save_live_iterate(f, se->opaque, remaining_space);
>          trace_savevm_section_end(se->section_id);
> 
>          if (ret <= 0) {
> @@ -1641,6 +1644,7 @@ int qemu_savevm_state_iterate(QEMUFile *f)
>                 synchronized over and over again. */
>              break;
>          }
> +        remaining_space -= ret;
>      }
>      if (ret != 0) {
>          return ret;
> @@ -1756,7 +1760,7 @@ static int qemu_savevm_state(QEMUFile *f)
>          goto out;
> 
>      do {
> -        ret = qemu_savevm_state_iterate(f);
> +        ret = qemu_savevm_state_iterate(f, SIZE_MAX);
>          if (ret < 0)
>              goto out;
>      } while (ret == 0);
>