From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:45470) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TapJ3-0000Q5-6U for qemu-devel@nongnu.org; Tue, 20 Nov 2012 10:00:18 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TapIp-0005Wd-Ko for qemu-devel@nongnu.org; Tue, 20 Nov 2012 10:00:03 -0500 Received: from mx1.redhat.com ([209.132.183.28]:8616) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TapIp-0005W3-CV for qemu-devel@nongnu.org; Tue, 20 Nov 2012 09:59:55 -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 qAKExsjF016346 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Tue, 20 Nov 2012 09:59:54 -0500 From: Juan Quintela In-Reply-To: <50AB9735.4090608@redhat.com> (Paolo Bonzini's message of "Tue, 20 Nov 2012 15:44:05 +0100") References: <1353342180-20392-1-git-send-email-pbonzini@redhat.com> <87wqxgcz9l.fsf@elfo.mitica> <50AB9735.4090608@redhat.com> Date: Tue, 20 Nov 2012 15:59:47 +0100 Message-ID: <87obiscobg.fsf@elfo.mitica> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH] migration: fix rate limiting Reply-To: quintela@redhat.com List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: owasserm@redhat.com, qemu-devel@nongnu.org Paolo Bonzini wrote: > 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. this is the whole definition of rate-limiting O:-) Remember how this works, with callbacks. This was "another" of the reasons to move to a thread, to have finer control about that. If you look at latest patches you can see that I pass the "amount" of space in the buffer to the producer, and plan to just remove this calls (make completely no sense once that you moved to a thread). >> 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. Later, Juan.