[PATCH 0/5] virtio-scsi: create a separate workqueue

[PATCH 0/5] virtio-scsi: create a separate workqueue

[Wanlong Gao]

patch 1-3,5 are some cleanups.
patch 4: create a separate work queue for virtio-scsi
to improve the performance, I tested with tmpfs backed
disk, the config file is like below,
[global]
bsrange=4k-64k
ioengine=libaio
direct=1
iodepth=4
loops=10
size=64M

Before:
Disk stats (read/write):
  sda: ios=6547/5275, merge=37/39, ticks=3144/2645, in_queue=5780, util=81.62%
  sdb: ios=6542/5277, merge=54/59, ticks=3234/3143, in_queue=6364, util=84.50%
  sdc: ios=6532/5244, merge=65/62, ticks=3440/3076, in_queue=6505, util=86.83%

After:
Disk stats (read/write):
  sda: ios=6349/5318, merge=77/89, ticks=4820/3169, in_queue=7991, util=90.62%
  sdb: ios=6364/5358, merge=39/53, ticks=3810/2816, in_queue=6615, util=84.63%
  sdc: ios=6458/5352, merge=74/47, ticks=4425/3048, in_queue=7459, util=88.41%


Wanlong Gao (5):
  virtio-scsi: remove the useless assignment
  virtio-scsi: remove the needless variable gfp_mask
  virtio-scsi: use pr_err instead of printk
  virtio-scsi: create a separate work queue for virtio-scsi
  virtio-scsi: tidy up the goto label in init()



Cc: James E.J. Bottomley <JBottomley@parallels.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Rusty Russell <rusty@rustcorp.com.au>
Cc: linux-scsi@vger.kernel.org
Cc: kvm@vger.kernel.org
 drivers/scsi/virtio_scsi.c | 43 +++++++++++++++++++++++--------------------
 1 file changed, 23 insertions(+), 20 deletions(-)

-- 
1.8.0

[PATCH 1/5] virtio-scsi: remove the useless assignment

[Wanlong Gao]

Reassign err is not needed, just a cleanup.

Cc: James E.J. Bottomley <JBottomley@parallels.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Rusty Russell <rusty@rustcorp.com.au>
Cc: linux-scsi@vger.kernel.org
Cc: kvm@vger.kernel.org
Signed-off-by: Wanlong Gao <gaowanlong@cn.fujitsu.com>
---
 drivers/scsi/virtio_scsi.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
index d5f9f45..8746c37 100644
--- a/drivers/scsi/virtio_scsi.c
+++ b/drivers/scsi/virtio_scsi.c
@@ -669,7 +669,6 @@ static int virtscsi_init(struct virtio_device *vdev,
 			goto out;
 		}
 	}
-	err = 0;
 
 out:
 	if (err)
-- 
1.8.0

[PATCH 2/5] virtio-scsi: remove the needless variable gfp_mask

[Wanlong Gao]

Just use the macro instead of define a variable.


Cc: James E.J. Bottomley <JBottomley@parallels.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Rusty Russell <rusty@rustcorp.com.au>
Cc: linux-scsi@vger.kernel.org
Cc: kvm@vger.kernel.org
Signed-off-by: Wanlong Gao <gaowanlong@cn.fujitsu.com>
---
 drivers/scsi/virtio_scsi.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
index 8746c37..5390229 100644
--- a/drivers/scsi/virtio_scsi.c
+++ b/drivers/scsi/virtio_scsi.c
@@ -587,11 +587,10 @@ static struct virtio_scsi_target_state *virtscsi_alloc_tgt(
 	struct virtio_device *vdev, int sg_elems)
 {
 	struct virtio_scsi_target_state *tgt;
-	gfp_t gfp_mask = GFP_KERNEL;
 
 	/* We need extra sg elements at head and tail.  */
 	tgt = kmalloc(sizeof(*tgt) + sizeof(tgt->sg[0]) * (sg_elems + 2),
-		      gfp_mask);
+		      GFP_KERNEL);
 
 	if (!tgt)
 		return NULL;
-- 
1.8.0

[PATCH 3/5] virtio-scsi: use pr_err instead of printk

[Wanlong Gao]

Use pr_err() instead of printk() for code cleanups.

Cc: James E.J. Bottomley <JBottomley@parallels.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Rusty Russell <rusty@rustcorp.com.au>
Cc: linux-scsi@vger.kernel.org
Cc: kvm@vger.kernel.org
Signed-off-by: Wanlong Gao <gaowanlong@cn.fujitsu.com>
---
 drivers/scsi/virtio_scsi.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
index 5390229..765138a 100644
--- a/drivers/scsi/virtio_scsi.c
+++ b/drivers/scsi/virtio_scsi.c
@@ -790,8 +790,7 @@ static int __init init(void)
 
 	virtscsi_cmd_cache = KMEM_CACHE(virtio_scsi_cmd, 0);
 	if (!virtscsi_cmd_cache) {
-		printk(KERN_ERR "kmem_cache_create() for "
-				"virtscsi_cmd_cache failed\n");
+		pr_err("kmem_cache_create() for virtscsi_cmd_cache failed\n");
 		goto error;
 	}
 
@@ -800,8 +799,7 @@ static int __init init(void)
 		mempool_create_slab_pool(VIRTIO_SCSI_MEMPOOL_SZ,
 					 virtscsi_cmd_cache);
 	if (!virtscsi_cmd_pool) {
-		printk(KERN_ERR "mempool_create() for"
-				"virtscsi_cmd_pool failed\n");
+		pr_err("mempool_create() for virtscsi_cmd_pool failed\n");
 		goto error;
 	}
 	ret = register_virtio_driver(&virtio_scsi_driver);
-- 
1.8.0

[PATCH 4/5] virtio-scsi: create a separate work queue for virtio-scsi

[Wanlong Gao]

Create a separate work queue for virtio-scsi to improve the performance.

Cc: James E.J. Bottomley <JBottomley@parallels.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Rusty Russell <rusty@rustcorp.com.au>
Cc: linux-scsi@vger.kernel.org
Cc: kvm@vger.kernel.org
Signed-off-by: Wanlong Gao <gaowanlong@cn.fujitsu.com>
---
 drivers/scsi/virtio_scsi.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
index 765138a..fc05240 100644
--- a/drivers/scsi/virtio_scsi.c
+++ b/drivers/scsi/virtio_scsi.c
@@ -27,6 +27,8 @@
 #define VIRTIO_SCSI_MEMPOOL_SZ 64
 #define VIRTIO_SCSI_EVENT_LEN 8
 
+struct workqueue_struct *virtscsi_wq;
+
 /* Command queue element */
 struct virtio_scsi_cmd {
 	struct scsi_cmnd *sc;
@@ -337,7 +339,7 @@ static void virtscsi_complete_event(void *buf)
 	struct virtio_scsi_event_node *event_node = buf;
 
 	INIT_WORK(&event_node->work, virtscsi_handle_event);
-	schedule_work(&event_node->work);
+	queue_work(virtscsi_wq, &event_node->work);
 }
 
 static void virtscsi_event_done(struct virtqueue *vq)
@@ -788,6 +790,12 @@ static int __init init(void)
 {
 	int ret = -ENOMEM;
 
+	virtscsi_wq = alloc_workqueue("virtio-scsi", 0, 0);
+	if (!virtscsi_wq) {
+		pr_err("alloc_workqueue() for virtscsi_wq failed\n");
+		goto error;
+	}
+
 	virtscsi_cmd_cache = KMEM_CACHE(virtio_scsi_cmd, 0);
 	if (!virtscsi_cmd_cache) {
 		pr_err("kmem_cache_create() for virtscsi_cmd_cache failed\n");
@@ -817,6 +825,10 @@ error:
 		kmem_cache_destroy(virtscsi_cmd_cache);
 		virtscsi_cmd_cache = NULL;
 	}
+	if (virtscsi_wq) {
+		destroy_workqueue(virtscsi_wq);
+		virtscsi_wq = NULL;
+	}
 	return ret;
 }
 
@@ -825,6 +837,7 @@ static void __exit fini(void)
 	unregister_virtio_driver(&virtio_scsi_driver);
 	mempool_destroy(virtscsi_cmd_pool);
 	kmem_cache_destroy(virtscsi_cmd_cache);
+	destroy_workqueue(virtscsi_wq);
 }
 module_init(init);
 module_exit(fini);
-- 
1.8.0

[PATCH 5/5] virtio-scsi: tidy up the goto label in init()

[Wanlong Gao]

Tidy up the goto label in init(), and remove the useless
NULL pointer assignment.

Cc: James E.J. Bottomley <JBottomley@parallels.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Rusty Russell <rusty@rustcorp.com.au>
Cc: linux-scsi@vger.kernel.org
Cc: kvm@vger.kernel.org
Signed-off-by: Wanlong Gao <gaowanlong@cn.fujitsu.com>
---
 drivers/scsi/virtio_scsi.c | 28 +++++++++++-----------------
 1 file changed, 11 insertions(+), 17 deletions(-)

diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
index fc05240..afee1d4 100644
--- a/drivers/scsi/virtio_scsi.c
+++ b/drivers/scsi/virtio_scsi.c
@@ -793,13 +793,13 @@ static int __init init(void)
 	virtscsi_wq = alloc_workqueue("virtio-scsi", 0, 0);
 	if (!virtscsi_wq) {
 		pr_err("alloc_workqueue() for virtscsi_wq failed\n");
-		goto error;
+		goto workqueue_failed;
 	}
 
 	virtscsi_cmd_cache = KMEM_CACHE(virtio_scsi_cmd, 0);
 	if (!virtscsi_cmd_cache) {
 		pr_err("kmem_cache_create() for virtscsi_cmd_cache failed\n");
-		goto error;
+		goto kmem_cache_failed;
 	}
 
 
@@ -808,27 +808,21 @@ static int __init init(void)
 					 virtscsi_cmd_cache);
 	if (!virtscsi_cmd_pool) {
 		pr_err("mempool_create() for virtscsi_cmd_pool failed\n");
-		goto error;
+		goto mempool_failed;
 	}
 	ret = register_virtio_driver(&virtio_scsi_driver);
 	if (ret < 0)
-		goto error;
+		goto register_failed;
 
 	return 0;
 
-error:
-	if (virtscsi_cmd_pool) {
-		mempool_destroy(virtscsi_cmd_pool);
-		virtscsi_cmd_pool = NULL;
-	}
-	if (virtscsi_cmd_cache) {
-		kmem_cache_destroy(virtscsi_cmd_cache);
-		virtscsi_cmd_cache = NULL;
-	}
-	if (virtscsi_wq) {
-		destroy_workqueue(virtscsi_wq);
-		virtscsi_wq = NULL;
-	}
+register_failed:
+	mempool_destroy(virtscsi_cmd_pool);
+mempool_failed:
+	kmem_cache_destroy(virtscsi_cmd_cache);
+kmem_cache_failed:
+	destroy_workqueue(virtscsi_wq);
+workqueue_failed:
 	return ret;
 }
 
-- 
1.8.0

Re: [PATCH 0/5] virtio-scsi: create a separate workqueue

[Asias He]

On Wed, Nov 7, 2012 at 6:18 PM, Wanlong Gao <gaowanlong@cn.fujitsu.com> wrote:
> patch 1-3,5 are some cleanups.
> patch 4: create a separate work queue for virtio-scsi
> to improve the performance, I tested with tmpfs backed
> disk, the config file is like below,
> [global]
> bsrange=4k-64k
> ioengine=libaio
> direct=1
> iodepth=4
> loops=10
> size=64M
>
> Before:
> Disk stats (read/write):
>   sda: ios=6547/5275, merge=37/39, ticks=3144/2645, in_queue=5780, util=81.62%
>   sdb: ios=6542/5277, merge=54/59, ticks=3234/3143, in_queue=6364, util=84.50%
>   sdc: ios=6532/5244, merge=65/62, ticks=3440/3076, in_queue=6505, util=86.83%

The result is about the disk stat. Can you please post the iops and
the bw instead?

> After:
> Disk stats (read/write):
>   sda: ios=6349/5318, merge=77/89, ticks=4820/3169, in_queue=7991, util=90.62%
>   sdb: ios=6364/5358, merge=39/53, ticks=3810/2816, in_queue=6615, util=84.63%
>   sdc: ios=6458/5352, merge=74/47, ticks=4425/3048, in_queue=7459, util=88.41%
>
>
> Wanlong Gao (5):
>   virtio-scsi: remove the useless assignment
>   virtio-scsi: remove the needless variable gfp_mask
>   virtio-scsi: use pr_err instead of printk
>   virtio-scsi: create a separate work queue for virtio-scsi
>   virtio-scsi: tidy up the goto label in init()
>
>
>
> Cc: James E.J. Bottomley <JBottomley@parallels.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Rusty Russell <rusty@rustcorp.com.au>
> Cc: linux-scsi@vger.kernel.org
> Cc: kvm@vger.kernel.org
>  drivers/scsi/virtio_scsi.c | 43 +++++++++++++++++++++++--------------------
>  1 file changed, 23 insertions(+), 20 deletions(-)
>
> --
> 1.8.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Asias He

Re: [PATCH 0/5] virtio-scsi: create a separate workqueue

[Wanlong Gao]

On 11/07/2012 08:23 PM, Asias He wrote:
> On Wed, Nov 7, 2012 at 6:18 PM, Wanlong Gao <gaowanlong@cn.fujitsu.com> wrote:
>> patch 1-3,5 are some cleanups.
>> patch 4: create a separate work queue for virtio-scsi
>> to improve the performance, I tested with tmpfs backed
>> disk, the config file is like below,
>> [global]
>> bsrange=4k-64k
>> ioengine=libaio
>> direct=1
>> iodepth=4
>> loops=10
>> size=64M
>>
>> Before:
>> Disk stats (read/write):
>>   sda: ios=6547/5275, merge=37/39, ticks=3144/2645, in_queue=5780, util=81.62%
>>   sdb: ios=6542/5277, merge=54/59, ticks=3234/3143, in_queue=6364, util=84.50%
>>   sdc: ios=6532/5244, merge=65/62, ticks=3440/3076, in_queue=6505, util=86.83%
> 
> The result is about the disk stat. Can you please post the iops and
> the bw instead?

Sure,

before:
  write: io=589788KB, bw=122211KB/s, iops=3584 , runt=  4826msec
  bw (KB/s)  : min=24208, max=116272, per=57.47%, avg=70236.40, stdev=35496.33  
  WRITE: io=589788KB, aggrb=122210KB/s, minb=122210KB/s, maxb=122210KB/s, mint=4826msec, maxt=4826msec 

after:
  write: io=589788KB, bw=124244KB/s, iops=3530 , runt=  4747msec
  bw (KB/s)  : min=63496, max=117943, per=73.39%, avg=91184.20, stdev=20062.42
  WRITE: io=589788KB, aggrb=124244KB/s, minb=124244KB/s, maxb=124244KB/s, mint=4747msec, maxt=4747msec


Thanks,
Wanlong Gao


> 
>> After:
>> Disk stats (read/write):
>>   sda: ios=6349/5318, merge=77/89, ticks=4820/3169, in_queue=7991, util=90.62%
>>   sdb: ios=6364/5358, merge=39/53, ticks=3810/2816, in_queue=6615, util=84.63%
>>   sdc: ios=6458/5352, merge=74/47, ticks=4425/3048, in_queue=7459, util=88.41%
>>
>>
>> Wanlong Gao (5):
>>   virtio-scsi: remove the useless assignment
>>   virtio-scsi: remove the needless variable gfp_mask
>>   virtio-scsi: use pr_err instead of printk
>>   virtio-scsi: create a separate work queue for virtio-scsi
>>   virtio-scsi: tidy up the goto label in init()
>>
>>
>>
>> Cc: James E.J. Bottomley <JBottomley@parallels.com>
>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>> Cc: Rusty Russell <rusty@rustcorp.com.au>
>> Cc: linux-scsi@vger.kernel.org
>> Cc: kvm@vger.kernel.org
>>  drivers/scsi/virtio_scsi.c | 43 +++++++++++++++++++++++--------------------
>>  1 file changed, 23 insertions(+), 20 deletions(-)
>>
>> --
>> 1.8.0
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe kvm" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 
> 

Re: [PATCH 0/5] virtio-scsi: create a separate workqueue

[Paolo Bonzini]

> patch 1-3,5 are some cleanups.
> patch 4: create a separate work queue for virtio-scsi
> to improve the performance, I tested with tmpfs backed
> disk, the config file is like below,

I think something else caused the improvement, because the code you
touched (complete_event) shouldn't ever run during a normal benchmark.
It's only used for hotplug/hot-unplug.

Nevertheless, I'll queue the cleanup patches.  Thanks for those.

Paolo

> [global]
> bsrange=4k-64k
> ioengine=libaio
> direct=1
> iodepth=4
> loops=10
> size=64M
> 
> Before:
> Disk stats (read/write):
>   sda: ios=6547/5275, merge=37/39, ticks=3144/2645, in_queue=5780,
>   util=81.62%
>   sdb: ios=6542/5277, merge=54/59, ticks=3234/3143, in_queue=6364,
>   util=84.50%
>   sdc: ios=6532/5244, merge=65/62, ticks=3440/3076, in_queue=6505,
>   util=86.83%
> 
> After:
> Disk stats (read/write):
>   sda: ios=6349/5318, merge=77/89, ticks=4820/3169, in_queue=7991,
>   util=90.62%
>   sdb: ios=6364/5358, merge=39/53, ticks=3810/2816, in_queue=6615,
>   util=84.63%
>   sdc: ios=6458/5352, merge=74/47, ticks=4425/3048, in_queue=7459,
>   util=88.41%
> 
> 
> Wanlong Gao (5):
>   virtio-scsi: remove the useless assignment
>   virtio-scsi: remove the needless variable gfp_mask
>   virtio-scsi: use pr_err instead of printk
>   virtio-scsi: create a separate work queue for virtio-scsi
>   virtio-scsi: tidy up the goto label in init()
> 
> 
> 
> Cc: James E.J. Bottomley <JBottomley@parallels.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Rusty Russell <rusty@rustcorp.com.au>
> Cc: linux-scsi@vger.kernel.org
> Cc: kvm@vger.kernel.org
>  drivers/scsi/virtio_scsi.c | 43
>  +++++++++++++++++++++++--------------------
>  1 file changed, 23 insertions(+), 20 deletions(-)
> 
> --
> 1.8.0
> 
> 

Re: [PATCH 0/5] virtio-scsi: create a separate workqueue

[Wanlong Gao]

On 11/08/2012 06:12 AM, Paolo Bonzini wrote:
>> patch 1-3,5 are some cleanups.
>> patch 4: create a separate work queue for virtio-scsi
>> to improve the performance, I tested with tmpfs backed
>> disk, the config file is like below,
> 
> I think something else caused the improvement, because the code you
> touched (complete_event) shouldn't ever run during a normal benchmark.
> It's only used for hotplug/hot-unplug.
> 
> Nevertheless, I'll queue the cleanup patches.  Thanks for those.

Oops, got it, I will investigate more. Thank you for teaching me.

Regards,
Wanlong Gao