[Qemu-devel] Re: [PATCH 09/10] Exit loop if we have been there too long

[Michael S. Tsirkin]

On Wed, Nov 24, 2010 at 12:03:06AM +0100, Juan Quintela wrote:
> From: Juan Quintela <address@hidden>
> 
> cheking each 64 pages is a random magic number as good as any other.
> We don't want to test too many times, but on the other hand,
> qemu_get_clock_ns() is not so expensive either.
> 

Could you please explain what's the problem this fixes?
I would like to see an API that documents the contract
we are making with the backend.

> Signed-off-by: Juan Quintela <address@hidden>
> Signed-off-by: Juan Quintela <address@hidden>
> ---
>  arch_init.c |   16 ++++++++++++++++
>  1 files changed, 16 insertions(+), 0 deletions(-)
> 
> diff --git a/arch_init.c b/arch_init.c
> index d32aaae..b463798 100644
> --- a/arch_init.c
> +++ b/arch_init.c
> @@ -40,6 +40,7 @@
>  #include "net.h"
>  #include "gdbstub.h"
>  #include "hw/smbios.h"
> +#include "buffered_file.h"
> 
>  #ifdef TARGET_SPARC
>  int graphic_width = 1024;
> @@ -218,6 +219,7 @@ int ram_save_live(Monitor *mon, QEMUFile *f, int stage, 
> void *opaque)
>      uint64_t bytes_transferred_last;
>      uint64_t t0;
>      double bwidth = 0;
> +    int i;
> 
>      if (stage < 0) {
>          cpu_physical_memory_set_dirty_tracking(0);
> @@ -261,6 +263,7 @@ int ram_save_live(Monitor *mon, QEMUFile *f, int stage, 
> void *opaque)
>      bytes_transferred_last = bytes_transferred;
>      t0 = qemu_get_clock_ns(rt_clock);
> 
> +    i = 0;
>      while (!qemu_file_rate_limit(f)) {
>          int bytes_sent;
> 
> @@ -269,6 +272,19 @@ int ram_save_live(Monitor *mon, QEMUFile *f, int stage, 
> void *opaque)
>          if (bytes_sent == 0) { /* no more blocks */
>              break;
>          }
> +     /* we want to check in the 1st loop, just in case it was the 1st time
> +           and we had to sync the dirty bitmap.
> +           qemu_get_clock_ns() is a bit expensive, so we only check each some
> +           iterations
> +     */
> +        if ((i & 63) == 0) {
> +            uint64_t t1 = (qemu_get_clock_ns(rt_clock) - t0) / 1000000;

This adds even more non-determinism to savevm behaviour.  If bandwidth
limit is higth enough, I expect it to just keep going.

> +            if (t1 > buffered_file_interval/2) {

arch_init should not depend on buffered_file implementation IMO.

Also - / 2?

> +                printf("big delay %ld milliseconds, %d iterations\n", t1, i);

Is this a debugging aid?

> +             break;
> +         }
> +     }
> +        i++;
>      }
> 
>      t0 = qemu_get_clock_ns(rt_clock) - t0;
> -- 
> 1.7.3.2
>