Re: [PATCH v3 2/2] virtio-blk: support mq_ops->queue_rqs()

[Suwan Kim <suwan.kim027@gmail.com>]

On Tue, Mar 29, 2022 at 04:01:46PM +0100, Stefan Hajnoczi wrote:
> On Tue, Mar 29, 2022 at 10:48:16PM +0900, Suwan Kim wrote:
> > 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:
> > > > 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().
> > 
> > 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;
> > ...
> 
> Yes, that sounds good.
> 
> (I noticed struct request has a reference count so that might be a way
> to keep requests alive, if necessary, but I haven't investigated. See
> req_ref_put_and_test() though it's not used by block drivers and maybe
> virtio-blk shouldn't mess with it either.)

I also think that using ref count is not a good idea.
I will send the next version soon.

Regards,
Suwan Kim