From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34961) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XS22l-0007Xx-M0 for qemu-devel@nongnu.org; Thu, 11 Sep 2014 06:56:10 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XS22d-0005yA-5L for qemu-devel@nongnu.org; Thu, 11 Sep 2014 06:56:03 -0400 Received: from mx1.redhat.com ([209.132.183.28]:13171) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XS22c-0005xy-TG for qemu-devel@nongnu.org; Thu, 11 Sep 2014 06:55:55 -0400 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s8BAtsEO011995 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL) for ; Thu, 11 Sep 2014 06:55:54 -0400 Message-ID: <54117FB7.6020103@redhat.com> Date: Thu, 11 Sep 2014 12:55:51 +0200 From: Paolo Bonzini MIME-Version: 1.0 References: <1410430599-27540-1-git-send-email-famz@redhat.com> <1410430599-27540-3-git-send-email-famz@redhat.com> In-Reply-To: <1410430599-27540-3-git-send-email-famz@redhat.com> Content-Type: text/plain; charset=iso-8859-15 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 2/2] virtio-scsi: Optimize virtio_scsi_init_req List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Fam Zheng , qemu-devel@nongnu.org Similar nits to patch 1, but a good patch nevertheless! Il 11/09/2014 12:16, Fam Zheng ha scritto: > The VirtQueueElement is a very big structure (> 4k), since it will be > initialzed by virtqueue_pop, we can save the expensive zeroing here. > > This saves a few nanoseconds per request in my test: > > [fio-test] rw bs iodepth jobs bw iops latency > -------------------------------------------------------------------------------------------- > Before read 4k 1 1 110 28269 34 > After read 4k 1 1 131 33745 28 > > virtio-blk read 4k 1 1 217 55673 16 > > Signed-off-by: Fam Zheng > --- > hw/scsi/virtio-scsi.c | 17 ++++++++++------- > include/hw/virtio/virtio-scsi.h | 1 + > 2 files changed, 11 insertions(+), 7 deletions(-) > > diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c > index 86aba88..0792529 100644 > --- a/hw/scsi/virtio-scsi.c > +++ b/hw/scsi/virtio-scsi.c > @@ -24,12 +24,12 @@ > typedef struct VirtIOSCSIReq { > VirtIOSCSI *dev; > VirtQueue *vq; > - VirtQueueElement elem; > QEMUSGList qsgl; > + QEMUIOVector resp_iov; > + VirtQueueElement elem; Needs a comment where the zeroed section begins. > SCSIRequest *sreq; > size_t resp_size; > enum SCSIXferMode mode; > - QEMUIOVector resp_iov; > union { > VirtIOSCSICmdResp cmd; > VirtIOSCSICtrlTMFResp tmf; > @@ -44,6 +44,7 @@ typedef struct VirtIOSCSIReq { > VirtIOSCSICtrlTMFReq tmf; > VirtIOSCSICtrlANReq an; > } req; > + uint8_t cdb[VIRTIO_SCSI_CDB_SIZE_MAX]; > } VirtIOSCSIReq; > > QEMU_BUILD_BUG_ON(offsetof(VirtIOSCSIReq, req.cdb) != > @@ -68,15 +69,16 @@ static inline SCSIDevice *virtio_scsi_device_find(VirtIOSCSI *s, uint8_t *lun) > static VirtIOSCSIReq *virtio_scsi_init_req(VirtIOSCSI *s, VirtQueue *vq) > { > VirtIOSCSIReq *req; > - VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(s); > - > - req = g_malloc0(sizeof(*req) + vs->cdb_size); > + const size_t zero_skip = offsetof(VirtIOSCSIReq, elem) > + + sizeof(VirtQueueElement); > > + req = g_slice_new(VirtIOSCSIReq); I would use g_slice_alloc here, and avoid zeroing the largeish cdb field. > req->vq = vq; > req->dev = s; > req->sreq = NULL; This NULL initialization can be removed. Paolo > qemu_sglist_init(&req->qsgl, DEVICE(s), 8, &address_space_memory); > qemu_iovec_init(&req->resp_iov, 1); > + memset((uint8_t *)req + zero_skip, 0, sizeof(*req) - zero_skip); > return req; > } > > @@ -84,7 +86,7 @@ static void virtio_scsi_free_req(VirtIOSCSIReq *req) > { > qemu_iovec_destroy(&req->resp_iov); > qemu_sglist_destroy(&req->qsgl); > - g_free(req); > + g_slice_free(VirtIOSCSIReq, req); > } > > static void virtio_scsi_complete_req(VirtIOSCSIReq *req) > @@ -532,7 +534,8 @@ static void virtio_scsi_set_config(VirtIODevice *vdev, > VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(vdev); > > if ((uint32_t) virtio_ldl_p(vdev, &scsiconf->sense_size) >= 65536 || > - (uint32_t) virtio_ldl_p(vdev, &scsiconf->cdb_size) >= 256) { > + (uint32_t) virtio_ldl_p(vdev, &scsiconf->cdb_size) > + >= VIRTIO_SCSI_CDB_SIZE_MAX) { > error_report("bad data written to virtio-scsi configuration space"); > exit(1); > } > diff --git a/include/hw/virtio/virtio-scsi.h b/include/hw/virtio/virtio-scsi.h > index 188a2d9..6e876f4 100644 > --- a/include/hw/virtio/virtio-scsi.h > +++ b/include/hw/virtio/virtio-scsi.h > @@ -37,6 +37,7 @@ > > #define VIRTIO_SCSI_VQ_SIZE 128 > #define VIRTIO_SCSI_CDB_SIZE 32 > +#define VIRTIO_SCSI_CDB_SIZE_MAX 256 > #define VIRTIO_SCSI_SENSE_SIZE 96 > #define VIRTIO_SCSI_MAX_CHANNEL 0 > #define VIRTIO_SCSI_MAX_TARGET 255 >