[PATCH 0/2] Remove improper workqueue usage

[PATCH 0/2] Remove improper workqueue usage

[Bhaktipriya Shridhar]

This patch set fixes the improper usage of the workqueue API.
This includes dropping the freeing of workqueue and removing the deprecated
create_singlethread_workqueue instance.

Bhaktipriya Shridhar (2):
  [media] cx25821: Drop Freeing of Workqueue
  [media] cx25821: Remove deprecated create_singlethread_workqueue

 drivers/media/pci/cx25821/cx25821-audio-upstream.c | 13 ++-----------
 drivers/media/pci/cx25821/cx25821.h                |  1 -
 2 files changed, 2 insertions(+), 12 deletions(-)

--
2.1.4

[PATCH 1/2] [media] cx25821: Drop Freeing of Workqueue

[Bhaktipriya Shridhar]

Workqueues shouldn't be freed. destroy_workqueue should be used instead.
destroy_workqueue safely destroys a workqueue and ensures that all pending
work items are done before destroying the workqueue.

Signed-off-by: Bhaktipriya Shridhar <bhaktipriya96@gmail.com>
---
 drivers/media/pci/cx25821/cx25821-audio-upstream.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/pci/cx25821/cx25821-audio-upstream.c b/drivers/media/pci/cx25821/cx25821-audio-upstream.c
index 68dbc2d..05bd957 100644
--- a/drivers/media/pci/cx25821/cx25821-audio-upstream.c
+++ b/drivers/media/pci/cx25821/cx25821-audio-upstream.c
@@ -242,7 +242,7 @@ void cx25821_stop_upstream_audio(struct cx25821_dev *dev)
 	dev->_audioframe_count = 0;
 	dev->_audiofile_status = END_OF_FILE;

-	kfree(dev->_irq_audio_queues);
+	destroy_workqueue(dev->_irq_audio_queues);
 	dev->_irq_audio_queues = NULL;

 	kfree(dev->_audiofilename);
--
2.1.4

[PATCH 2/2] [media] cx25821: Remove deprecated create_singlethread_workqueue

[Bhaktipriya Shridhar]

The workqueue "_irq_audio_queues" runs the audio upstream handler.
It has a single work item(&dev->_audio_work_entry) and hence doesn't
require ordering. Also, it is not being used on a memory reclaim path.
Hence, the singlethreaded workqueue has been replaced with the use of
system_wq.

System workqueues have been able to handle high level of concurrency
for a long time now and hence it's not required to have a singlethreaded
workqueue just to gain concurrency. Unlike a dedicated per-cpu workqueue
created with create_singlethread_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
guarantee unless the target CPU is explicitly specified and thus the
increase of local concurrency shouldn't make any difference.

Signed-off-by: Bhaktipriya Shridhar <bhaktipriya96@gmail.com>
---
 drivers/media/pci/cx25821/cx25821-audio-upstream.c | 11 +----------
 drivers/media/pci/cx25821/cx25821.h                |  1 -
 2 files changed, 1 insertion(+), 11 deletions(-)

diff --git a/drivers/media/pci/cx25821/cx25821-audio-upstream.c b/drivers/media/pci/cx25821/cx25821-audio-upstream.c
index 05bd957..1a86dff 100644
--- a/drivers/media/pci/cx25821/cx25821-audio-upstream.c
+++ b/drivers/media/pci/cx25821/cx25821-audio-upstream.c
@@ -446,8 +446,7 @@ static int cx25821_audio_upstream_irq(struct cx25821_dev *dev, int chan_num,

 			dev->_audioframe_index = dev->_last_index_irq;

-			queue_work(dev->_irq_audio_queues,
-				   &dev->_audio_work_entry);
+			schedule_work(&dev->_audio_work_entry);
 		}

 		if (dev->_is_first_audio_frame) {
@@ -639,14 +638,6 @@ int cx25821_audio_upstream_init(struct cx25821_dev *dev, int channel_select)

 	/* Work queue */
 	INIT_WORK(&dev->_audio_work_entry, cx25821_audioups_handler);
-	dev->_irq_audio_queues =
-	    create_singlethread_workqueue("cx25821_audioworkqueue");
-
-	if (!dev->_irq_audio_queues) {
-		printk(KERN_DEBUG
-			pr_fmt("ERROR: create_singlethread_workqueue() for Audio FAILED!\n"));
-		return -ENOMEM;
-	}

 	dev->_last_index_irq = 0;
 	dev->_audio_is_running = 0;
diff --git a/drivers/media/pci/cx25821/cx25821.h b/drivers/media/pci/cx25821/cx25821.h
index a513b68..c813d88 100644
--- a/drivers/media/pci/cx25821/cx25821.h
+++ b/drivers/media/pci/cx25821/cx25821.h
@@ -294,7 +294,6 @@ struct cx25821_dev {
 	u32 audio_upstream_riscbuf_size;
 	u32 audio_upstream_databuf_size;
 	int _audioframe_index;
-	struct workqueue_struct *_irq_audio_queues;
 	struct work_struct _audio_work_entry;
 	char *input_audiofilename;

--
2.1.4

Re: [PATCH 1/2] [media] cx25821: Drop Freeing of Workqueue

[Tejun Heo]

On Sat, Jul 16, 2016 at 02:41:49PM +0530, Bhaktipriya Shridhar wrote:
> Workqueues shouldn't be freed. destroy_workqueue should be used instead.
> destroy_workqueue safely destroys a workqueue and ensures that all pending
> work items are done before destroying the workqueue.
> 
> Signed-off-by: Bhaktipriya Shridhar <bhaktipriya96@gmail.com>

Acked-by: Tejun Heo <tj@kernel.org>

This needs to be marked for stable.

Thanks.

-- 
tejun

Re: [PATCH 2/2] [media] cx25821: Remove deprecated create_singlethread_workqueue

[Tejun Heo]

On Sat, Jul 16, 2016 at 02:43:20PM +0530, Bhaktipriya Shridhar wrote:
> The workqueue "_irq_audio_queues" runs the audio upstream handler.
> It has a single work item(&dev->_audio_work_entry) and hence doesn't
> require ordering. Also, it is not being used on a memory reclaim path.
> Hence, the singlethreaded workqueue has been replaced with the use of
> system_wq.
> 
> System workqueues have been able to handle high level of concurrency
> for a long time now and hence it's not required to have a singlethreaded
> workqueue just to gain concurrency. Unlike a dedicated per-cpu workqueue
> created with create_singlethread_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
> guarantee unless the target CPU is explicitly specified and thus the
> increase of local concurrency shouldn't make any difference.

The patch seems to be missing update to wq destruction path.

Thanks.

-- 
tejun