xen: xen-pciback: Remove create_workqueue

PATCH Applied Bhaktipriya Shridhar 3 participants 9 messages Jun 1, 2016 – Jun 29, 2016 Xen hypervisor interface

Share & tools

Bhaktipriya Shridhar

author

2016-06-01 14:15 UTC #0 lore raw

PATCH v2 1/1 xen: xen-pciback: Remove create_workqueue

System workqueues have been able to handle high level of concurrency for a long time now and there's no reason to use dedicated workqueues just to gain concurrency. Replace dedicated xen_pcibk_wq with the use of system_wq.

Unlike a dedicated per-cpu workqueue created with create_workqueue(), system_wq allows multiple work items to overlap executions even on the same CPU; however, a per-cpu workqueue doesn't have any CPU locality or global ordering guarantees unless the target CPU is explicitly specified and thus the increase of local concurrency shouldn't make any difference.

Since the work items could be pending, flush_work() has been used in xen_pcibk_disconnect(). xen_pcibk_xenbus_remove() calls free_pdev() which in turn calls xen_pcibk_disconnect() for every pdev to ensure that there is no pending task while disconnecting the driver.

Signed-off-by Bhaktipriya Shridhar <[email protected]>

---
Changes in v2:
-Changed cancel_work_sync to flush_work
-Changed commit description

Feedback to update a comment was received in v1 from David Vrabel. It has not be included in v2 since some clarification was required. Will include it in v3 once the details about the content and the placement of the comment are received.

 drivers/xen/xen-pciback/pciback.h     |  1 -
 drivers/xen/xen-pciback/pciback_ops.c |  2 +-
 drivers/xen/xen-pciback/xenbus.c      | 10 +---------
 3 files changed, 2 insertions(+), 11 deletions(-)

drivers/xen/xen-pciback/pciback.h +0 -1

index 4d529f3..7af369b6 100644
@@ -55,7 +55,6 @@ struct xen_pcibk_dev_data {
 
 /* Used by XenBus and xen_pcibk_ops.c */
 extern wait_queue_head_t xen_pcibk_aer_wait_queue;
-extern struct workqueue_struct *xen_pcibk_wq;
 /* Used by pcistub.c and conf_space_quirks.c */
 extern struct list_head xen_pcibk_quirks;

drivers/xen/xen-pciback/pciback_ops.c +1 -1

index 2f19dd7..f8c7775 100644
@@ -310,7 +310,7 @@ void xen_pcibk_test_and_schedule_op(struct xen_pcibk_device *pdev)
 	 * already processing a request */
 	if (test_bit(_XEN_PCIF_active, (unsigned long *)&pdev->sh_info->flags)
 	    && !test_and_set_bit(_PDEVF_op_active, &pdev->flags)) {
-		queue_work(xen_pcibk_wq, &pdev->op_work);
+		schedule_work(&pdev->op_work);
 	}
 	/*_XEN_PCIB_active should have been cleared by pcifront. And also make
 	sure xen_pcibk is waiting for ack by checking _PCIB_op_pending*/

drivers/xen/xen-pciback/xenbus.c +1 -9

index c252eb3..5ce878c 100644
@@ -17,7 +17,6 @@
 #include "pciback.h"
 
 #define INVALID_EVTCHN_IRQ  (-1)
-struct workqueue_struct *xen_pcibk_wq;
 
 static bool __read_mostly passthrough;
 module_param(passthrough, bool, S_IRUGO);

@@ -76,8 +75,7 @@ static void xen_pcibk_disconnect(struct xen_pcibk_device *pdev)
 	/* If the driver domain started an op, make sure we complete it
 	 * before releasing the shared memory */
 
-	/* Note, the workqueue does not use spinlocks at all.*/
-	flush_workqueue(xen_pcibk_wq);
+	flush_work(&pdev->op_work);
 
 	if (pdev->sh_info != NULL) {
 		xenbus_unmap_ring_vfree(pdev->xdev, pdev->sh_info);

@@ -733,11 +731,6 @@ const struct xen_pcibk_backend *__read_mostly xen_pcibk_backend;
 
 int __init xen_pcibk_xenbus_register(void)
 {
-	xen_pcibk_wq = create_workqueue("xen_pciback_workqueue");
-	if (!xen_pcibk_wq) {
-		pr_err("%s: create xen_pciback_workqueue failed\n", __func__);
-		return -EFAULT;
-	}
 	xen_pcibk_backend = &xen_pcibk_vpci_backend;
 	if (passthrough)
 		xen_pcibk_backend = &xen_pcibk_passthrough_backend;

@@ -747,6 +740,5 @@ int __init xen_pcibk_xenbus_register(void)
 
 void __exit xen_pcibk_xenbus_unregister(void)
 {
-	destroy_workqueue(xen_pcibk_wq);
 	xenbus_unregister_driver(&xen_pcibk_driver);
 }

--
2.1.4

David Vrabel

2016-06-01 14:53 UTC #1 lore raw

Quoted 23 lines · Bhaktipriya Shridhar

System workqueues have been able to handle high level of concurrency for a long time now and there's no reason to use dedicated workqueues just to gain concurrency. Replace dedicated xen_pcibk_wq with the use of system_wq.
Unlike a dedicated per-cpu workqueue created with create_workqueue(), system_wq allows multiple work items to overlap executions even on the same CPU; however, a per-cpu workqueue doesn't have any CPU locality or global ordering guarantees unless the target CPU is explicitly specified and thus the increase of local concurrency shouldn't make any difference.
Since the work items could be pending, flush_work() has been used in xen_pcibk_disconnect(). xen_pcibk_xenbus_remove() calls free_pdev() which in turn calls xen_pcibk_disconnect() for every pdev to ensure that there is no pending task while disconnecting the driver.
Signed-off-by Bhaktipriya Shridhar <[email protected]>
---
Changes in v2:
-Changed cancel_work_sync to flush_work
-Changed commit description

Quoted 4 lines

Feedback to update a comment was received in v1 from David Vrabel. It has not be included in v2 since some clarification was required. Will include it in v3 once the details about the content and the placement of the comment are received.

The comment needed updating iff this continued to use cancel_work_sync() but since it's using flush_work() the comment is fine.

David

David Vrabel

2016-06-01 14:53 UTC #2 lore raw

In reply to Bhaktipriya Shridhar

Quoted 23 lines · Bhaktipriya Shridhar

System workqueues have been able to handle high level of concurrency for a long time now and there's no reason to use dedicated workqueues just to gain concurrency. Replace dedicated xen_pcibk_wq with the use of system_wq.
Unlike a dedicated per-cpu workqueue created with create_workqueue(), system_wq allows multiple work items to overlap executions even on the same CPU; however, a per-cpu workqueue doesn't have any CPU locality or global ordering guarantees unless the target CPU is explicitly specified and thus the increase of local concurrency shouldn't make any difference.
Since the work items could be pending, flush_work() has been used in xen_pcibk_disconnect(). xen_pcibk_xenbus_remove() calls free_pdev() which in turn calls xen_pcibk_disconnect() for every pdev to ensure that there is no pending task while disconnecting the driver.
Signed-off-by Bhaktipriya Shridhar <[email protected]>
---
Changes in v2:
-Changed cancel_work_sync to flush_work
-Changed commit description

Quoted 4 lines

Feedback to update a comment was received in v1 from David Vrabel. It has not be included in v2 since some clarification was required. Will include it in v3 once the details about the content and the placement of the comment are received.

The comment needed updating iff this continued to use cancel_work_sync() but since it's using flush_work() the comment is fine.

David

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

Tejun Heo

Acked-by on v2 1/1

2016-06-01 15:45 UTC #3 lore raw

In reply to Bhaktipriya Shridhar

Quoted 12 lines · Bhaktipriya Shridhar

System workqueues have been able to handle high level of concurrency for a long time now and there's no reason to use dedicated workqueues just to gain concurrency. Replace dedicated xen_pcibk_wq with the use of system_wq.
Unlike a dedicated per-cpu workqueue created with create_workqueue(), system_wq allows multiple work items to overlap executions even on the same CPU; however, a per-cpu workqueue doesn't have any CPU locality or global ordering guarantees unless the target CPU is explicitly specified and thus the increase of local concurrency shouldn't make any difference.

Quoted 6 lines

Since the work items could be pending, flush_work() has been used in xen_pcibk_disconnect(). xen_pcibk_xenbus_remove() calls free_pdev() which in turn calls xen_pcibk_disconnect() for every pdev to ensure that there is no pending task while disconnecting the driver.
Signed-off-by Bhaktipriya Shridhar <[email protected]>

Acked-by Tejun Heo <[email protected]>

Thanks.

Signature · 1 line

tejun

Bhaktipriya Shridhar

author

2016-06-28 16:47 UTC #4 lore raw

Ping!
Thanks,
Bhaktipriya

Quoted 18 lines · Tejun Heo

Quoted 17 lines · Bhaktipriya Shridhar
System workqueues have been able to handle high level of concurrency for a long time now and there's no reason to use dedicated workqueues just to gain concurrency. Replace dedicated xen_pcibk_wq with the use of system_wq.
Unlike a dedicated per-cpu workqueue created with create_workqueue(), system_wq allows multiple work items to overlap executions even on the same CPU; however, a per-cpu workqueue doesn't have any CPU locality or global ordering guarantees unless the target CPU is explicitly specified and thus the increase of local concurrency shouldn't make any difference.
Since the work items could be pending, flush_work() has been used in xen_pcibk_disconnect(). xen_pcibk_xenbus_remove() calls free_pdev() which in turn calls xen_pcibk_disconnect() for every pdev to ensure that there is no pending task while disconnecting the driver.

Quoted 8 lines

Quoted 1 line
Signed-off-by Bhaktipriya Shridhar <[email protected]>
Acked-by Tejun Heo <[email protected]>
Thanks.
--
tejun

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

Bhaktipriya Shridhar

author

2016-06-28 16:47 UTC #5 lore raw

In reply to Tejun Heo

Ping!
Thanks,
Bhaktipriya

Quoted 26 lines · Tejun Heo

Quoted 12 lines · Bhaktipriya Shridhar
System workqueues have been able to handle high level of concurrency for a long time now and there's no reason to use dedicated workqueues just to gain concurrency. Replace dedicated xen_pcibk_wq with the use of system_wq.
Unlike a dedicated per-cpu workqueue created with create_workqueue(), system_wq allows multiple work items to overlap executions even on the same CPU; however, a per-cpu workqueue doesn't have any CPU locality or global ordering guarantees unless the target CPU is explicitly specified and thus the increase of local concurrency shouldn't make any difference.
Quoted 6 lines
Since the work items could be pending, flush_work() has been used in xen_pcibk_disconnect(). xen_pcibk_xenbus_remove() calls free_pdev() which in turn calls xen_pcibk_disconnect() for every pdev to ensure that there is no pending task while disconnecting the driver.
Signed-off-by Bhaktipriya Shridhar <[email protected]>
Acked-by Tejun Heo <[email protected]>
Thanks.
--
tejun

Tejun Heo

Acked-by on v2 1/1

2016-06-01 15:45 UTC #6 lore raw

In reply to Bhaktipriya Shridhar

Quoted 12 lines · Bhaktipriya Shridhar

System workqueues have been able to handle high level of concurrency for a long time now and there's no reason to use dedicated workqueues just to gain concurrency. Replace dedicated xen_pcibk_wq with the use of system_wq.
Unlike a dedicated per-cpu workqueue created with create_workqueue(), system_wq allows multiple work items to overlap executions even on the same CPU; however, a per-cpu workqueue doesn't have any CPU locality or global ordering guarantees unless the target CPU is explicitly specified and thus the increase of local concurrency shouldn't make any difference.

Quoted 6 lines

Since the work items could be pending, flush_work() has been used in xen_pcibk_disconnect(). xen_pcibk_xenbus_remove() calls free_pdev() which in turn calls xen_pcibk_disconnect() for every pdev to ensure that there is no pending task while disconnecting the driver.
Signed-off-by Bhaktipriya Shridhar <[email protected]>

Acked-by Tejun Heo <[email protected]>

Thanks.

Signature · 6 lines

tejun

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

David Vrabel

Applied

2016-06-29 12:49 UTC #7 lore raw

In reply to Bhaktipriya Shridhar

Quoted 12 lines · Bhaktipriya Shridhar

System workqueues have been able to handle high level of concurrency for a long time now and there's no reason to use dedicated workqueues just to gain concurrency. Replace dedicated xen_pcibk_wq with the use of system_wq.
Unlike a dedicated per-cpu workqueue created with create_workqueue(), system_wq allows multiple work items to overlap executions even on the same CPU; however, a per-cpu workqueue doesn't have any CPU locality or global ordering guarantees unless the target CPU is explicitly specified and thus the increase of local concurrency shouldn't make any difference.

Quoted 4 lines

Since the work items could be pending, flush_work() has been used in xen_pcibk_disconnect(). xen_pcibk_xenbus_remove() calls free_pdev() which in turn calls xen_pcibk_disconnect() for every pdev to ensure that there is no pending task while disconnecting the driver.

Applied to for-linus-4.8, thanks.

David

David Vrabel

Applied

2016-06-29 12:49 UTC #8 lore raw

In reply to Bhaktipriya Shridhar

Quoted 12 lines · Bhaktipriya Shridhar

System workqueues have been able to handle high level of concurrency for a long time now and there's no reason to use dedicated workqueues just to gain concurrency. Replace dedicated xen_pcibk_wq with the use of system_wq.
Unlike a dedicated per-cpu workqueue created with create_workqueue(), system_wq allows multiple work items to overlap executions even on the same CPU; however, a per-cpu workqueue doesn't have any CPU locality or global ordering guarantees unless the target CPU is explicitly specified and thus the increase of local concurrency shouldn't make any difference.

Quoted 4 lines

Since the work items could be pending, flush_work() has been used in xen_pcibk_disconnect(). xen_pcibk_xenbus_remove() calls free_pdev() which in turn calls xen_pcibk_disconnect() for every pdev to ensure that there is no pending task while disconnecting the driver.

Applied to for-linus-4.8, thanks.

David

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel