On Sun, 2011-05-29 at 04:09 -0400, Vincent, Pradeep wrote:
> 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'.
There's not much room for opportunism left here. After FINAL_CHECK
returning with !_work_to_do you're going to receive an interrupt.
Holding that notification off would kill performance.
>From there on, still leaving a duplicate check around end_io has only an
infinitesimal chance to preempt (none to prevent) the event reception.
Even if it ever happens, the chance of making a difference in time to
actual thread wake is probably even smaller.
I think it's just overhead. If you disagree, this stuff is easy to prove
or confute with event counting. Good luck :)
> Also, should RING_FINAL_CHECK_FOR_REQUESTS be protected by blk_ring_lock ?
Nope. The ring lock is only needed to sync rsp production. Specifically,
make_response upon request completion colliding with make_response
called from the backend thread (the error case in do_block_io_op).
Should rather be named rsp_lock or so, it doesn't lock anything except
rsp_prod.
Daniel
>
> - Pradeep Vincent
>
>
> On 5/28/11 1:21 PM, "Daniel Stodden" <daniel.stodden@xxxxxxxxxx> 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 <daniel.stodden@xxxxxxxxxx>
> >---
> > 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 = &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;
> > }
> >
> >+static int
> >+do_block_io_op(blkif_t *blkif)
> >+{
> >+ blkif_back_rings_t *blk_rings = &blkif->blk_rings;
> >+ int more_to_do;
> >+
> >+ do {
> >+ more_to_do = __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 = &blkif->blk_rings;
> >- int more_to_do = 0;
> > int notify;
> >
> > resp.id = 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 == 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 = 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);
> > }
> >--
> >1.7.4.1
> >
>
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
Home