On Thu, Mar 24, 2022 at 11:04:50PM +0900, Suwan Kim wrote:
> @@ -367,6 +381,66 @@ static blk_status_t virtio_queue_rq(struct blk_mq_hw_ctx *hctx,
> return BLK_STS_OK;
> }
>
> +static bool virtblk_prep_rq_batch(struct virtio_blk_vq *vq, struct request *req)
> +{
> + struct virtio_blk *vblk = req->mq_hctx->queue->queuedata;
> + struct virtblk_req *vbr = blk_mq_rq_to_pdu(req);
> + unsigned long flags;
> + int num, err;
> +
> + req->mq_hctx->tags->rqs[req->tag] = req;
> +
> + if (virtblk_prep_rq(req->mq_hctx, vblk, req, vbr, &num) != BLK_STS_OK)
> + return false;
> +
> + spin_lock_irqsave(&vq->lock, flags);
> + err = virtblk_add_req(vq->vq, vbr, vbr->sg_table.sgl, num);
> + if (err) {
> + spin_unlock_irqrestore(&vq->lock, flags);
> + virtblk_unmap_data(req, vbr);
> + virtblk_cleanup_cmd(req);
> + return false;
> + }
> + spin_unlock_irqrestore(&vq->lock, flags);
Simplification:
spin_lock_irqsave(&vq->lock, flags);
err = virtblk_add_req(vq->vq, vbr, vbr->sg_table.sgl, num);
spin_unlock_irqrestore(&vq->lock, flags);
if (err) {
virtblk_unmap_data(req, vbr);
virtblk_cleanup_cmd(req);
return false;
}
> +
> + return true;
> +}
> +
> +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.
> + kick = virtqueue_kick_prepare(vq->vq);
> + spin_unlock_irqrestore(&vq->lock, flags);
> + if (kick)
> + virtqueue_notify(vq->vq);
> +
> + req->rq_next = NULL;
> + *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.
Attachment:
signature.asc
Description: PGP signature
