From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=34063 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Q7AsB-0006f2-U7 for qemu-devel@nongnu.org; Tue, 05 Apr 2011 14:21:05 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Q7AsA-0000ND-JN for qemu-devel@nongnu.org; Tue, 05 Apr 2011 14:21:03 -0400 Received: from e9.ny.us.ibm.com ([32.97.182.139]:49069) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Q7AsA-0000Lu-GJ for qemu-devel@nongnu.org; Tue, 05 Apr 2011 14:21:02 -0400 Received: from d01dlp01.pok.ibm.com (d01dlp01.pok.ibm.com [9.56.224.56]) by e9.ny.us.ibm.com (8.14.4/8.13.1) with ESMTP id p35Hr8Ou008221 for ; Tue, 5 Apr 2011 13:53:08 -0400 Received: from d01relay02.pok.ibm.com (d01relay02.pok.ibm.com [9.56.227.234]) by d01dlp01.pok.ibm.com (Postfix) with ESMTP id 6E7A538C803B for ; Tue, 5 Apr 2011 14:20:34 -0400 (EDT) Received: from d03av03.boulder.ibm.com (d03av03.boulder.ibm.com [9.17.195.169]) by d01relay02.pok.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id p35IKgP3459704 for ; Tue, 5 Apr 2011 14:20:42 -0400 Received: from d03av03.boulder.ibm.com (loopback [127.0.0.1]) by d03av03.boulder.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id p35IKfZK007984 for ; Tue, 5 Apr 2011 12:20:41 -0600 Message-ID: <4D9B5D70.8090105@linux.vnet.ibm.com> Date: Tue, 05 Apr 2011 13:20:32 -0500 From: Michael Roth MIME-Version: 1.0 References: In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: [Qemu-devel] Re: [PATCH v2] Improve accuracy of block migration bandwidth calculation List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Avishay Traeger Cc: qemu-devel@nongnu.org, Liran Schour On 04/03/2011 03:31 AM, Avishay Traeger wrote: > > Revised patch for improving the accuracy of the block migration bandwidth > calculation. Thanks a lot to Michael Roth for the input. > > For those that missed the original patch, here is the description: > block_mig_state.total_time is currently the sum of the read request > latencies. This is not very accurate because block migration uses aio and > so several requests can be submitted at once. Bandwidth should be computed > with wall-clock time, not by adding the latencies. In this case, > "total_time" has a higher value than it should, and so the computed > bandwidth is lower than it is in reality. This means that migration can > take longer than it needs to. > However, we don't want to use pure wall-clock time here. We are computing > bandwidth in the asynchronous phase, where the migration repeatedly wakes > up and sends some aio requests. The computed bandwidth will be used for > synchronous transfer. > > > Avishay > > > Signed-off-by: Avishay Traeger > --- > block-migration.c | 21 +++++++++------------ > 1 files changed, 9 insertions(+), 12 deletions(-) > > diff --git a/block-migration.c b/block-migration.c > index 8218bac..47296ca 100644 > --- a/block-migration.c > +++ b/block-migration.c > @@ -62,7 +62,6 @@ typedef struct BlkMigBlock { > QEMUIOVector qiov; > BlockDriverAIOCB *aiocb; > int ret; > - int64_t time; > QSIMPLEQ_ENTRY(BlkMigBlock) entry; > } BlkMigBlock; > > @@ -78,6 +77,7 @@ typedef struct BlkMigState { > int prev_progress; > int bulk_completed; > long double total_time; > + long double prev_time_offset; > int reads; > } BlkMigState; > > @@ -131,12 +131,6 @@ uint64_t blk_mig_bytes_total(void) > return sum<< BDRV_SECTOR_BITS; > } > > -static inline void add_avg_read_time(int64_t time) > -{ > - block_mig_state.reads++; > - block_mig_state.total_time += time; > -} > - > static inline long double compute_read_bwidth(void) > { > assert(block_mig_state.total_time != 0); > @@ -191,13 +185,14 @@ static void alloc_aio_bitmap(BlkMigDevState *bmds) > > static void blk_mig_read_cb(void *opaque, int ret) > { > + long double curr_time = qemu_get_clock_ns(rt_clock); > BlkMigBlock *blk = opaque; > > blk->ret = ret; > > - blk->time = qemu_get_clock_ns(rt_clock) - blk->time; > - > - add_avg_read_time(blk->time); > + block_mig_state.reads++; > + block_mig_state.total_time += (curr_time - > block_mig_state.prev_time_offset); > + block_mig_state.prev_time_offset = curr_time; > > QSIMPLEQ_INSERT_TAIL(&block_mig_state.blk_list, blk, entry); > bmds_set_aio_inflight(blk->bmds, blk->sector, blk->nr_sectors, 0); > @@ -250,7 +245,8 @@ static int mig_save_device_bulk(Monitor *mon, QEMUFile > *f, > blk->iov.iov_len = nr_sectors * BDRV_SECTOR_SIZE; > qemu_iovec_init_external(&blk->qiov,&blk->iov, 1); > > - blk->time = qemu_get_clock_ns(rt_clock); > + if (block_mig_state.submitted == 0) > + block_mig_state.prev_time_offset = qemu_get_clock_ns(rt_clock); > > blk->aiocb = bdrv_aio_readv(bs, cur_sector,&blk->qiov, > nr_sectors, blk_mig_read_cb, blk); > @@ -409,7 +405,8 @@ static int mig_save_device_dirty(Monitor *mon, QEMUFile > *f, > blk->iov.iov_len = nr_sectors * BDRV_SECTOR_SIZE; > qemu_iovec_init_external(&blk->qiov,&blk->iov, 1); > > - blk->time = qemu_get_clock_ns(rt_clock); > + if (block_mig_state.submitted == 0) > + block_mig_state.prev_time_offset = qemu_get_clock_ns > (rt_clock); > > blk->aiocb = bdrv_aio_readv(bmds->bs, sector,&blk->qiov, > nr_sectors, blk_mig_read_cb, > blk); > -- > 1.7.0.4 > Looks good. This addresses the concerns I had. Reviewed-by: Michael Roth