Re: [PATCH] xen/blkback: Don't let in-flight requests defer pending ones.

[Daniel Stodden <daniel.stodden@citrix.com>]

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@citrix.com> 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@citrix.com>
> >---
> > 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
> >
>