On Tue, Mar 29, 2022 at 09:45:29AM +0100, Stefan Hajnoczi wrote:

On Tue, Mar 29, 2022 at 12:50:33AM +0900, Suwan Kim wrote:

quoted

On Mon, Mar 28, 2022 at 02:16:13PM +0100, Stefan Hajnoczi wrote:

quoted

On Thu, Mar 24, 2022 at 11:04:50PM +0900, Suwan Kim wrote:

quoted

+static void virtio_queue_rqs(struct request **rqlist)
+{
+	struct request *req, *next, *prev = NULL;
+	struct request *requeue_list = NULL;
+
+	rq_list_for_each_safe(rqlist, req, next) {
+		struct virtio_blk_vq *vq = req->mq_hctx->driver_data;
+		unsigned long flags;
+		bool kick;
+
+		if (!virtblk_prep_rq_batch(vq, req)) {
+			rq_list_move(rqlist, &requeue_list, req, prev);
+			req = prev;
+
+			if (!req)
+				continue;
+		}
+
+		if (!next || req->mq_hctx != next->mq_hctx) {
+			spin_lock_irqsave(&vq->lock, flags);

Did you try calling virtblk_add_req() here to avoid acquiring and
releasing the lock multiple times? In other words, do virtblk_prep_rq()
but wait until we get here to do virtblk_add_req().

I don't know if it has any measurable effect on performance or maybe the
code would become too complex, but I noticed that we're not fully
exploiting batching.

I tried as you said. I called virtlblk_add_req() and added requests
of rqlist to virtqueue in this if statement with holding the lock
only once.

I attach the code at the end of this mail.
Please refer the code.

But I didn't see improvement. It showed slightly worse performance
than the current patch.

Okay, thanks for trying it!

quoted

quoted

quoted

+			kick = virtqueue_kick_prepare(vq->vq);
+			spin_unlock_irqrestore(&vq->lock, flags);
+			if (kick)
+				virtqueue_notify(vq->vq);
+
+			req->rq_next = NULL;

Did you ask this part?

quoted

quoted

+			*rqlist = next;
+			prev = NULL;
+		} else
+			prev = req;

What guarantees that req is still alive after we called
virtblk_add_req()? The device may have seen it and completed it already
by the time we get here.

Isn't request completed after the kick?

If you asked about "req->rq_next = NULL",
I think it should be placed before
"kick = virtqueue_kick_prepare(vq->vq);"

-----------
	req->rq_next = NULL;
	kick = virtqueue_kick_prepare(vq->vq);
	spin_unlock_irqrestore(&vq->lock, flags);
	if (kick)
		virtqueue_notify(vq->vq);
-----------

No, virtqueue_add_sgs() exposes vring descriptors to the device. The
device may process immediately. In other words, VIRTIO devices may poll
the vring instead of waiting for virtqueue_notify(). There is no
guarantee that the request is alive until virtqueue_notify() is called.

The code has to handle the case where the request is completed during
virtqueue_add_sgs().

Thanks for the explanation.

We should not use req again after virtblk_add_req().
I understand...

Then, as you commented in previous mail, is it ok that we do
virtblk_add_req() in "if (!next || req->mq_hctx != next->mq_hctx)"
statement to avoid use req again after virtblk_add_req() as below code?

In this code, It adds reqs to virtqueue in batch just before
virtqueue_notify(), and it doesn't use req again after calling
virtblk_add_req().

If it is fine, I will try it again.
This code is slightly different from the code I sent in previous mail.

---
static void virtio_queue_rqs(struct request **rqlist)
...
	rq_list_for_each_safe(rqlist, req, next) {
...
		if (!next || req->mq_hctx != next->mq_hctx) {
			// Cut the list at current req
			req->rq_next = NULL;
			// Add req list to virtqueue in batch with holding lock once
			kick = virtblk_add_req_batch(vq, rqlist, &requeue_list);
			if (kick)
				virtqueue_notify(vq->vq);

			// setup new req list. Don't use previous req again.
			*rqlist = next;
			prev = NULL;
...
---

Regards,
Suwan Kim