WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

[Xen-devel] Re: [PATCH] xen/blkback: Don't let in-flight requests defer

To: Daniel Stodden <daniel.stodden@xxxxxxxxxx>, Jan Beulich <JBeulich@xxxxxxxxxx>
Subject: [Xen-devel] Re: [PATCH] xen/blkback: Don't let in-flight requests defer pending ones.
From: "Vincent, Pradeep" <pradeepv@xxxxxxxxxx>
Date: Wed, 1 Jun 2011 01:02:36 -0700
Accept-language: en-US
Acceptlanguage: en-US
Cc: Xen <xen-devel@xxxxxxxxxxxxxxxxxxx>, "konrad.wilk@xxxxxxxxxx" <konrad.wilk@xxxxxxxxxx>
Delivery-date: Wed, 01 Jun 2011 01:05:12 -0700
Envelope-to: www-data@xxxxxxxxxxxxxxxxxxx
In-reply-to: <1306668851.20284.123.camel@ramone>
List-help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-id: Xen developer discussion <xen-devel.lists.xensource.com>
List-post: <mailto:xen-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
Thread-index: AcwgMkWHm2IE0CDISOG7OK8DCoTtyw==
Thread-topic: [PATCH] xen/blkback: Don't let in-flight requests defer pending ones.
User-agent: Microsoft-MacOutlook/14.0.0.100825
Re: Opportunistic check in make_response

Looking through blkfront logic, req_prod is only updated in batches - so
you are right that given the current blkfront design the opportunistic
check in make_response doesn't reduce interrupt rate and the benefit will
be limited to the reduction of I/O latency by an amount less than the
interrupt latency. 

Having said that, I think batching req_prod update delays the processing
of the first I/O by blkback until all the I/Os in the batch have been
added to the I/O ring - potentially increasing I/O latencies. While batche
interrupt generation makes perfect sense, I don't see a solid reason for
req_prod not being updated when I/Os are processed by blkfront. Netfront
does increment req_prod as soon as it processes each skb (but it might
send an interrupt for each skb as well which isn't great).

I think it would make sense to enhance blkfront design to increment
req_prod as soon as it processes an I/O while batching irq generation.
When blkfront and blkback are busy processing continuous stream of I/O
requests, it would be great if blkfront-blkback pipeline is able to
process them without generating unnecessary interrupts while improving I/O
latency performance. Thoughts ? Any historical context on why this might
be bad ?

If blkfront does increment req_prod on every I/O submission, I think
opportunistic check in make_response would make sense since such a check
could trigger blkback to start processing requests well ahead of the
'batch' completion on blkfront side (just the check for
RING_HAS_UNCONSUMED_REQUESTS should suffice - other parts of what you
removed should go)


Re: Locking

I was reflecting Jan Beulich's comment earlier in this thread. Like I said
before (in this thread), the locking logic in blkback isn't obvious from
the code and the failure modes seem benign. If someone has good context on
blkback locking strategy, I would love to learn. Also it would be very
useful to add some comments around lock usage to the blkback code.

Jan ??

- Pradeep Vincent
 

On 5/29/11 4:34 AM, "Daniel Stodden" <daniel.stodden@xxxxxxxxxx> wrote:

>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