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

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

[-- Attachment #1: Type: text/plain, Size: 3058 bytes --]


I attached the event counting patch. It should make sense, but it was
against XCP, so it may not apply all smoothly for you. If you really
want to play with it, and manage to find more interesting
storage/system/patch combinations than the one quoted below, that'd
probaby be really useful.

Daniel

On Wed, 2011-06-01 at 13:49 -0400, Daniel Stodden wrote:
> > 
> > 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 ?
> 
> No, and it's a good idea, and your assumptions are imho correct.
> 
> The extreme case is an PUSH_REQUESTS_AND_CHECK_NOTIFY after each
> request. Even in this case, the majority of interrupts should be held
> off by looking at req_event. There are certainly variants, but I can
> show you some results for what you're asking. because I happened to try
> that just last week. 
> 
> Next consider adding a couple event counters in the backend. And you
> need the patch above, of course.
> 
> req_event: Frontend event received
> rsp_event: Frontend notification sent
> req_again: FINAL_CHECK indicated more_to_do.

> boost-1, order=0:
> 
> (This is the unmodified version)
> 
> dd if=/dev/zero of=/dev/xvdc bs=1M count=1024
> 1073741824 bytes (1.1 GB) copied, 17.0105 s, 63.1 MB/s
> 1073741824 bytes (1.1 GB) copied, 17.7566 s, 60.5 MB/s
> 1073741824 bytes (1.1 GB) copied, 17.163 s, 62.6 MB/s
> 
> rsp_event 6759
> req_event 6753
> req_again 16
> 
> boost-2, order=0
> 
> (This was the aggressive one, one PUSH_NOTIFY per ring request).
> 
> dd if=/dev/zero of=/dev/xvdc bs=1M count=1024
> 1073741824 bytes (1.1 GB) copied, 17.3208 s, 62.0 MB/s
> 1073741824 bytes (1.1 GB) copied, 17.4851 s, 61.4 MB/s
> 1073741824 bytes (1.1 GB) copied, 17.7333 s, 60.5 MB/s
> 
> rsp_event 7122
> req_event 7141
> req_again 5497
> 
> 
> So the result is that the event load even in the most aggressive case
> will typically increase only moderately. Instead, the restored outer
> loop in the dispatcher just starts to play out.
> 
> I'm not proposing this as the final solution, we might chose to be more
> careful and limit event emission to some stride instead.
> 
> Don't be confused by the throughput values not going up, the problem I
> had with the array (iSCSI in this case) just turned out to be elsewhere.
> I'm pretty convinced there are workload/storage configurations which
> benefit from that.
> 
> In the case at hand, increasing the ring size was way more productive.
> At which point the queue depth multiplies as well. And I currently
> expect that the longer it gets the more urgent the issue you describe
> will be.
> 
> But I also think it needs some experiments and wants to be backed by
> numbers.
> 
> Daniel


[-- Attachment #2: blkback-final-check-stats.diff --]
[-- Type: text/x-patch, Size: 2262 bytes --]

# HG changeset patch
# Parent 9c6a7fdebc7fcb76421a8f053be7f5dfcf30c741

diff -r 9c6a7fdebc7f drivers/xen/blkback/blkback.c
--- a/drivers/xen/blkback/blkback.c	Fri May 27 15:32:58 2011 -0700
+++ b/drivers/xen/blkback/blkback.c	Fri May 27 15:33:39 2011 -0700
@@ -543,6 +543,8 @@
 	if (blkif->xenblkd)
 		wake_up_process(blkif->xenblkd);
 
+	blkif->st_req_event++;
+
 	return IRQ_HANDLED;
 }
 
@@ -641,6 +643,8 @@
 			break;
 
 		RING_FINAL_CHECK_FOR_REQUESTS(&blk_rings->common, more_to_do);
+		if (more_to_do)
+			blkif->st_req_again++;
 	} while (more_to_do);
 
 	return more_to_do;
@@ -862,8 +866,10 @@
 	blk_rings->common.rsp_prod_pvt++;
 	RING_PUSH_RESPONSES_AND_CHECK_NOTIFY(&blk_rings->common, notify);
 	spin_unlock_irqrestore(&blkif->blk_ring_lock, flags);
-	if (notify)
+	if (notify) {
 		notify_remote_via_irq(blkif->irq);
+		blkif->st_rsp_event++;
+	}
 }
 
 static int __init blkif_init(void)
diff -r 9c6a7fdebc7f drivers/xen/blkback/common.h
--- a/drivers/xen/blkback/common.h	Fri May 27 15:32:58 2011 -0700
+++ b/drivers/xen/blkback/common.h	Fri May 27 15:33:39 2011 -0700
@@ -117,6 +117,10 @@
 	s64                 st_wr_sum_usecs;
 	s64                 st_wr_max_usecs;
 
+	u64                 st_req_event;
+	u64		    st_req_again;
+	u64                 st_rsp_event;
+
 	unsigned int   nr_shared_pages;
 	grant_handle_t shmem_handle[BLKIF_MAX_RING_PAGES];
 } blkif_t;
diff -r 9c6a7fdebc7f drivers/xen/blkback/xenbus.c
--- a/drivers/xen/blkback/xenbus.c	Fri May 27 15:32:58 2011 -0700
+++ b/drivers/xen/blkback/xenbus.c	Fri May 27 15:33:39 2011 -0700
@@ -337,6 +337,9 @@
 VBD_SHOW(br_req,  "%d\n", be->blkif->st_br_req);
 VBD_SHOW(rd_sect, "%d\n", be->blkif->st_rd_sect);
 VBD_SHOW(wr_sect, "%d\n", be->blkif->st_wr_sect);
+VBD_SHOW(req_event, "%llu\n", be->blkif->st_req_event);
+VBD_SHOW(rsp_event, "%llu\n", be->blkif->st_rsp_event);
+VBD_SHOW(req_again, "%llu\n", be->blkif->st_req_again);
 VBD_SHOW_AVG(rd_usecs, be->blkif->st_rd_cnt,
 	     be->blkif->st_rd_sum_usecs, be->blkif->st_rd_max_usecs);
 VBD_SHOW_AVG(wr_usecs, be->blkif->st_wr_cnt,
@@ -351,6 +354,9 @@
 	&dev_attr_wr_sect.attr,
 	&dev_attr_rd_usecs.attr,
 	&dev_attr_wr_usecs.attr,
+	&dev_attr_req_event.attr,
+	&dev_attr_rsp_event.attr,
+	&dev_attr_req_again.attr,
 	NULL
 };
 

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel