From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:60938) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Tap3d-0001pR-45 for qemu-devel@nongnu.org; Tue, 20 Nov 2012 09:44:17 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Tap3Y-00005S-JH for qemu-devel@nongnu.org; Tue, 20 Nov 2012 09:44:13 -0500 Received: from mx1.redhat.com ([209.132.183.28]:57743) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Tap3Y-00005O-BA for qemu-devel@nongnu.org; Tue, 20 Nov 2012 09:44:08 -0500 Received: from int-mx02.intmail.prod.int.phx2.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id qAKEi7xS010840 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Tue, 20 Nov 2012 09:44:07 -0500 Message-ID: <50AB9735.4090608@redhat.com> Date: Tue, 20 Nov 2012 15:44:05 +0100 From: Paolo Bonzini MIME-Version: 1.0 References: <1353342180-20392-1-git-send-email-pbonzini@redhat.com> <87wqxgcz9l.fsf@elfo.mitica> In-Reply-To: <87wqxgcz9l.fsf@elfo.mitica> Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] migration: fix rate limiting List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: quintela@redhat.com Cc: owasserm@redhat.com, qemu-devel@nongnu.org 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 >> --- >> 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;