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
Attachment:
signature.asc
Description: PGP signature
