[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 2/2] virtio-scsi: Optimize virtio_scsi_init_r
From: |
Paolo Bonzini |
Subject: |
Re: [Qemu-devel] [PATCH v2 2/2] virtio-scsi: Optimize virtio_scsi_init_req |
Date: |
Tue, 16 Sep 2014 10:15:56 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.0 |
Il 16/09/2014 09:16, Fam Zheng ha scritto:
> On Mon, 09/15 12:17, Paolo Bonzini wrote:
>> Il 15/09/2014 07:23, Fam Zheng ha scritto:
>>> SCSIRequest *sreq;
>>> size_t resp_size;
>>> enum SCSIXferMode mode;
>>> - QEMUIOVector resp_iov;
>>> union {
>>> VirtIOSCSICmdResp cmd;
>>> VirtIOSCSICtrlTMFResp tmf;
>>> @@ -68,23 +75,27 @@ 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);
>>> + VirtIOSCSICommon *vs = (VirtIOSCSICommon *)s;
>>> + const size_t zero_skip = offsetof(VirtIOSCSIReq, elem)
>>> + + sizeof(VirtQueueElement);
>>>
>>> + req = g_slice_alloc(sizeof(*req) + vs->cdb_size);
>>
>> Looks good, but why do you need to zero the union? You only need to
>> zero sreq, resp_size and mode, don't you (and at this point, memset
>> becomes superfluous)?
>>
>
> The structures in unions are not zeroed by caller, also leaving them breaks
> virtio-scsi in my test.
>
> FWIW, I will remove the "req->sreq = NULL;" two lines below in v3. At this
> point tuning these small fields are subtle optimization compared to the
> arrays,
> I say let's just simply keep the memset so that adding more fields in the
> future are also safe.
Perhaps the response fields have to be zeroed? The request shouldn't
need it. It can be done separately though---the VirtQueueElement is the
big one that we have to fix.
Paolo