qemu-block
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Stefan Hajnoczi
Subject: Re: [Qemu-block] [PATCH v3] virtio-blk: trivial code optimization
Date: Tue, 10 Nov 2015 09:56:16 +0000
User-agent: Mutt/1.5.23 (2015-06-09)

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, 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,

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: signature.asc
Description: PGP signature


reply via email to

[Prev in Thread] Current Thread [Next in Thread]