virtio-blk: trivial code optimization

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(-)

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) {
      ...
  }

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

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