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

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

[arei.gonglei]

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

1. avoid possible superflous checking
2. make code more robustness

Signed-off-by: Gonglei <arei.gonglei@huawei.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
---
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) {
                 submit_requests(blk, mrb, start, num_reqs, niov);
                 num_reqs = 0;
             }
-- 
1.7.12.4

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

[Paolo Bonzini]

On 09/11/2015 10:03, arei.gonglei@huawei.com wrote:
> From: Gonglei <arei.gonglei@huawei.com>
> 
> 1. avoid possible superflous checking
> 2. make code more robustness
> 
> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> Reviewed-by: Fam Zheng <famz@redhat.com>
> ---
> 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) {
>                  submit_requests(blk, mrb, start, num_reqs, niov);
>                  num_reqs = 0;
>              }
> 

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

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

[Stefan Hajnoczi]

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

On Mon, Nov 09, 2015 at 05:03:30PM +0800, arei.gonglei@huawei.com wrote:
> From: Gonglei <arei.gonglei@huawei.com>
> 
> 1. avoid possible superflous checking
> 2. make code more robustness
> 
> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> Reviewed-by: Fam Zheng <famz@redhat.com>
> ---
> 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.

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.

Stefan

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

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, arei.gonglei@huawei.com wrote:
>> From: Gonglei <arei.gonglei@huawei.com>
>>
>> 1. avoid possible superflous checking
>> 2. make code more robustness
>>
>> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
>> Reviewed-by: Fam Zheng <famz@redhat.com>
>> ---
>> 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

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

[Paolo Bonzini]

On 10/11/2015 07:35, Gonglei wrote:
>> > 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,

No, the result will be unsigned long long, and the comparison is wrong
if max_xfer_len < req->qiov.size / BDRV_SECTOR_SIZE.

Paolo

> and nb_sectors is int too, so this comparison is right. Am I wrong?
> 

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

[Stefan Hajnoczi]

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

On Tue, Nov 10, 2015 at 02:35:19PM +0800, Gonglei wrote:
> On 2015/11/9 21:57, Stefan Hajnoczi wrote:
> > On Mon, Nov 09, 2015 at 05:03:30PM +0800, arei.gonglei@huawei.com wrote:
> >> From: Gonglei <arei.gonglei@huawei.com>
> >>
> >> 1. avoid possible superflous checking
> >> 2. make code more robustness
> >>
> >> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> >> Reviewed-by: Fam Zheng <famz@redhat.com>
> >> ---
> >> 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,

The type of "max_xfer_len - req->qiov.size / BDRV_SECTOR_SIZE" cannot be
int because you said req->qiov.size / BDRV_SECTOR_SIZE" is  unsigned
long long.

The C99 standard says:

6.3.1.1 Boolean, characters, and integers

- The rank of a signed integer type shall be greater than the rank of
any signed integer type with less precision.

...

- The rank of any unsigned integer type shall equal the rank of the
corresponding signed integer type, if any.

6.3.1.8 Usual arithmetic conversions

Otherwise, if the operand that has unsigned integer type has rank
greater or equal to the rank of the type of the other operand, then the
operand with signed integer type is converted to the type of the operand
with unsigned integer type.

So the max_xfer_len int operand must be converted to the higher ranking
unsigned long long.

Stefan

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

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

[Gonglei]

On 2015/11/10 17:56, Stefan Hajnoczi wrote:

> The C99 standard says:
> 
> 6.3.1.1 Boolean, characters, and integers
> 
> - The rank of a signed integer type shall be greater than the rank of
> any signed integer type with less precision.
> 
> ...
> 
> - The rank of any unsigned integer type shall equal the rank of the
> corresponding signed integer type, if any.
> 
> 6.3.1.8 Usual arithmetic conversions
> 
> Otherwise, if the operand that has unsigned integer type has rank
> greater or equal to the rank of the type of the other operand, then the
> operand with signed integer type is converted to the type of the operand
> with unsigned integer type.
> 
> So the max_xfer_len int operand must be converted to the higher ranking
> unsigned long long.
> 

Thank you so much, it's clear. I'll add a check before subtraction.
Please review v4.

Regards,
-Gonglei