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

[Gonglei]

On 2015/11/9 21:57, Stefan Hajnoczi wrote:
> On Mon, Nov 09, 2015 at 05:03:30PM +0800, address@hidden wrote:
>> From: Gonglei <address@hidden>
>>
>> 1. avoid possible superflous checking
>> 2. make code more robustness
>>
>> Signed-off-by: Gonglei <address@hidden>
>> Reviewed-by: Fam Zheng <address@hidden>
>> ---
>> 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