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
|
Home