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

["Michael S. Tsirkin" <mst@redhat.com>]

On Wed, Nov 24, 2010 at 12:01:51PM +0100, Juan Quintela wrote:
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > On Wed, Nov 24, 2010 at 12:03:06AM +0100, Juan Quintela wrote:
> >> From: Juan Quintela <quintela@trasno.org>
> >> 
> >> 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.
> 
> buffered_file is an "abstraction" that uses a buffer.
> 
> live migration code (remember it can't sleep, it runs on the main loop)
> stores its "stuff" on that buffer.  And a timer writes that buffer to
> the fd that is associated with migration.
> 
> This design is due to the main_loop/no threads qemu model.
> 
> buffered_file timer runs each 100ms.  And we "try" to measure channel
> bandwidth from there.  If we are not able to run the timer, all the
> calculations are wrong, and then stalls happens.

So the problem is the timer in the buffered file abstraction?
Why don't we just flush out data if the buffer is full?

> 
> 
> >> @@ -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 we find a row of 512MB of zero pages together (and that happens if
> you have a 64GB iddle guest, then you can spent more than 3seconds to
> fill the default bandwith).  After that everything that uses the main
> loop has had stalls.
> 
> 
> >> +            if (t1 > buffered_file_interval/2) {
> >
> > arch_init should not depend on buffered_file implementation IMO.
> >
> > Also - / 2?
> 
> We need to run a timer each 100ms.  For times look at the 0/6 patch.
> We can't spent more that 50ms in each function.  It is something that
> should happen for all funnctions called from io_handlers.
> 
> >> +                printf("big delay %ld milliseconds, %d iterations\n", t1, i);
> >
> > Is this a debugging aid?
> 
> I left that on purpose, to show that it happens a lot.  There is no
> DEBUG_ARCH or DEBUG_RAM around, I can create them if you preffer.  But
> notice that this is something that shouldn't happen (but it happens).
> 
> DPRINTF for that file should be a good idea, will do.
> 
> >> +		break;
> >> +	    }
> >> +	}
> >> +        i++;
> >>      }
> >> 
> >>      t0 = qemu_get_clock_ns(rt_clock) - t0;
> >> -- 
> >> 1.7.3.2
> >>