From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Vincent, Pradeep" Subject: Re: [PATCH] xen/blkback: Don't let in-flight requests defer pending ones. Date: Sun, 29 May 2011 01:09:19 -0700 Message-ID: References: <1306614070-13137-1-git-send-email-daniel.stodden@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Return-path: In-Reply-To: <1306614070-13137-1-git-send-email-daniel.stodden@citrix.com> Content-Language: en-US List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xensource.com Errors-To: xen-devel-bounces@lists.xensource.com To: Daniel Stodden , Xen Cc: "konrad.wilk@oracle.com" List-Id: xen-devel@lists.xenproject.org Opportunistically avoiding interrupts by checking for I/Os in the flight doesn't sound like a bad idea. I think the RING_HAS_UNCONSUMED_REQUESTS call and what follows should be retained in 'make_response'. Also, should RING_FINAL_CHECK_FOR_REQUESTS be protected by blk_ring_lock ? - Pradeep Vincent On 5/28/11 1:21 PM, "Daniel Stodden" wrote: >Running RING_FINAL_CHECK_FOR_REQUESTS from make_response is a bad >idea. It means that in-flight I/O is essentially blocking continued >batches. This essentially kills throughput on frontends which unplug >(or even just notify) early and rightfully assume addtional requests >will be picked up on time, not synchronously. > >Signed-off-by: Daniel Stodden >--- > drivers/block/xen-blkback/blkback.c | 36 >++++++++++++++++++---------------- > 1 files changed, 19 insertions(+), 17 deletions(-) > >diff --git a/drivers/block/xen-blkback/blkback.c >b/drivers/block/xen-blkback/blkback.c >index 9dee545..48ad7fa 100644 >--- a/drivers/block/xen-blkback/blkback.c >+++ b/drivers/block/xen-blkback/blkback.c >@@ -451,7 +451,8 @@ static void end_block_io_op(struct bio *bio, int >error) > * (which has the sectors we want, number of them, grant references, >etc), > * and transmute it to the block API to hand it over to the proper >block disk. > */ >-static int do_block_io_op(struct xen_blkif *blkif) >+static int >+__do_block_io_op(struct xen_blkif *blkif) > { > union blkif_back_rings *blk_rings =3D &blkif->blk_rings; > struct blkif_request req; >@@ -508,6 +509,23 @@ static int do_block_io_op(struct xen_blkif *blkif) > return more_to_do; > } >=20 >+static int >+do_block_io_op(blkif_t *blkif) >+{ >+ blkif_back_rings_t *blk_rings =3D &blkif->blk_rings; >+ int more_to_do; >+ >+ do { >+ more_to_do =3D __do_block_io_op(blkif); >+ if (more_to_do) >+ break; >+ >+ RING_FINAL_CHECK_FOR_REQUESTS(&blk_rings->common, more_to_do); >+ } while (more_to_do); >+ >+ return more_to_do; >+} >+ > /* > * Transmutation of the 'struct blkif_request' to a proper 'struct bio' > * and call the 'submit_bio' to pass it to the underlying storage. >@@ -698,7 +716,6 @@ static void make_response(struct xen_blkif *blkif, >u64 id, > struct blkif_response resp; > unsigned long flags; > union blkif_back_rings *blk_rings =3D &blkif->blk_rings; >- int more_to_do =3D 0; > int notify; >=20 > resp.id =3D id; >@@ -725,22 +742,7 @@ static void make_response(struct xen_blkif *blkif, >u64 id, > } > blk_rings->common.rsp_prod_pvt++; > RING_PUSH_RESPONSES_AND_CHECK_NOTIFY(&blk_rings->common, notify); >- if (blk_rings->common.rsp_prod_pvt =3D=3D blk_rings->common.req_cons)= { >- /* >- * Tail check for pending requests. Allows frontend to avoid >- * notifications if requests are already in flight (lower >- * overheads and promotes batching). >- */ >- RING_FINAL_CHECK_FOR_REQUESTS(&blk_rings->common, more_to_do); >- >- } else if (RING_HAS_UNCONSUMED_REQUESTS(&blk_rings->common)) { >- more_to_do =3D 1; >- } >- > spin_unlock_irqrestore(&blkif->blk_ring_lock, flags); >- >- if (more_to_do) >- blkif_notify_work(blkif); > if (notify) > notify_remote_via_irq(blkif->irq); > } >--=20 >1.7.4.1 >