From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48544) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zw2XD-0002J0-LL for qemu-devel@nongnu.org; Tue, 10 Nov 2015 01:36:04 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Zw2XC-0004xF-JA for qemu-devel@nongnu.org; Tue, 10 Nov 2015 01:36:03 -0500 Message-ID: <56419027.8090509@huawei.com> Date: Tue, 10 Nov 2015 14:35:19 +0800 From: Gonglei MIME-Version: 1.0 References: <1447059810-11376-1-git-send-email-arei.gonglei@huawei.com> <20151109135722.GB27221@stefanha-x1.localdomain> In-Reply-To: <20151109135722.GB27221@stefanha-x1.localdomain> Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v3] virtio-blk: trivial code optimization List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi Cc: pbonzini@redhat.com, qemu-devel@nongnu.org, qemu-block@nongnu.org On 2015/11/9 21:57, Stefan Hajnoczi wrote: > On Mon, Nov 09, 2015 at 05:03:30PM +0800, arei.gonglei@huawei.com wrote: >> From: Gonglei >> >> 1. avoid possible superflous checking >> 2. make code more robustness >> >> Signed-off-by: Gonglei >> Reviewed-by: Fam Zheng >> --- >> v3: change the third condition too [Paolo] >> add Fam's R-by >> --- >> hw/block/virtio-blk.c | 27 +++++++++------------------ >> 1 file changed, 9 insertions(+), 18 deletions(-) >> >> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c >> index 093e475..9124358 100644 >> --- a/hw/block/virtio-blk.c >> +++ b/hw/block/virtio-blk.c >> @@ -404,24 +404,15 @@ void virtio_blk_submit_multireq(BlockBackend *blk, MultiReqBuffer *mrb) >> for (i = 0; i < mrb->num_reqs; i++) { >> VirtIOBlockReq *req = mrb->reqs[i]; >> if (num_reqs > 0) { >> - bool merge = true; >> - >> - /* merge would exceed maximum number of IOVs */ >> - if (niov + req->qiov.niov > IOV_MAX) { >> - merge = false; >> - } >> - >> - /* merge would exceed maximum transfer length of backend device */ >> - if (req->qiov.size / BDRV_SECTOR_SIZE + nb_sectors > max_xfer_len) { >> - merge = false; >> - } >> - >> - /* requests are not sequential */ >> - if (sector_num + nb_sectors != req->sector_num) { >> - merge = false; >> - } >> - >> - if (!merge) { >> + /* >> + * NOTE: We cannot merge the requests in below situations: >> + * 1. requests are not sequential >> + * 2. merge would exceed maximum number of IOVs >> + * 3. merge would exceed maximum transfer length of backend device >> + */ >> + if (sector_num + nb_sectors != req->sector_num || >> + niov > IOV_MAX - req->qiov.niov || >> + nb_sectors > max_xfer_len - req->qiov.size / BDRV_SECTOR_SIZE) { > > nb_sectors - int > max_xfer_len - int > req->qiov.size - size_t > BDRV_SECTOR_SIZE - unsigned long long > > Therefore this expression is an int > unsigned long long comparison. > Sorry, I'm confused. max_xfer_len is int, "req->qiov.size / BDRV_SECTOR_SIZE" is unsigned long long, so, "max_xfer_len - req->qiov.size / BDRV_SECTOR_SIZE" will be int, and nb_sectors is int too, so this comparison is right. Am I wrong? > req->qiov.size can be larger than max_xfer_len so this comparison > suffers from underflow. Please check that req->qiov.size <= > max_xfer_len first. > Regards, -Gonglei