* [PATCH 1/2] virtio-blk: set req->state to MQ_RQ_COMPLETE after polling I/O is finished
@ 2022-12-06 14:11 Suwan Kim
2022-12-06 14:11 ` [PATCH 2/2] virtio-blk: support completion batching for the IRQ path Suwan Kim
2022-12-07 21:47 ` Stefan Hajnoczi
0 siblings, 2 replies; 12+ messages in thread
From: Suwan Kim @ 2022-12-06 14:11 UTC (permalink / raw)
To: mst, jasowang, stefanha, pbonzini, hch, axboe
Cc: virtualization, linux-block, Suwan Kim
Driver should set req->state to MQ_RQ_COMPLETE after it finishes to process
req. But virtio-blk doesn't set MQ_RQ_COMPLETE after virtblk_poll() handles
req and req->state still remains MQ_RQ_IN_FLIGHT. Fortunately so far there
is no issue about it because blk_mq_end_request_batch() sets req->state to
MQ_RQ_IDLE. This patch properly sets req->state after polling I/O is finished.
Fixes: 4e0400525691 ("virtio-blk: support polling I/O")
Signed-off-by: Suwan Kim <suwan.kim027@gmail.com>
---
drivers/block/virtio_blk.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index 19da5defd734..cf64d256787e 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -839,6 +839,7 @@ static void virtblk_complete_batch(struct io_comp_batch *iob)
rq_list_for_each(&iob->req_list, req) {
virtblk_unmap_data(req, blk_mq_rq_to_pdu(req));
virtblk_cleanup_cmd(req);
+ blk_mq_set_request_complete(req);
}
blk_mq_end_request_batch(iob);
}
--
2.26.3
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 2/2] virtio-blk: support completion batching for the IRQ path
2022-12-06 14:11 [PATCH 1/2] virtio-blk: set req->state to MQ_RQ_COMPLETE after polling I/O is finished Suwan Kim
@ 2022-12-06 14:11 ` Suwan Kim
2022-12-07 22:05 ` Stefan Hajnoczi
2022-12-07 21:47 ` Stefan Hajnoczi
1 sibling, 1 reply; 12+ messages in thread
From: Suwan Kim @ 2022-12-06 14:11 UTC (permalink / raw)
To: mst, jasowang, stefanha, pbonzini, hch, axboe
Cc: virtualization, linux-block, Suwan Kim
This patch adds completion batching to the IRQ path. It reuses batch
completion code of virtblk_poll(). It collects requests to io_comp_batch
and processes them all at once. It can boost up the performance by 2%.
To validate the performance improvement and stabilty, I did fio test with
4 vCPU VM and 12 vCPU VM respectively. Both VMs have 8GB ram and the same
number of HW queues as vCPU.
The fio cammad is as follows and I ran the fio 5 times and got IOPS average.
(io_uring, randread, direct=1, bs=512, iodepth=64 numjobs=2,4)
Test result shows about 2% improvement.
4 vcpu VM | numjobs=2 | numjobs=4
-----------------------------------------------------------
fio without patch | 367.2K IOPS | 397.6K IOPS
-----------------------------------------------------------
fio with patch | 372.8K IOPS | 407.7K IOPS
12 vcpu VM | numjobs=2 | numjobs=4
-----------------------------------------------------------
fio without patch | 363.6K IOPS | 374.8K IOPS
-----------------------------------------------------------
fio with patch | 373.8K IOPS | 385.3K IOPS
Signed-off-by: Suwan Kim <suwan.kim027@gmail.com>
---
drivers/block/virtio_blk.c | 38 +++++++++++++++++++++++---------------
1 file changed, 23 insertions(+), 15 deletions(-)
diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index cf64d256787e..48fcf745f007 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -272,6 +272,18 @@ static inline void virtblk_request_done(struct request *req)
blk_mq_end_request(req, virtblk_result(vbr));
}
+static void virtblk_complete_batch(struct io_comp_batch *iob)
+{
+ struct request *req;
+
+ rq_list_for_each(&iob->req_list, req) {
+ virtblk_unmap_data(req, blk_mq_rq_to_pdu(req));
+ virtblk_cleanup_cmd(req);
+ blk_mq_set_request_complete(req);
+ }
+ blk_mq_end_request_batch(iob);
+}
+
static void virtblk_done(struct virtqueue *vq)
{
struct virtio_blk *vblk = vq->vdev->priv;
@@ -280,6 +292,7 @@ static void virtblk_done(struct virtqueue *vq)
struct virtblk_req *vbr;
unsigned long flags;
unsigned int len;
+ DEFINE_IO_COMP_BATCH(iob);
spin_lock_irqsave(&vblk->vqs[qid].lock, flags);
do {
@@ -287,7 +300,9 @@ static void virtblk_done(struct virtqueue *vq)
while ((vbr = virtqueue_get_buf(vblk->vqs[qid].vq, &len)) != NULL) {
struct request *req = blk_mq_rq_from_pdu(vbr);
- if (likely(!blk_should_fake_timeout(req->q)))
+ if (likely(!blk_should_fake_timeout(req->q)) &&
+ !blk_mq_add_to_batch(req, &iob, vbr->status,
+ virtblk_complete_batch))
blk_mq_complete_request(req);
req_done = true;
}
@@ -295,9 +310,14 @@ static void virtblk_done(struct virtqueue *vq)
break;
} while (!virtqueue_enable_cb(vq));
- /* In case queue is stopped waiting for more buffers. */
- if (req_done)
+ if (req_done) {
+ if (!rq_list_empty(iob.req_list))
+ virtblk_complete_batch(&iob);
+
+ /* In case queue is stopped waiting for more buffers. */
blk_mq_start_stopped_hw_queues(vblk->disk->queue, true);
+ }
+
spin_unlock_irqrestore(&vblk->vqs[qid].lock, flags);
}
@@ -832,18 +852,6 @@ static void virtblk_map_queues(struct blk_mq_tag_set *set)
}
}
-static void virtblk_complete_batch(struct io_comp_batch *iob)
-{
- struct request *req;
-
- rq_list_for_each(&iob->req_list, req) {
- virtblk_unmap_data(req, blk_mq_rq_to_pdu(req));
- virtblk_cleanup_cmd(req);
- blk_mq_set_request_complete(req);
- }
- blk_mq_end_request_batch(iob);
-}
-
static int virtblk_poll(struct blk_mq_hw_ctx *hctx, struct io_comp_batch *iob)
{
struct virtio_blk *vblk = hctx->queue->queuedata;
--
2.26.3
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] virtio-blk: set req->state to MQ_RQ_COMPLETE after polling I/O is finished
2022-12-06 14:11 [PATCH 1/2] virtio-blk: set req->state to MQ_RQ_COMPLETE after polling I/O is finished Suwan Kim
@ 2022-12-07 21:47 ` Stefan Hajnoczi
2022-12-07 21:47 ` Stefan Hajnoczi
1 sibling, 0 replies; 12+ messages in thread
From: Stefan Hajnoczi @ 2022-12-07 21:47 UTC (permalink / raw)
To: axboe; +Cc: linux-block, mst, virtualization, hch, pbonzini, Suwan Kim
[-- Attachment #1.1: Type: text/plain, Size: 1734 bytes --]
On Tue, Dec 06, 2022 at 11:11:24PM +0900, Suwan Kim wrote:
> Driver should set req->state to MQ_RQ_COMPLETE after it finishes to process
> req. But virtio-blk doesn't set MQ_RQ_COMPLETE after virtblk_poll() handles
> req and req->state still remains MQ_RQ_IN_FLIGHT. Fortunately so far there
> is no issue about it because blk_mq_end_request_batch() sets req->state to
> MQ_RQ_IDLE. This patch properly sets req->state after polling I/O is finished.
>
> Fixes: 4e0400525691 ("virtio-blk: support polling I/O")
> Signed-off-by: Suwan Kim <suwan.kim027@gmail.com>
> ---
> drivers/block/virtio_blk.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> index 19da5defd734..cf64d256787e 100644
> --- a/drivers/block/virtio_blk.c
> +++ b/drivers/block/virtio_blk.c
> @@ -839,6 +839,7 @@ static void virtblk_complete_batch(struct io_comp_batch *iob)
> rq_list_for_each(&iob->req_list, req) {
> virtblk_unmap_data(req, blk_mq_rq_to_pdu(req));
> virtblk_cleanup_cmd(req);
> + blk_mq_set_request_complete(req);
> }
> blk_mq_end_request_batch(iob);
> }
The doc comment for blk_mq_set_request_complete() mentions this being
used in ->queue_rq(), but that's not the case here. Does the doc comment
need to be updated if we're using the function in a different way?
I'm not familiar enough with the Linux block APIs, but this feels weird
to me. Shouldn't blk_mq_end_request_batch(iob) take care of this for us?
Why does it set the state to IDLE instead of COMPLETE?
I think Jens can confirm whether we really want all drivers that use
polling and io_comp_batch to manually call
blk_mq_set_request_complete().
Thanks,
Stefan
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
[-- Attachment #2: Type: text/plain, Size: 183 bytes --]
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] virtio-blk: set req->state to MQ_RQ_COMPLETE after polling I/O is finished
@ 2022-12-07 21:47 ` Stefan Hajnoczi
0 siblings, 0 replies; 12+ messages in thread
From: Stefan Hajnoczi @ 2022-12-07 21:47 UTC (permalink / raw)
To: axboe
Cc: mst, jasowang, pbonzini, hch, virtualization, linux-block, Suwan Kim
[-- Attachment #1: Type: text/plain, Size: 1734 bytes --]
On Tue, Dec 06, 2022 at 11:11:24PM +0900, Suwan Kim wrote:
> Driver should set req->state to MQ_RQ_COMPLETE after it finishes to process
> req. But virtio-blk doesn't set MQ_RQ_COMPLETE after virtblk_poll() handles
> req and req->state still remains MQ_RQ_IN_FLIGHT. Fortunately so far there
> is no issue about it because blk_mq_end_request_batch() sets req->state to
> MQ_RQ_IDLE. This patch properly sets req->state after polling I/O is finished.
>
> Fixes: 4e0400525691 ("virtio-blk: support polling I/O")
> Signed-off-by: Suwan Kim <suwan.kim027@gmail.com>
> ---
> drivers/block/virtio_blk.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> index 19da5defd734..cf64d256787e 100644
> --- a/drivers/block/virtio_blk.c
> +++ b/drivers/block/virtio_blk.c
> @@ -839,6 +839,7 @@ static void virtblk_complete_batch(struct io_comp_batch *iob)
> rq_list_for_each(&iob->req_list, req) {
> virtblk_unmap_data(req, blk_mq_rq_to_pdu(req));
> virtblk_cleanup_cmd(req);
> + blk_mq_set_request_complete(req);
> }
> blk_mq_end_request_batch(iob);
> }
The doc comment for blk_mq_set_request_complete() mentions this being
used in ->queue_rq(), but that's not the case here. Does the doc comment
need to be updated if we're using the function in a different way?
I'm not familiar enough with the Linux block APIs, but this feels weird
to me. Shouldn't blk_mq_end_request_batch(iob) take care of this for us?
Why does it set the state to IDLE instead of COMPLETE?
I think Jens can confirm whether we really want all drivers that use
polling and io_comp_batch to manually call
blk_mq_set_request_complete().
Thanks,
Stefan
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] virtio-blk: support completion batching for the IRQ path
2022-12-06 14:11 ` [PATCH 2/2] virtio-blk: support completion batching for the IRQ path Suwan Kim
@ 2022-12-07 22:05 ` Stefan Hajnoczi
0 siblings, 0 replies; 12+ messages in thread
From: Stefan Hajnoczi @ 2022-12-07 22:05 UTC (permalink / raw)
To: Suwan Kim; +Cc: axboe, linux-block, mst, virtualization, hch, pbonzini
[-- Attachment #1.1: Type: text/plain, Size: 3594 bytes --]
On Tue, Dec 06, 2022 at 11:11:25PM +0900, Suwan Kim wrote:
> This patch adds completion batching to the IRQ path. It reuses batch
> completion code of virtblk_poll(). It collects requests to io_comp_batch
> and processes them all at once. It can boost up the performance by 2%.
>
> To validate the performance improvement and stabilty, I did fio test with
> 4 vCPU VM and 12 vCPU VM respectively. Both VMs have 8GB ram and the same
> number of HW queues as vCPU.
> The fio cammad is as follows and I ran the fio 5 times and got IOPS average.
> (io_uring, randread, direct=1, bs=512, iodepth=64 numjobs=2,4)
>
> Test result shows about 2% improvement.
>
> 4 vcpu VM | numjobs=2 | numjobs=4
> -----------------------------------------------------------
> fio without patch | 367.2K IOPS | 397.6K IOPS
> -----------------------------------------------------------
> fio with patch | 372.8K IOPS | 407.7K IOPS
>
> 12 vcpu VM | numjobs=2 | numjobs=4
> -----------------------------------------------------------
> fio without patch | 363.6K IOPS | 374.8K IOPS
> -----------------------------------------------------------
> fio with patch | 373.8K IOPS | 385.3K IOPS
>
> Signed-off-by: Suwan Kim <suwan.kim027@gmail.com>
> ---
> drivers/block/virtio_blk.c | 38 +++++++++++++++++++++++---------------
> 1 file changed, 23 insertions(+), 15 deletions(-)
Cool, thanks for doing this!
> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> index cf64d256787e..48fcf745f007 100644
> --- a/drivers/block/virtio_blk.c
> +++ b/drivers/block/virtio_blk.c
> @@ -272,6 +272,18 @@ static inline void virtblk_request_done(struct request *req)
> blk_mq_end_request(req, virtblk_result(vbr));
> }
>
> +static void virtblk_complete_batch(struct io_comp_batch *iob)
> +{
> + struct request *req;
> +
> + rq_list_for_each(&iob->req_list, req) {
> + virtblk_unmap_data(req, blk_mq_rq_to_pdu(req));
> + virtblk_cleanup_cmd(req);
> + blk_mq_set_request_complete(req);
> + }
> + blk_mq_end_request_batch(iob);
> +}
> +
> static void virtblk_done(struct virtqueue *vq)
> {
> struct virtio_blk *vblk = vq->vdev->priv;
> @@ -280,6 +292,7 @@ static void virtblk_done(struct virtqueue *vq)
> struct virtblk_req *vbr;
> unsigned long flags;
> unsigned int len;
> + DEFINE_IO_COMP_BATCH(iob);
>
> spin_lock_irqsave(&vblk->vqs[qid].lock, flags);
> do {
> @@ -287,7 +300,9 @@ static void virtblk_done(struct virtqueue *vq)
> while ((vbr = virtqueue_get_buf(vblk->vqs[qid].vq, &len)) != NULL) {
> struct request *req = blk_mq_rq_from_pdu(vbr);
>
> - if (likely(!blk_should_fake_timeout(req->q)))
> + if (likely(!blk_should_fake_timeout(req->q)) &&
> + !blk_mq_add_to_batch(req, &iob, vbr->status,
> + virtblk_complete_batch))
> blk_mq_complete_request(req);
> req_done = true;
> }
> @@ -295,9 +310,14 @@ static void virtblk_done(struct virtqueue *vq)
> break;
> } while (!virtqueue_enable_cb(vq));
>
> - /* In case queue is stopped waiting for more buffers. */
> - if (req_done)
> + if (req_done) {
> + if (!rq_list_empty(iob.req_list))
> + virtblk_complete_batch(&iob);
A little optimization to avoid the indirect call: iob.complete(&iob) :).
Not sure if it's good style to do that but it works in this case because
we know it can only be virtblk_complete_batch().
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
[-- Attachment #2: Type: text/plain, Size: 183 bytes --]
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] virtio-blk: support completion batching for the IRQ path
@ 2022-12-07 22:05 ` Stefan Hajnoczi
0 siblings, 0 replies; 12+ messages in thread
From: Stefan Hajnoczi @ 2022-12-07 22:05 UTC (permalink / raw)
To: Suwan Kim
Cc: mst, jasowang, pbonzini, hch, axboe, virtualization, linux-block
[-- Attachment #1: Type: text/plain, Size: 3594 bytes --]
On Tue, Dec 06, 2022 at 11:11:25PM +0900, Suwan Kim wrote:
> This patch adds completion batching to the IRQ path. It reuses batch
> completion code of virtblk_poll(). It collects requests to io_comp_batch
> and processes them all at once. It can boost up the performance by 2%.
>
> To validate the performance improvement and stabilty, I did fio test with
> 4 vCPU VM and 12 vCPU VM respectively. Both VMs have 8GB ram and the same
> number of HW queues as vCPU.
> The fio cammad is as follows and I ran the fio 5 times and got IOPS average.
> (io_uring, randread, direct=1, bs=512, iodepth=64 numjobs=2,4)
>
> Test result shows about 2% improvement.
>
> 4 vcpu VM | numjobs=2 | numjobs=4
> -----------------------------------------------------------
> fio without patch | 367.2K IOPS | 397.6K IOPS
> -----------------------------------------------------------
> fio with patch | 372.8K IOPS | 407.7K IOPS
>
> 12 vcpu VM | numjobs=2 | numjobs=4
> -----------------------------------------------------------
> fio without patch | 363.6K IOPS | 374.8K IOPS
> -----------------------------------------------------------
> fio with patch | 373.8K IOPS | 385.3K IOPS
>
> Signed-off-by: Suwan Kim <suwan.kim027@gmail.com>
> ---
> drivers/block/virtio_blk.c | 38 +++++++++++++++++++++++---------------
> 1 file changed, 23 insertions(+), 15 deletions(-)
Cool, thanks for doing this!
> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> index cf64d256787e..48fcf745f007 100644
> --- a/drivers/block/virtio_blk.c
> +++ b/drivers/block/virtio_blk.c
> @@ -272,6 +272,18 @@ static inline void virtblk_request_done(struct request *req)
> blk_mq_end_request(req, virtblk_result(vbr));
> }
>
> +static void virtblk_complete_batch(struct io_comp_batch *iob)
> +{
> + struct request *req;
> +
> + rq_list_for_each(&iob->req_list, req) {
> + virtblk_unmap_data(req, blk_mq_rq_to_pdu(req));
> + virtblk_cleanup_cmd(req);
> + blk_mq_set_request_complete(req);
> + }
> + blk_mq_end_request_batch(iob);
> +}
> +
> static void virtblk_done(struct virtqueue *vq)
> {
> struct virtio_blk *vblk = vq->vdev->priv;
> @@ -280,6 +292,7 @@ static void virtblk_done(struct virtqueue *vq)
> struct virtblk_req *vbr;
> unsigned long flags;
> unsigned int len;
> + DEFINE_IO_COMP_BATCH(iob);
>
> spin_lock_irqsave(&vblk->vqs[qid].lock, flags);
> do {
> @@ -287,7 +300,9 @@ static void virtblk_done(struct virtqueue *vq)
> while ((vbr = virtqueue_get_buf(vblk->vqs[qid].vq, &len)) != NULL) {
> struct request *req = blk_mq_rq_from_pdu(vbr);
>
> - if (likely(!blk_should_fake_timeout(req->q)))
> + if (likely(!blk_should_fake_timeout(req->q)) &&
> + !blk_mq_add_to_batch(req, &iob, vbr->status,
> + virtblk_complete_batch))
> blk_mq_complete_request(req);
> req_done = true;
> }
> @@ -295,9 +310,14 @@ static void virtblk_done(struct virtqueue *vq)
> break;
> } while (!virtqueue_enable_cb(vq));
>
> - /* In case queue is stopped waiting for more buffers. */
> - if (req_done)
> + if (req_done) {
> + if (!rq_list_empty(iob.req_list))
> + virtblk_complete_batch(&iob);
A little optimization to avoid the indirect call: iob.complete(&iob) :).
Not sure if it's good style to do that but it works in this case because
we know it can only be virtblk_complete_batch().
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] virtio-blk: set req->state to MQ_RQ_COMPLETE after polling I/O is finished
2022-12-07 21:47 ` Stefan Hajnoczi
@ 2022-12-08 16:48 ` Jens Axboe
-1 siblings, 0 replies; 12+ messages in thread
From: Jens Axboe @ 2022-12-08 16:48 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: mst, jasowang, pbonzini, hch, virtualization, linux-block, Suwan Kim
On 12/7/22 2:47 PM, Stefan Hajnoczi wrote:
> On Tue, Dec 06, 2022 at 11:11:24PM +0900, Suwan Kim wrote:
>> Driver should set req->state to MQ_RQ_COMPLETE after it finishes to process
>> req. But virtio-blk doesn't set MQ_RQ_COMPLETE after virtblk_poll() handles
>> req and req->state still remains MQ_RQ_IN_FLIGHT. Fortunately so far there
>> is no issue about it because blk_mq_end_request_batch() sets req->state to
>> MQ_RQ_IDLE. This patch properly sets req->state after polling I/O is finished.
>>
>> Fixes: 4e0400525691 ("virtio-blk: support polling I/O")
>> Signed-off-by: Suwan Kim <suwan.kim027@gmail.com>
>> ---
>> drivers/block/virtio_blk.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
>> index 19da5defd734..cf64d256787e 100644
>> --- a/drivers/block/virtio_blk.c
>> +++ b/drivers/block/virtio_blk.c
>> @@ -839,6 +839,7 @@ static void virtblk_complete_batch(struct io_comp_batch *iob)
>> rq_list_for_each(&iob->req_list, req) {
>> virtblk_unmap_data(req, blk_mq_rq_to_pdu(req));
>> virtblk_cleanup_cmd(req);
>> + blk_mq_set_request_complete(req);
>> }
>> blk_mq_end_request_batch(iob);
>> }
>
> The doc comment for blk_mq_set_request_complete() mentions this being
> used in ->queue_rq(), but that's not the case here. Does the doc comment
> need to be updated if we're using the function in a different way?
Looks like it's a bit outdated...
> I'm not familiar enough with the Linux block APIs, but this feels weird
> to me. Shouldn't blk_mq_end_request_batch(iob) take care of this for us?
> Why does it set the state to IDLE instead of COMPLETE?
>
> I think Jens can confirm whether we really want all drivers that use
> polling and io_comp_batch to manually call
> blk_mq_set_request_complete().
Should not be a need to call blk_mq_set_request_complete() directly in
the driver for this.
--
Jens Axboe
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] virtio-blk: set req->state to MQ_RQ_COMPLETE after polling I/O is finished
@ 2022-12-08 16:48 ` Jens Axboe
0 siblings, 0 replies; 12+ messages in thread
From: Jens Axboe @ 2022-12-08 16:48 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: linux-block, mst, virtualization, hch, pbonzini, Suwan Kim
On 12/7/22 2:47 PM, Stefan Hajnoczi wrote:
> On Tue, Dec 06, 2022 at 11:11:24PM +0900, Suwan Kim wrote:
>> Driver should set req->state to MQ_RQ_COMPLETE after it finishes to process
>> req. But virtio-blk doesn't set MQ_RQ_COMPLETE after virtblk_poll() handles
>> req and req->state still remains MQ_RQ_IN_FLIGHT. Fortunately so far there
>> is no issue about it because blk_mq_end_request_batch() sets req->state to
>> MQ_RQ_IDLE. This patch properly sets req->state after polling I/O is finished.
>>
>> Fixes: 4e0400525691 ("virtio-blk: support polling I/O")
>> Signed-off-by: Suwan Kim <suwan.kim027@gmail.com>
>> ---
>> drivers/block/virtio_blk.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
>> index 19da5defd734..cf64d256787e 100644
>> --- a/drivers/block/virtio_blk.c
>> +++ b/drivers/block/virtio_blk.c
>> @@ -839,6 +839,7 @@ static void virtblk_complete_batch(struct io_comp_batch *iob)
>> rq_list_for_each(&iob->req_list, req) {
>> virtblk_unmap_data(req, blk_mq_rq_to_pdu(req));
>> virtblk_cleanup_cmd(req);
>> + blk_mq_set_request_complete(req);
>> }
>> blk_mq_end_request_batch(iob);
>> }
>
> The doc comment for blk_mq_set_request_complete() mentions this being
> used in ->queue_rq(), but that's not the case here. Does the doc comment
> need to be updated if we're using the function in a different way?
Looks like it's a bit outdated...
> I'm not familiar enough with the Linux block APIs, but this feels weird
> to me. Shouldn't blk_mq_end_request_batch(iob) take care of this for us?
> Why does it set the state to IDLE instead of COMPLETE?
>
> I think Jens can confirm whether we really want all drivers that use
> polling and io_comp_batch to manually call
> blk_mq_set_request_complete().
Should not be a need to call blk_mq_set_request_complete() directly in
the driver for this.
--
Jens Axboe
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] virtio-blk: set req->state to MQ_RQ_COMPLETE after polling I/O is finished
2022-12-08 16:48 ` Jens Axboe
@ 2022-12-12 6:50 ` Christoph Hellwig
-1 siblings, 0 replies; 12+ messages in thread
From: Christoph Hellwig @ 2022-12-12 6:50 UTC (permalink / raw)
To: Jens Axboe
Cc: Stefan Hajnoczi, mst, jasowang, pbonzini, hch, virtualization,
linux-block, Suwan Kim
On Thu, Dec 08, 2022 at 09:48:23AM -0700, Jens Axboe wrote:
> > The doc comment for blk_mq_set_request_complete() mentions this being
> > used in ->queue_rq(), but that's not the case here. Does the doc comment
> > need to be updated if we're using the function in a different way?
>
> Looks like it's a bit outdated...
I think the comment is still entirely correct.
>
> > I'm not familiar enough with the Linux block APIs, but this feels weird
> > to me. Shouldn't blk_mq_end_request_batch(iob) take care of this for us?
> > Why does it set the state to IDLE instead of COMPLETE?
> >
> > I think Jens can confirm whether we really want all drivers that use
> > polling and io_comp_batch to manually call
> > blk_mq_set_request_complete().
>
> Should not be a need to call blk_mq_set_request_complete() directly in
> the driver for this.
Exactly. Polling or not, drivers should go through the normal completion
interface, that is blk_mq_complete_request or the lower-level options
blk_mq_complete_request_remote and blk_mq_complete_request_direct.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] virtio-blk: set req->state to MQ_RQ_COMPLETE after polling I/O is finished
@ 2022-12-12 6:50 ` Christoph Hellwig
0 siblings, 0 replies; 12+ messages in thread
From: Christoph Hellwig @ 2022-12-12 6:50 UTC (permalink / raw)
To: Jens Axboe
Cc: linux-block, mst, virtualization, hch, Stefan Hajnoczi, pbonzini,
Suwan Kim
On Thu, Dec 08, 2022 at 09:48:23AM -0700, Jens Axboe wrote:
> > The doc comment for blk_mq_set_request_complete() mentions this being
> > used in ->queue_rq(), but that's not the case here. Does the doc comment
> > need to be updated if we're using the function in a different way?
>
> Looks like it's a bit outdated...
I think the comment is still entirely correct.
>
> > I'm not familiar enough with the Linux block APIs, but this feels weird
> > to me. Shouldn't blk_mq_end_request_batch(iob) take care of this for us?
> > Why does it set the state to IDLE instead of COMPLETE?
> >
> > I think Jens can confirm whether we really want all drivers that use
> > polling and io_comp_batch to manually call
> > blk_mq_set_request_complete().
>
> Should not be a need to call blk_mq_set_request_complete() directly in
> the driver for this.
Exactly. Polling or not, drivers should go through the normal completion
interface, that is blk_mq_complete_request or the lower-level options
blk_mq_complete_request_remote and blk_mq_complete_request_direct.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] virtio-blk: set req->state to MQ_RQ_COMPLETE after polling I/O is finished
2022-12-12 6:50 ` Christoph Hellwig
(?)
@ 2022-12-12 14:21 ` Suwan Kim
-1 siblings, 0 replies; 12+ messages in thread
From: Suwan Kim @ 2022-12-12 14:21 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Jens Axboe, Stefan Hajnoczi, mst, jasowang, pbonzini,
virtualization, linux-block
On Sun, Dec 11, 2022 at 10:50:03PM -0800, Christoph Hellwig wrote:
> On Thu, Dec 08, 2022 at 09:48:23AM -0700, Jens Axboe wrote:
> > > The doc comment for blk_mq_set_request_complete() mentions this being
> > > used in ->queue_rq(), but that's not the case here. Does the doc comment
> > > need to be updated if we're using the function in a different way?
> >
> > Looks like it's a bit outdated...
>
> I think the comment is still entirely correct.
>
> >
> > > I'm not familiar enough with the Linux block APIs, but this feels weird
> > > to me. Shouldn't blk_mq_end_request_batch(iob) take care of this for us?
> > > Why does it set the state to IDLE instead of COMPLETE?
> > >
> > > I think Jens can confirm whether we really want all drivers that use
> > > polling and io_comp_batch to manually call
> > > blk_mq_set_request_complete().
> >
> > Should not be a need to call blk_mq_set_request_complete() directly in
> > the driver for this.
>
> Exactly. Polling or not, drivers should go through the normal completion
> interface, that is blk_mq_complete_request or the lower-level options
> blk_mq_complete_request_remote and blk_mq_complete_request_direct.
Hi all,
Thanks for the comment. It was the wrong use of the function...
I will use blk_mq_complete_request_remote() instead of
blk_mq_set_request_complete() and send next version soon.
Regards,
Suwan Kim
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] virtio-blk: support completion batching for the IRQ path
2022-12-07 22:05 ` Stefan Hajnoczi
(?)
@ 2022-12-12 14:43 ` Suwan Kim
-1 siblings, 0 replies; 12+ messages in thread
From: Suwan Kim @ 2022-12-12 14:43 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: mst, jasowang, pbonzini, hch, axboe, virtualization, linux-block
On Wed, Dec 07, 2022 at 05:05:06PM -0500, Stefan Hajnoczi wrote:
> On Tue, Dec 06, 2022 at 11:11:25PM +0900, Suwan Kim wrote:
> > This patch adds completion batching to the IRQ path. It reuses batch
> > completion code of virtblk_poll(). It collects requests to io_comp_batch
> > and processes them all at once. It can boost up the performance by 2%.
> >
> > To validate the performance improvement and stabilty, I did fio test with
> > 4 vCPU VM and 12 vCPU VM respectively. Both VMs have 8GB ram and the same
> > number of HW queues as vCPU.
> > The fio cammad is as follows and I ran the fio 5 times and got IOPS average.
> > (io_uring, randread, direct=1, bs=512, iodepth=64 numjobs=2,4)
> >
> > Test result shows about 2% improvement.
> >
> > 4 vcpu VM | numjobs=2 | numjobs=4
> > -----------------------------------------------------------
> > fio without patch | 367.2K IOPS | 397.6K IOPS
> > -----------------------------------------------------------
> > fio with patch | 372.8K IOPS | 407.7K IOPS
> >
> > 12 vcpu VM | numjobs=2 | numjobs=4
> > -----------------------------------------------------------
> > fio without patch | 363.6K IOPS | 374.8K IOPS
> > -----------------------------------------------------------
> > fio with patch | 373.8K IOPS | 385.3K IOPS
> >
> > Signed-off-by: Suwan Kim <suwan.kim027@gmail.com>
> > ---
> > drivers/block/virtio_blk.c | 38 +++++++++++++++++++++++---------------
> > 1 file changed, 23 insertions(+), 15 deletions(-)
>
> Cool, thanks for doing this!
>
> > diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> > index cf64d256787e..48fcf745f007 100644
> > --- a/drivers/block/virtio_blk.c
> > +++ b/drivers/block/virtio_blk.c
> > @@ -272,6 +272,18 @@ static inline void virtblk_request_done(struct request *req)
> > blk_mq_end_request(req, virtblk_result(vbr));
> > }
> >
> > +static void virtblk_complete_batch(struct io_comp_batch *iob)
> > +{
> > + struct request *req;
> > +
> > + rq_list_for_each(&iob->req_list, req) {
> > + virtblk_unmap_data(req, blk_mq_rq_to_pdu(req));
> > + virtblk_cleanup_cmd(req);
> > + blk_mq_set_request_complete(req);
> > + }
> > + blk_mq_end_request_batch(iob);
> > +}
> > +
> > static void virtblk_done(struct virtqueue *vq)
> > {
> > struct virtio_blk *vblk = vq->vdev->priv;
> > @@ -280,6 +292,7 @@ static void virtblk_done(struct virtqueue *vq)
> > struct virtblk_req *vbr;
> > unsigned long flags;
> > unsigned int len;
> > + DEFINE_IO_COMP_BATCH(iob);
> >
> > spin_lock_irqsave(&vblk->vqs[qid].lock, flags);
> > do {
> > @@ -287,7 +300,9 @@ static void virtblk_done(struct virtqueue *vq)
> > while ((vbr = virtqueue_get_buf(vblk->vqs[qid].vq, &len)) != NULL) {
> > struct request *req = blk_mq_rq_from_pdu(vbr);
> >
> > - if (likely(!blk_should_fake_timeout(req->q)))
> > + if (likely(!blk_should_fake_timeout(req->q)) &&
> > + !blk_mq_add_to_batch(req, &iob, vbr->status,
> > + virtblk_complete_batch))
> > blk_mq_complete_request(req);
> > req_done = true;
> > }
> > @@ -295,9 +310,14 @@ static void virtblk_done(struct virtqueue *vq)
> > break;
> > } while (!virtqueue_enable_cb(vq));
> >
> > - /* In case queue is stopped waiting for more buffers. */
> > - if (req_done)
> > + if (req_done) {
> > + if (!rq_list_empty(iob.req_list))
> > + virtblk_complete_batch(&iob);
>
> A little optimization to avoid the indirect call: iob.complete(&iob) :).
> Not sure if it's good style to do that but it works in this case because
> we know it can only be virtblk_complete_batch().
>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Hi Stefan,
Thanks for the comment!
It also needs to use blk_mq_complete_request_remote() instead of
blk_mq_set_request_complete()
I will resend it with the modification of patch #1 and then please
review it again.
Regards,
Suwan Kim
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2022-12-12 14:44 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-12-06 14:11 [PATCH 1/2] virtio-blk: set req->state to MQ_RQ_COMPLETE after polling I/O is finished Suwan Kim
2022-12-06 14:11 ` [PATCH 2/2] virtio-blk: support completion batching for the IRQ path Suwan Kim
2022-12-07 22:05 ` Stefan Hajnoczi
2022-12-07 22:05 ` Stefan Hajnoczi
2022-12-12 14:43 ` Suwan Kim
2022-12-07 21:47 ` [PATCH 1/2] virtio-blk: set req->state to MQ_RQ_COMPLETE after polling I/O is finished Stefan Hajnoczi
2022-12-07 21:47 ` Stefan Hajnoczi
2022-12-08 16:48 ` Jens Axboe
2022-12-08 16:48 ` Jens Axboe
2022-12-12 6:50 ` Christoph Hellwig
2022-12-12 6:50 ` Christoph Hellwig
2022-12-12 14:21 ` Suwan Kim
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.