[Qemu-devel] [PATCH] virtio-blk: trivial code optimization

[Qemu-devel] [PATCH] virtio-blk: trivial code optimization

[arei.gonglei]

From: Gonglei <arei.gonglei@huawei.com>

Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
 hw/block/virtio-blk.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 093e475..752586d 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -409,18 +409,20 @@ void virtio_blk_submit_multireq(BlockBackend *blk, MultiReqBuffer *mrb)
             /* merge would exceed maximum number of IOVs */
             if (niov + req->qiov.niov > IOV_MAX) {
                 merge = false;
+                goto unmerge;
             }
 
             /* merge would exceed maximum transfer length of backend device */
             if (req->qiov.size / BDRV_SECTOR_SIZE + nb_sectors > max_xfer_len) {
                 merge = false;
+                goto unmerge;
             }
 
             /* requests are not sequential */
             if (sector_num + nb_sectors != req->sector_num) {
                 merge = false;
             }
-
+unmerge:
             if (!merge) {
                 submit_requests(blk, mrb, start, num_reqs, niov);
                 num_reqs = 0;
-- 
1.7.12.4

Re: [Qemu-devel] [PATCH] virtio-blk: trivial code optimization

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1146 bytes --]

On Fri, Nov 06, 2015 at 09:04:57AM +0800, arei.gonglei@huawei.com wrote:
> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> index 093e475..752586d 100644
> --- a/hw/block/virtio-blk.c
> +++ b/hw/block/virtio-blk.c
> @@ -409,18 +409,20 @@ void virtio_blk_submit_multireq(BlockBackend *blk, MultiReqBuffer *mrb)
>              /* merge would exceed maximum number of IOVs */
>              if (niov + req->qiov.niov > IOV_MAX) {
>                  merge = false;
> +                goto unmerge;
>              }
>  
>              /* merge would exceed maximum transfer length of backend device */
>              if (req->qiov.size / BDRV_SECTOR_SIZE + nb_sectors > max_xfer_len) {
>                  merge = false;
> +                goto unmerge;
>              }
>  
>              /* requests are not sequential */
>              if (sector_num + nb_sectors != req->sector_num) {
>                  merge = false;
>              }
> -
> +unmerge:

C has a way of expressing this without gotos.  Please use else if:

  if (a) {
      ...
  } else if (b) {
      ...
  } else if (c) {
      ...
  }

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH] virtio-blk: trivial code optimization

[Paolo Bonzini]

On 06/11/2015 11:35, Stefan Hajnoczi wrote:
>> >              if (niov + req->qiov.niov > IOV_MAX) {
>> >                  merge = false;
>> > +                goto unmerge;
>> >              }
>> >  
>> >              /* merge would exceed maximum transfer length of backend device */
>> >              if (req->qiov.size / BDRV_SECTOR_SIZE + nb_sectors > max_xfer_len) {
>> >                  merge = false;
>> > +                goto unmerge;
>> >              }
>> >  
>> >              /* requests are not sequential */
>> >              if (sector_num + nb_sectors != req->sector_num) {
>> >                  merge = false;
>> >              }
>> > -
>> > +unmerge:
> C has a way of expressing this without gotos.  Please use else if:
> 
>   if (a) {
>       ...
>   } else if (b) {
>       ...
>   } else if (c) {
>       ...
>   }

Another way is

if (niov + req->qiov.niov > IOV_MAX ||
    req->qiov.size / BDRV_SECTOR_SIZE + nb_sectors > max_xfer_len ||
    sector_num + nb_sectors != req->sector_num) {
    submit_requests(...)
    ...
}

While at it, we could reorder the conditions so that the most common
("requests are not sequential") comes first.

I'm not sure about handling of overflow.  It's probably better to
write conditions as "new > max - old" (e.g. "niov > IOV_MAX -
req->qiov.niov") rather than "old + new > max".  The former is always
safe, because we know that old <= max and there can be no integer
overflow.

Thanks,

Paolo

Re: [Qemu-devel] [PATCH] virtio-blk: trivial code optimization

[Gonglei]

On 2015/11/6 20:54, Paolo Bonzini wrote:
> 
> 
> On 06/11/2015 11:35, Stefan Hajnoczi wrote:
>>>>              if (niov + req->qiov.niov > IOV_MAX) {
>>>>                  merge = false;
>>>> +                goto unmerge;
>>>>              }
>>>>  
>>>>              /* merge would exceed maximum transfer length of backend device */
>>>>              if (req->qiov.size / BDRV_SECTOR_SIZE + nb_sectors > max_xfer_len) {
>>>>                  merge = false;
>>>> +                goto unmerge;
>>>>              }
>>>>  
>>>>              /* requests are not sequential */
>>>>              if (sector_num + nb_sectors != req->sector_num) {
>>>>                  merge = false;
>>>>              }
>>>> -
>>>> +unmerge:
>> C has a way of expressing this without gotos.  Please use else if:
>>
>>   if (a) {
>>       ...
>>   } else if (b) {
>>       ...
>>   } else if (c) {
>>       ...
>>   }
> 
> Another way is
> 
> if (niov + req->qiov.niov > IOV_MAX ||
>     req->qiov.size / BDRV_SECTOR_SIZE + nb_sectors > max_xfer_len ||
>     sector_num + nb_sectors != req->sector_num) {
>     submit_requests(...)
>     ...
> }
> 
> While at it, we could reorder the conditions so that the most common
> ("requests are not sequential") comes first.
> 
> I'm not sure about handling of overflow.  It's probably better to
> write conditions as "new > max - old" (e.g. "niov > IOV_MAX -
> req->qiov.niov") rather than "old + new > max".  The former is always
> safe, because we know that old <= max and there can be no integer
> overflow.
> 
Nice points.  Thanks, both of you.

Regards,
-Gonglei