Xen devel - Oct 2009 - [PATCH] make blkfront/blktagp2 respect the elevator=xyz command line option

For some workloads, CFQ has better performance than the no-op scheduler
that is forced by the blkfront driver.  The only way to set a different
scheduler is the sysfs interface, because elevator_init is called
unconditionally.  This patch allows one to use "elevator=cfq" as well.

While one could argue that the driver''s behavior is expected (after all
"elevator=cfq" is the default and should not have any effect), the
do-what-I-mean behavior implemented by this patch is more logical.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block/elevator.c             |    3 ++-
 drivers/xen/blkfront/vbd.c   |   11 ++++++-----
 drivers/xen/blktap2/device.c |   11 ++++++-----
 3 files changed, 14 insertions(+), 11 deletions(-)

diff --git a/block/elevator.c b/block/elevator.c
--- a/block/elevator.c
+++ b/block/elevator.c
@@ -132,7 +132,8 @@
 	eq->elevator_data = data;
 }
 
-static char chosen_elevator[16];
+char chosen_elevator[16];
+EXPORT_SYMBOL_GPL(chosen_elevator);
 
 static int __init elevator_setup(char *str)
 {
diff --git a/drivers/xen/blkfront/vbd.c b/drivers/xen/blkfront/vbd.c
--- a/drivers/xen/blkfront/vbd.c
+++ b/drivers/xen/blkfront/vbd.c
@@ -208,6 +208,8 @@
 	/* XXX: release major if 0 */
 }
 
+extern char chosen_elevator[];
+
 static int
 xlvbd_init_blk_queue(struct gendisk *gd, u16 sector_size)
 {
@@ -217,11 +219,10 @@
 	if (rq == NULL)
 		return -1;
 
-#if LINUX_VERSION_CODE >= KERNEL_VERSION(2,6,10)
-	elevator_init(rq, "noop");
-#else
-	elevator_init(rq, &elevator_noop);
-#endif
+	/* Always respect the user''s explicitly chosen elevator, but
otherwise
+	   pick a different default than CONFIG_DEFAULT_IOSCHED.  */
+	if (!*chosen_elevator)
+		elevator_init(rq, "noop");
 
 	/* Hard sector size and max sectors impersonate the equiv. hardware. */
 	blk_queue_hardsect_size(rq, sector_size);
diff --git a/drivers/xen/blktap2/device.c b/drivers/xen/blktap2/device.c
--- a/drivers/xen/blktap2/device.c
+++ b/drivers/xen/blktap2/device.c
@@ -1034,6 +1034,8 @@
 	return 0;
 }
 
+extern char chosen_elevator[];
+
 int
 blktap_device_create(struct blktap *tap)
 {
@@ -1078,11 +1080,10 @@
 	if (!rq)
 		goto error;
 
-#if LINUX_VERSION_CODE >= KERNEL_VERSION(2,6,10)
-	elevator_init(rq, "noop");
-#else
-	elevator_init(rq, &elevator_noop);
-#endif
+	/* Always respect the user''s explicitly chosen elevator, but
otherwise
+	   pick a different default than CONFIG_DEFAULT_IOSCHED.  */
+	if (!*chosen_elevator)
+		elevator_init(rq, "noop");
 
 	gd->queue     = rq;
 	rq->queuedata = dev;

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

Should we bother to call elevator_init() at all? The call has been in that
2.6 driver forever, and there''s probably no great reason. Shall we just
kill
it?

 -- Keir

On 09/10/2009 17:31, "Paolo Bonzini" <pbonzini@redhat.com>
wrote:
> For some workloads, CFQ has better performance than the no-op scheduler
> that is forced by the blkfront driver.  The only way to set a different
> scheduler is the sysfs interface, because elevator_init is called
> unconditionally.  This patch allows one to use "elevator=cfq" as
well.
> 
> While one could argue that the driver''s behavior is expected
(after all
> "elevator=cfq" is the default and should not have any effect),
the
> do-what-I-mean behavior implemented by this patch is more logical.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  block/elevator.c             |    3 ++-
>  drivers/xen/blkfront/vbd.c   |   11 ++++++-----
>  drivers/xen/blktap2/device.c |   11 ++++++-----
>  3 files changed, 14 insertions(+), 11 deletions(-)
> 
> diff --git a/block/elevator.c b/block/elevator.c
> --- a/block/elevator.c
> +++ b/block/elevator.c
> @@ -132,7 +132,8 @@
> eq->elevator_data = data;
>  }
>  
> -static char chosen_elevator[16];
> +char chosen_elevator[16];
> +EXPORT_SYMBOL_GPL(chosen_elevator);
>  
>  static int __init elevator_setup(char *str)
>  {
> diff --git a/drivers/xen/blkfront/vbd.c b/drivers/xen/blkfront/vbd.c
> --- a/drivers/xen/blkfront/vbd.c
> +++ b/drivers/xen/blkfront/vbd.c
> @@ -208,6 +208,8 @@
> /* XXX: release major if 0 */
>  }
>  
> +extern char chosen_elevator[];
> +
>  static int
>  xlvbd_init_blk_queue(struct gendisk *gd, u16 sector_size)
>  {
> @@ -217,11 +219,10 @@
> if (rq == NULL)
> return -1;
>  
> -#if LINUX_VERSION_CODE >= KERNEL_VERSION(2,6,10)
> - elevator_init(rq, "noop");
> -#else
> - elevator_init(rq, &elevator_noop);
> -#endif
> + /* Always respect the user''s explicitly chosen elevator, but
otherwise
> +    pick a different default than CONFIG_DEFAULT_IOSCHED.  */
> + if (!*chosen_elevator)
> +  elevator_init(rq, "noop");
>  
> /* Hard sector size and max sectors impersonate the equiv. hardware. */
> blk_queue_hardsect_size(rq, sector_size);
> diff --git a/drivers/xen/blktap2/device.c b/drivers/xen/blktap2/device.c
> --- a/drivers/xen/blktap2/device.c
> +++ b/drivers/xen/blktap2/device.c
> @@ -1034,6 +1034,8 @@
> return 0;
>  }
>  
> +extern char chosen_elevator[];
> +
>  int
>  blktap_device_create(struct blktap *tap)
>  {
> @@ -1078,11 +1080,10 @@
> if (!rq)
> goto error;
>  
> -#if LINUX_VERSION_CODE >= KERNEL_VERSION(2,6,10)
> - elevator_init(rq, "noop");
> -#else
> - elevator_init(rq, &elevator_noop);
> -#endif
> + /* Always respect the user''s explicitly chosen elevator, but
otherwise
> +    pick a different default than CONFIG_DEFAULT_IOSCHED.  */
> + if (!*chosen_elevator)
> +  elevator_init(rq, "noop");
>  
> gd->queue     = rq;
> rq->queuedata = dev;
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel


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

On Fri, 2009-10-09 at 12:57 -0400, Keir Fraser wrote:
> Should we bother to call elevator_init() at all? The call has been in that
> 2.6 driver forever, and there''s probably no great reason. Shall we
just kill
> it?
> 
>  -- Keir
In blktap2, I''m not convinced that going with the system default
scheduler should be the common choice. Let''s go for "noop" vs
a module
option, if people indeed want to experiment with it.

For blkfront, I agree. Is there a use case for defaulting to something
different than a system default?

Daniel


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

On 09/10/2009 19:33, "Daniel Stodden"
<Daniel.Stodden@citrix.com> wrote:
> In blktap2, I''m not convinced that going with the system default
> scheduler should be the common choice. Let''s go for
"noop" vs a module
> option, if people indeed want to experiment with it.
I think the patch should touch only blkfront. I suspect blktap2 has been
misunderstood and oncorrectly included in the patch.
> For blkfront, I agree. Is there a use case for defaulting to something
> different than a system default?
Noone else does it, except maybe some old s390 drivers. We''ll kill it
then.

 -- Keir



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

>> For blkfront, I agree. Is there a use case for defaulting to something
>> different than a system default?
>
> Noone else does it, except maybe some old s390 drivers. We''ll kill
it then.
The rationale was that guest I/O delays depend on the I/O done by all 
other guests at the same time.  So, in theory there is no need to 
schedule it on the guest---assuming hypercalls are fast enough it should 
be passed down to the host immediately and left to be scheduled together 
with all the other host I/O.

However, actual benchmarks show that this does not always hold 
(sometimes disastrously) so killing the elevator_init call is safe and 
is fine by me.

Paolo

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

On Fri, Oct 09, 2009 at 06:31:54PM +0200, Paolo Bonzini
wrote:
> For some workloads, CFQ has better performance than the no-op scheduler
> that is forced by the blkfront driver.  The only way to set a different
> scheduler is the sysfs interface, because elevator_init is called
> unconditionally.  This patch allows one to use "elevator=cfq" as
well.
> 
> While one could argue that the driver''s behavior is expected
(after all
> "elevator=cfq" is the default and should not have any effect),
the
> do-what-I-mean behavior implemented by this patch is more logical.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Are you going to follow another patch for the upstream driver as well?
Or is that not needed at all in the 2.6.31 tree?

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

On 10/12/2009 09:32 PM, Konrad Rzeszutek Wilk wrote:
> Are you going to follow another patch for the upstream driver as well?
> Or is that not needed at all in the 2.6.31 tree?
It is not needed.

Paolo

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