Re: [PATCH 2/2] virtio-blk: support completion batching for the IRQ path

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

On Wed, Dec 07, 2022 at 05:05:06PM -0500, Stefan Hajnoczi wrote:
> On Tue, Dec 06, 2022 at 11:11:25PM +0900, Suwan Kim wrote:
> > This patch adds completion batching to the IRQ path. It reuses batch
> > completion code of virtblk_poll(). It collects requests to io_comp_batch
> > and processes them all at once. It can boost up the performance by 2%.
> > 
> > To validate the performance improvement and stabilty, I did fio test with
> > 4 vCPU VM and 12 vCPU VM respectively. Both VMs have 8GB ram and the same
> > number of HW queues as vCPU.
> > The fio cammad is as follows and I ran the fio 5 times and got IOPS average.
> > (io_uring, randread, direct=1, bs=512, iodepth=64 numjobs=2,4)
> > 
> > Test result shows about 2% improvement.
> > 
> >            4 vcpu VM       |   numjobs=2   |   numjobs=4
> >       -----------------------------------------------------------
> >         fio without patch  |  367.2K IOPS  |   397.6K IOPS
> >       -----------------------------------------------------------
> >         fio with patch     |  372.8K IOPS  |   407.7K IOPS
> > 
> >            12 vcpu VM      |   numjobs=2   |   numjobs=4
> >       -----------------------------------------------------------
> >         fio without patch  |  363.6K IOPS  |   374.8K IOPS
> >       -----------------------------------------------------------
> >         fio with patch     |  373.8K IOPS  |   385.3K IOPS
> > 
> > Signed-off-by: Suwan Kim <suwan.kim027@gmail.com>
> > ---
> >  drivers/block/virtio_blk.c | 38 +++++++++++++++++++++++---------------
> >  1 file changed, 23 insertions(+), 15 deletions(-)
> 
> Cool, thanks for doing this!
> 
> > diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> > index cf64d256787e..48fcf745f007 100644
> > --- a/drivers/block/virtio_blk.c
> > +++ b/drivers/block/virtio_blk.c
> > @@ -272,6 +272,18 @@ static inline void virtblk_request_done(struct request *req)
> >  	blk_mq_end_request(req, virtblk_result(vbr));
> >  }
> >  
> > +static void virtblk_complete_batch(struct io_comp_batch *iob)
> > +{
> > +	struct request *req;
> > +
> > +	rq_list_for_each(&iob->req_list, req) {
> > +		virtblk_unmap_data(req, blk_mq_rq_to_pdu(req));
> > +		virtblk_cleanup_cmd(req);
> > +		blk_mq_set_request_complete(req);
> > +	}
> > +	blk_mq_end_request_batch(iob);
> > +}
> > +
> >  static void virtblk_done(struct virtqueue *vq)
> >  {
> >  	struct virtio_blk *vblk = vq->vdev->priv;
> > @@ -280,6 +292,7 @@ static void virtblk_done(struct virtqueue *vq)
> >  	struct virtblk_req *vbr;
> >  	unsigned long flags;
> >  	unsigned int len;
> > +	DEFINE_IO_COMP_BATCH(iob);
> >  
> >  	spin_lock_irqsave(&vblk->vqs[qid].lock, flags);
> >  	do {
> > @@ -287,7 +300,9 @@ static void virtblk_done(struct virtqueue *vq)
> >  		while ((vbr = virtqueue_get_buf(vblk->vqs[qid].vq, &len)) != NULL) {
> >  			struct request *req = blk_mq_rq_from_pdu(vbr);
> >  
> > -			if (likely(!blk_should_fake_timeout(req->q)))
> > +			if (likely(!blk_should_fake_timeout(req->q)) &&
> > +				!blk_mq_add_to_batch(req, &iob, vbr->status,
> > +							virtblk_complete_batch))
> >  				blk_mq_complete_request(req);
> >  			req_done = true;
> >  		}
> > @@ -295,9 +310,14 @@ static void virtblk_done(struct virtqueue *vq)
> >  			break;
> >  	} while (!virtqueue_enable_cb(vq));
> >  
> > -	/* In case queue is stopped waiting for more buffers. */
> > -	if (req_done)
> > +	if (req_done) {
> > +		if (!rq_list_empty(iob.req_list))
> > +			virtblk_complete_batch(&iob);
> 
> A little optimization to avoid the indirect call: iob.complete(&iob) :).
> Not sure if it's good style to do that but it works in this case because
> we know it can only be virtblk_complete_batch().
> 
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

Hi Stefan,

Thanks for the comment!
It also needs to use blk_mq_complete_request_remote() instead of
blk_mq_set_request_complete()
I will resend it with the modification of patch #1 and then please
review it again.

Regards,
Suwan Kim