Stefan Hajnoczi
2022-Mar-29 08:45 UTC
[PATCH v3 2/2] virtio-blk: support mq_ops->queue_rqs()
On Tue, Mar 29, 2022 at 12:50:33AM +0900, Suwan Kim wrote:> On Mon, Mar 28, 2022 at 02:16:13PM +0100, Stefan Hajnoczi wrote: > > On Thu, Mar 24, 2022 at 11:04:50PM +0900, Suwan Kim wrote: > > > +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!> > > + 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? > > > > + *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(). Stefan -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 488 bytes Desc: not available URL: <http://lists.linuxfoundation.org/pipermail/virtualization/attachments/20220329/d2e870f3/attachment.sig>