Re: [Qemu-devel] [RFC][PATCH v2] suppressing queue notification with queue depth parameter (was: [RFC][PATCH] Queue notify support for virtio block device.)

[Anthony Liguori <anthony@codemonkey.ws>]

On 01/14/2010 04:33 AM, Vadim Rozenfeld wrote:
> The following patch allows to suppress virtio queue notification with
> the number of requests passed as parameter.
>
> Changes from v1:
> - code styling,
> - parameter "x-queue-depth-suppress-notify" for queue depth adjustment.
>
> repository: /home/vadimr/work/upstream/qemu
> branch: master
> commit d23a1c2fbd23e3da9a671a35c95324d2c630e4c9
> Author: Vadim Rozenfeld<vrozenfe@redhat.com>
> Date:   Thu Jan 14 08:09:26 2010 +0200
>
>      [RFC][PATCH v2] suppressing queue notification with queue depth
> parameter
>
> diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
> index cb1ae1f..93b584d 100644
> --- a/hw/virtio-blk.c
> +++ b/hw/virtio-blk.c
> @@ -28,6 +28,8 @@ typedef struct VirtIOBlock
>       char serial_str[BLOCK_SERIAL_STRLEN + 1];
>       QEMUBH *bh;
>       size_t config_size;
> +    unsigned int queue_depth;
> +    unsigned int requests;
>   } VirtIOBlock;
>
>   static VirtIOBlock *to_virtio_blk(VirtIODevice *vdev)
> @@ -87,6 +89,8 @@ typedef struct VirtIOBlockReq
>       struct VirtIOBlockReq *next;
>   } VirtIOBlockReq;
>
> +static void virtio_blk_handle_output(VirtIODevice *vdev, VirtQueue
> *vq);
> +
>   static void virtio_blk_req_complete(VirtIOBlockReq *req, int status)
>   {
>       VirtIOBlock *s = req->dev;
> @@ -95,6 +99,11 @@ static void virtio_blk_req_complete(VirtIOBlockReq
> *req, int status)
>       virtqueue_push(s->vq,&req->elem, req->qiov.size +
> sizeof(*req->in));
>       virtio_notify(&s->vdev, s->vq);
>
> +    virtio_blk_handle_output(&s->vdev, s->vq);
> +    if (--s->requests == (s->queue_depth - 1)) {
> +        virtio_queue_set_notification(s->vq, 1);
> +    }
> +
>       qemu_free(req);
>   }
>
> @@ -340,6 +349,10 @@ static void virtio_blk_handle_output(VirtIODevice
> *vdev, VirtQueue *vq)
>               exit(1);
>           }
>
> +        if (++s->requests == s->queue_depth) {
> +            virtio_queue_set_notification(s->vq, 0);
> +        }
> +
>           req->out = (void *)req->elem.out_sg[0].iov_base;
>           req->in = (void *)req->elem.in_sg[req->elem.in_num -
> 1].iov_base;
>
> @@ -502,6 +515,7 @@ VirtIODevice *virtio_blk_init(DeviceState *dev,
> DriveInfo *dinfo)
>       s->vdev.reset = virtio_blk_reset;
>       s->bs = dinfo->bdrv;
>       s->rq = NULL;
> +    s->queue_depth = drive_get_queue_depth(dinfo->bdrv);
>       if (strlen(ps))
>           strncpy(s->serial_str, ps, sizeof(s->serial_str));
>       else
> diff --git a/qemu-config.c b/qemu-config.c
> index c3203c8..6fcf958 100644
> --- a/qemu-config.c
> +++ b/qemu-config.c
> @@ -78,6 +78,10 @@ QemuOptsList qemu_drive_opts = {
>           },{
>               .name = "readonly",
>               .type = QEMU_OPT_BOOL,
> +        },{
> +            .name = "x-queue-depth-suppress-notify",
> +            .type = QEMU_OPT_NUMBER,
> +            .help = "number of requests in queueu before suppressing
> notify",
>           },
>           { /* end if list */ }
>       },
> diff --git a/sysemu.h b/sysemu.h
> index 9d80bb2..6eecbe1 100644
> --- a/sysemu.h
> +++ b/sysemu.h
> @@ -183,6 +183,7 @@ typedef struct DriveInfo {
>       BlockInterfaceErrorAction on_read_error;
>       BlockInterfaceErrorAction on_write_error;
>       char serial[BLOCK_SERIAL_STRLEN + 1];
> +    int queue_depth;
>       QTAILQ_ENTRY(DriveInfo) next;
>   } DriveInfo;
>
> @@ -198,6 +199,7 @@ extern DriveInfo *drive_get_by_id(const char *id);
>   extern int drive_get_max_bus(BlockInterfaceType type);
>   extern void drive_uninit(DriveInfo *dinfo);
>   extern const char *drive_get_serial(BlockDriverState *bdrv);
> +extern int drive_get_queue_depth(BlockDriverState *bdrv);
>
>   extern BlockInterfaceErrorAction drive_get_on_error(
>       BlockDriverState *bdrv, int is_read);
> diff --git a/vl.c b/vl.c
> index b048e89..517d290 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2043,6 +2043,18 @@ const char *drive_get_serial(BlockDriverState
> *bdrv)
>       return "\0";
>   }
>
> +int drive_get_queue_depth(BlockDriverState *bdrv)
> +{
> +    DriveInfo *dinfo;
> +
> +    QTAILQ_FOREACH(dinfo,&drives, next) {
> +        if (dinfo->bdrv == bdrv)
> +            return dinfo->queue_depth;
> +    }
> +
> +    return 1;
> +}
> +
>   BlockInterfaceErrorAction drive_get_on_error(
>       BlockDriverState *bdrv, int is_read)
>   {
> @@ -2110,6 +2122,7 @@ DriveInfo *drive_init(QemuOpts *opts, void
> *opaque,
>       const char *devaddr;
>       DriveInfo *dinfo;
>       int snapshot = 0;
> +    int queue_depth = 1;
>    

It should be disabled by default.  queue_depth=1 by default is going to 
be devastating for multi-spindle set-ups.

Regards,

Anthony Liguori