Il 20/11/2012 12:03, Juan Quintela ha scritto: > This patch is wrong O;-) > That don't mean that current code is right. > > We have 4 variables: > - xfer_limit: how much we are allowed to send each 100ms (in bytes) > - buffer_capacity: the size of the buffer that we are using > - buffer_size: the amount of the previous buffer that we are using > buffer_size < buffer_capacity, or we are doing something > wrong. > - bytes_xfer: How many bytes we have transfered since last 100ms timer.
Note that the buffered_file places a wall between producer and consumer (or rather tries to place it; my patch is an attempt to fix). The consumer side is buffered_flush & bytes_xfer, and the rate-limiting there is mandatory. However, on the producer side, the rate-limiting is only advisory, to avoid making the buffer_capacity too large. In principle (and leaving MAX_WAIT aside for a moment) you could skip qemu_file_rate_limit calls completely. It would place the whole RAM in the buffer, and still transfer it in xfer_limit chunks on the wire. > And how this work: > - we have an input handler that copies stuff from RAM to buffer > it stops when we have sent more that xfer_limit on this period (each > 100ms) > - we have another handler that is run each 100ms, and this one sent > anything that is on the buffer, and reset bytes_xfer to zero. > > WHat you have done is just telling that in the 1st input handler, that > we always have size on the buffer for it, so that we are not doing any > rate limiting at all. > > It is very strange that buffer_capacity is bigger that xfer_limit (it > could happen, but it is very unusual), and then what you have done there > is just disabling rate limiting altogether. I haven't: once s->bytes_xfer >= s->xfer_limit, buffered_flush will not do anything, and data will accumulate in the buffer until bytes_xfer is moved back to 0. So what happens really is that ram_save_block is already preparing the next chunk of data to send, but only s->xfer_limit bytes are sent on each 100ms period. However, my patch was incomplete. The desired behavior is that buffered_put_buffer(s, NULL, 0, 0) will restart the iteration, so it has to check buffer_size too. In fact, the check in buffered_put_b There is also another bug in the current code, which is an off-by-one. Comparison is s->bytes_xfer > s->xfer_limit, but it should be >= instead. And more somewhat broken checks. I'm testing a more complete fix. Paolo > > So .... > > NACK > >> >> Signed-off-by: Paolo Bonzini <[email protected]> >> --- >> buffered_file.c | 2 +- >> 1 file modificato, 1 inserzione(+). 1 rimozione(-) >> >> diff --git a/buffered_file.c b/buffered_file.c >> index bd0f61d..46cd591 100644 >> --- a/buffered_file.c >> +++ b/buffered_file.c >> @@ -193,7 +193,7 @@ static int buffered_rate_limit(void *opaque) >> if (s->freeze_output) >> return 1; >> >> - if (s->bytes_xfer > s->xfer_limit) > o> + if (s->buffer_size > s->xfer_limit) >> return 1; >> >> return 0;
