Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1408156 > unrolled thread

[PATCH] xen: xen-pciback: Remove create_workqueue

Back to article view | Back to linux.kernel

Contents

  [PATCH] xen: xen-pciback: Remove create_workqueue Bhaktipriya Shridhar <bhaktipriya96@gmail.com> - 2016-05-27 18:00 +0200
    Re: [PATCH] xen: xen-pciback: Remove create_workqueue Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> - 2016-05-27 18:10 +0200
      Re: [PATCH] xen: xen-pciback: Remove create_workqueue Tejun Heo <tj@kernel.org> - 2016-05-27 18:10 +0200
        Re: [PATCH] xen: xen-pciback: Remove create_workqueue Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> - 2016-05-27 18:40 +0200
          Re: [PATCH] xen: xen-pciback: Remove create_workqueue David Vrabel <david.vrabel@citrix.com> - 2016-05-31 12:20 +0200

#1408156 — [PATCH] xen: xen-pciback: Remove create_workqueue

With concurrency managed workqueues, use of dedicated workqueues can be
replaced by using system_wq. Drop host->intr_wq by using
system_wq.

Since there is only a single work item, increase of concurrency level by
switching to system_wq should not break anything.

cancel_work_sync() has been used in xen_pcibk_disconnect() to ensure that
work item is not pending or executing by the time exit path runs.

Signed-off-by: Bhaktipriya Shridhar <bhaktipriya96@gmail.com>
---
 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(-)

diff --git a/drivers/xen/xen-pciback/pciback.h b/drivers/xen/xen-pciback/pciback.h
index 4d529f3..7af369b6 100644
--- a/drivers/xen/xen-pciback/pciback.h
+++ b/drivers/xen/xen-pciback/pciback.h
@@ -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;

diff --git a/drivers/xen/xen-pciback/pciback_ops.c b/drivers/xen/xen-pciback/pciback_ops.c
index 2f19dd7..f8c7775 100644
--- a/drivers/xen/xen-pciback/pciback_ops.c
+++ b/drivers/xen/xen-pciback/pciback_ops.c
@@ -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*/
diff --git a/drivers/xen/xen-pciback/xenbus.c b/drivers/xen/xen-pciback/xenbus.c
index c252eb3..f70a8e1 100644
--- a/drivers/xen/xen-pciback/xenbus.c
+++ b/drivers/xen/xen-pciback/xenbus.c
@@ -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);
+	cancel_work_sync(&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

[toc] | [next] | [standalone]

#1408161

On Fri, May 27, 2016 at 09:24:11PM +0530, Bhaktipriya Shridhar wrote:
> With concurrency managed workqueues, use of dedicated workqueues can be
> replaced by using system_wq. Drop host->intr_wq by using
> system_wq.
> 
> Since there is only a single work item, increase of concurrency level by
> switching to system_wq should not break anything.

_should_ not? Hehe. I presume this has not been tested?
> 
> cancel_work_sync() has been used in xen_pcibk_disconnect() to ensure that
> work item is not pending or executing by the time exit path runs.
> 
> Signed-off-by: Bhaktipriya Shridhar <bhaktipriya96@gmail.com>
> ---
>  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(-)
> 
> diff --git a/drivers/xen/xen-pciback/pciback.h b/drivers/xen/xen-pciback/pciback.h
> index 4d529f3..7af369b6 100644
> --- a/drivers/xen/xen-pciback/pciback.h
> +++ b/drivers/xen/xen-pciback/pciback.h
> @@ -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;
> 
> diff --git a/drivers/xen/xen-pciback/pciback_ops.c b/drivers/xen/xen-pciback/pciback_ops.c
> index 2f19dd7..f8c7775 100644
> --- a/drivers/xen/xen-pciback/pciback_ops.c
> +++ b/drivers/xen/xen-pciback/pciback_ops.c
> @@ -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*/
> diff --git a/drivers/xen/xen-pciback/xenbus.c b/drivers/xen/xen-pciback/xenbus.c
> index c252eb3..f70a8e1 100644
> --- a/drivers/xen/xen-pciback/xenbus.c
> +++ b/drivers/xen/xen-pciback/xenbus.c
> @@ -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);
> +	cancel_work_sync(&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
> 

[toc] | [prev] | [next] | [standalone]

#1408162

Hello,

On Fri, May 27, 2016 at 12:01:14PM -0400, Konrad Rzeszutek Wilk wrote:
> On Fri, May 27, 2016 at 09:24:11PM +0530, Bhaktipriya Shridhar wrote:
> > With concurrency managed workqueues, use of dedicated workqueues can be
> > replaced by using system_wq. Drop host->intr_wq by using
                                      ^
				      xen_pcibk_wq
> > system_wq.
> > 
> > Since there is only a single work item, increase of concurrency level by
> > switching to system_wq should not break anything.
> 
> _should_ not? Hehe. I presume this has not been tested?

Yeah, this is a part of sweeping conversions and it's challenging (and
often impossible for specific drivers) to setup test environments.
xen isn't as bad but can still be a pretty specialized setup.  The
conversions aren't high risk and shouldn't be too difficult to root
cause when something goes south.  We'd greatly appreciate any helps
with reviewing and testing.

> > cancel_work_sync() has been used in xen_pcibk_disconnect() to ensure that
> > work item is not pending or executing by the time exit path runs.
> > 
> > Signed-off-by: Bhaktipriya Shridhar <bhaktipriya96@gmail.com>
> > @@ -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);
> > +	cancel_work_sync(&pdev->op_work);

Should it be flush_work() instead?  Is it okay for a pdev->op_work to
be queued and canceled without actually getting executed?

Thanks.

-- 
tejun

[toc] | [prev] | [next] | [standalone]

#1408177

On Fri, May 27, 2016 at 12:08:22PM -0400, Tejun Heo wrote:
> Hello,
> 
> On Fri, May 27, 2016 at 12:01:14PM -0400, Konrad Rzeszutek Wilk wrote:
> > On Fri, May 27, 2016 at 09:24:11PM +0530, Bhaktipriya Shridhar wrote:
> > > With concurrency managed workqueues, use of dedicated workqueues can be
> > > replaced by using system_wq. Drop host->intr_wq by using
>                                       ^
> 				      xen_pcibk_wq
> > > system_wq.
> > > 
> > > Since there is only a single work item, increase of concurrency level by
> > > switching to system_wq should not break anything.
> > 
> > _should_ not? Hehe. I presume this has not been tested?
> 
> Yeah, this is a part of sweeping conversions and it's challenging (and
> often impossible for specific drivers) to setup test environments.
> xen isn't as bad but can still be a pretty specialized setup.  The
> conversions aren't high risk and shouldn't be too difficult to root
> cause when something goes south.  We'd greatly appreciate any helps
> with reviewing and testing.
> 
> > > cancel_work_sync() has been used in xen_pcibk_disconnect() to ensure that
> > > work item is not pending or executing by the time exit path runs.
> > > 
> > > Signed-off-by: Bhaktipriya Shridhar <bhaktipriya96@gmail.com>
> > > @@ -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);
> > > +	cancel_work_sync(&pdev->op_work);
> 
> Should it be flush_work() instead?  Is it okay for a pdev->op_work to
> be queued and canceled without actually getting executed?

It should really flush them. The comment above says so, but in reality it
does not matter that much as we tearing down the communication. As long as
the pdev->op_work either completes or is never executed - we are fine.

> 
> Thanks.
> 
> -- 
> tejun

[toc] | [prev] | [next] | [standalone]

#1410077

On 27/05/16 17:32, Konrad Rzeszutek Wilk wrote:
> On Fri, May 27, 2016 at 12:08:22PM -0400, Tejun Heo wrote:
>> Hello,
>>
>> On Fri, May 27, 2016 at 12:01:14PM -0400, Konrad Rzeszutek Wilk wrote:
>>> On Fri, May 27, 2016 at 09:24:11PM +0530, Bhaktipriya Shridhar wrote:
>>>> With concurrency managed workqueues, use of dedicated workqueues can be
>>>> replaced by using system_wq. Drop host->intr_wq by using
>>                                       ^
>> 				      xen_pcibk_wq
>>>> system_wq.
>>>>
>>>> Since there is only a single work item, increase of concurrency level by
>>>> switching to system_wq should not break anything.
>>>
>>> _should_ not? Hehe. I presume this has not been tested?
>>
>> Yeah, this is a part of sweeping conversions and it's challenging (and
>> often impossible for specific drivers) to setup test environments.
>> xen isn't as bad but can still be a pretty specialized setup.  The
>> conversions aren't high risk and shouldn't be too difficult to root
>> cause when something goes south.  We'd greatly appreciate any helps
>> with reviewing and testing.
>>
>>>> cancel_work_sync() has been used in xen_pcibk_disconnect() to ensure that
>>>> work item is not pending or executing by the time exit path runs.
>>>>
>>>> Signed-off-by: Bhaktipriya Shridhar <bhaktipriya96@gmail.com>
>>>> @@ -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);
>>>> +	cancel_work_sync(&pdev->op_work);
>>
>> Should it be flush_work() instead?  Is it okay for a pdev->op_work to
>> be queued and canceled without actually getting executed?
> 
> It should really flush them. The comment above says so, but in reality it
> does not matter that much as we tearing down the communication. As long as
> the pdev->op_work either completes or is never executed - we are fine.

The comment should be updated to reflect this.

David

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web