[11/32] v4l/cx18: update workqueue usage

  With cmwq, there's no reason to use separate out_work_queue.  Drop it
and use system_wq instead.  The in_work_queue needs to be ordered so
can't use one of the system wqs; however, as it isn't used to reclaim
memory, allocate the workqueue with alloc_ordered_workqueue() without
WQ_MEM_RECLAIM.

Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Andy Walls <awalls@md.metrocast.net>
Cc: linux-media@vger.kernel.org
---
Only compile tested.  Please feel free to take it into the subsystem
tree or simply ack - I'll route it through the wq tree.

Thanks.

 drivers/media/video/cx18/cx18-driver.c  |   24 ++----------------------
 drivers/media/video/cx18/cx18-driver.h  |    3 ---
 drivers/media/video/cx18/cx18-streams.h |    3 +--
 3 files changed, 3 insertions(+), 27 deletions(-)
  

On Mon, 2011-01-03 at 14:49 +0100, Tejun Heo wrote:
> With cmwq, there's no reason to use separate out_work_queue.  Drop it
> and use system_wq instead.  The in_work_queue needs to be ordered so
> can't use one of the system wqs; however, as it isn't used to reclaim
> memory, allocate the workqueue with alloc_ordered_workqueue() without
> WQ_MEM_RECLAIM.
> 
> Signed-off-by: Tejun Heo <tj@kernel.org>
> Cc: Andy Walls <awalls@md.metrocast.net>
> Cc: linux-media@vger.kernel.org

Tejun,

I have a question about cmwq, but to ask it, I need to give the context.

The non-system out_work_queue in cx18 had two purposes:

1. To prevent a userspace video rendering application from sleeping in
read(), when it had emptied a cx18 driver DMA buffer and the cx18 driver
had to sleep to notify the CX2318 engine that the buffer was available
again.

Your change has no adverse effect on this.


2. To prevent work items being handled by keventd/n from being delayed
too long, as the deferred work in question can involve a bit of sleeping
due to contention, the workload of the CX23418's MPEG encoding engine,
and the number of CX23418 devices in the system.

Will all the sleeping that can happen, is the move to a system wq, under
cmwq, going to have adverse affects on processing other work items in
the system?

I get the feeling it won't be a problem with cmwq, but I haven't paid
enough attention to be sure. 

Here are some gory details, if it helps:

A single CX23418 has *one* cx18 driver to CX23418 Capture Processing
Unit (epu2cpu_mb) mailbox though which the cx18 driver sends all normal
commands along with a notification interrupt to the CX23418.

The CX23418 will acknowledge all commands with ack back in the mailbox
and an interrupt for the cx18 driver.  Sometimes the ack happens right
away.  Sometimes the cx18 driver has to wait(), if the CX23418 is
busy.  

The cx18 driver uses a mutex (epu2cpu_mb_lock) to protect cx18 driver
access to the one epu2cpu_mb mailbox.  The mutex is grabbed and released
on a per epu2cpu_mb mailbox command basis

To tell the CX23418 about a usable empty buffer for DMA, one epu2cpu
mailbox command (CX18_CPU_DE_SET_MDL) must be performed for each usable
empty buffer.

A CX23418 can support multiple, simultaneous logical capture streams at
once.  The streams for a DVB TS, analog video converted to MPEG, VBI
data, and the MPEG Index data are common to have active simultaneously.

The CX23418 can keep track of 63 empty buffers for each active stream
and the cx18 driver tries to ensure the CX23418 always has as close to
63 as possible available at any time.

The out_work_handler, when triggered for a capture stream type, will try
to perform, as needed, from 1 to 63 CX18_CPU_DE_SET_MDL mailbox commands
for that stream.  At steady state, the number of CX18_CPU_DE_SET_MDL
mailbox commands executed one after the other in a batch will likely be
from 1 to 3.

It is not unusual for a non-commercial end user to have 2 or 3 CX23418
based capture cards in one machine.  I am aware of at least one
commercial user that had 5 CX23418 based cards in a modest machine.

It is not unusual for scheduled TV recording software to start nearly
simultaneous DTV TS, MPEG, and VBI or MPEG Index streams on multiple
cards.  So 3 CX23418 cards with 3 streams each.   Let's nominally
estimate the timing of the CX18_CPU_DE_SET_MDL commands per stream at
the PAL frame rate of 25 Hz; or 1 CX18_CPU_DE_SET_MDL mailbox command
per stream per 40 milliseconds.

Regards,
Andy

> ---
> Only compile tested.  Please feel free to take it into the subsystem
> tree or simply ack - I'll route it through the wq tree.
> 
> Thanks.
> 
>  drivers/media/video/cx18/cx18-driver.c  |   24 ++----------------------
>  drivers/media/video/cx18/cx18-driver.h  |    3 ---
>  drivers/media/video/cx18/cx18-streams.h |    3 +--
>  3 files changed, 3 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/media/video/cx18/cx18-driver.c b/drivers/media/video/cx18/cx18-driver.c
> index df60f27..41c0822 100644
> --- a/drivers/media/video/cx18/cx18-driver.c
> +++ b/drivers/media/video/cx18/cx18-driver.c
> @@ -656,7 +656,7 @@ static int __devinit cx18_create_in_workq(struct cx18 *cx)
>  {
>  	snprintf(cx->in_workq_name, sizeof(cx->in_workq_name), "%s-in",
>  		 cx->v4l2_dev.name);
> -	cx->in_work_queue = create_singlethread_workqueue(cx->in_workq_name);
> +	cx->in_work_queue = alloc_ordered_workqueue(cx->in_workq_name, 0);
>  	if (cx->in_work_queue == NULL) {
>  		CX18_ERR("Unable to create incoming mailbox handler thread\n");
>  		return -ENOMEM;
> @@ -664,18 +664,6 @@ static int __devinit cx18_create_in_workq(struct cx18 *cx)
>  	return 0;
>  }
>  
> -static int __devinit cx18_create_out_workq(struct cx18 *cx)
> -{
> -	snprintf(cx->out_workq_name, sizeof(cx->out_workq_name), "%s-out",
> -		 cx->v4l2_dev.name);
> -	cx->out_work_queue = create_workqueue(cx->out_workq_name);
> -	if (cx->out_work_queue == NULL) {
> -		CX18_ERR("Unable to create outgoing mailbox handler threads\n");
> -		return -ENOMEM;
> -	}
> -	return 0;
> -}
> -
>  static void __devinit cx18_init_in_work_orders(struct cx18 *cx)
>  {
>  	int i;
> @@ -702,15 +690,9 @@ static int __devinit cx18_init_struct1(struct cx18 *cx)
>  	mutex_init(&cx->epu2apu_mb_lock);
>  	mutex_init(&cx->epu2cpu_mb_lock);
>  
> -	ret = cx18_create_out_workq(cx);
> -	if (ret)
> -		return ret;
> -
>  	ret = cx18_create_in_workq(cx);
> -	if (ret) {
> -		destroy_workqueue(cx->out_work_queue);
> +	if (ret)
>  		return ret;
> -	}
>  
>  	cx18_init_in_work_orders(cx);
>  
> @@ -1094,7 +1076,6 @@ free_mem:
>  	release_mem_region(cx->base_addr, CX18_MEM_SIZE);
>  free_workqueues:
>  	destroy_workqueue(cx->in_work_queue);
> -	destroy_workqueue(cx->out_work_queue);
>  err:
>  	if (retval == 0)
>  		retval = -ENODEV;
> @@ -1244,7 +1225,6 @@ static void cx18_remove(struct pci_dev *pci_dev)
>  	cx18_halt_firmware(cx);
>  
>  	destroy_workqueue(cx->in_work_queue);
> -	destroy_workqueue(cx->out_work_queue);
>  
>  	cx18_streams_cleanup(cx, 1);
>  
> diff --git a/drivers/media/video/cx18/cx18-driver.h b/drivers/media/video/cx18/cx18-driver.h
> index 77be58c..f7f71d1 100644
> --- a/drivers/media/video/cx18/cx18-driver.h
> +++ b/drivers/media/video/cx18/cx18-driver.h
> @@ -614,9 +614,6 @@ struct cx18 {
>  	struct cx18_in_work_order in_work_order[CX18_MAX_IN_WORK_ORDERS];
>  	char epu_debug_str[256]; /* CX18_EPU_DEBUG is rare: use shared space */
>  
> -	struct workqueue_struct *out_work_queue;
> -	char out_workq_name[12]; /* "cx18-NN-out" */
> -
>  	/* i2c */
>  	struct i2c_adapter i2c_adap[2];
>  	struct i2c_algo_bit_data i2c_algo[2];
> diff --git a/drivers/media/video/cx18/cx18-streams.h b/drivers/media/video/cx18/cx18-streams.h
> index 77412be..5837ffb 100644
> --- a/drivers/media/video/cx18/cx18-streams.h
> +++ b/drivers/media/video/cx18/cx18-streams.h
> @@ -41,8 +41,7 @@ static inline bool cx18_stream_enabled(struct cx18_stream *s)
>  /* Related to submission of mdls to firmware */
>  static inline void cx18_stream_load_fw_queue(struct cx18_stream *s)
>  {
> -	struct cx18 *cx = s->cx;
> -	queue_work(cx->out_work_queue, &s->out_work_order);
> +	schedule_work(&s->out_work_order);
>  }
>  
>  static inline void cx18_stream_put_mdl_fw(struct cx18_stream *s,


--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

Hello,

On Mon, Jan 03, 2011 at 07:54:56PM -0500, Andy Walls wrote:
> 2. To prevent work items being handled by keventd/n from being delayed
> too long, as the deferred work in question can involve a bit of sleeping
> due to contention, the workload of the CX23418's MPEG encoding engine,
> and the number of CX23418 devices in the system.
> 
> Will all the sleeping that can happen, is the move to a system wq, under
> cmwq, going to have adverse affects on processing other work items in
> the system?
> 
> I get the feeling it won't be a problem with cmwq, but I haven't paid
> enough attention to be sure. 

It won't be a problem.  Now the system_wq supports parallel execution
of multiple works and manages concurrency automatically.  Work items
can sleep as necessary without worrying about other work items.

...
> It is not unusual for scheduled TV recording software to start nearly
> simultaneous DTV TS, MPEG, and VBI or MPEG Index streams on multiple
> cards.  So 3 CX23418 cards with 3 streams each.   Let's nominally
> estimate the timing of the CX18_CPU_DE_SET_MDL commands per stream at
> the PAL frame rate of 25 Hz; or 1 CX18_CPU_DE_SET_MDL mailbox command
> per stream per 40 milliseconds.

IIUC, if they spend any significant amount of time executing, they'll
be doing so by waiting for events (mutex, IRQ...), right?  If so,
there's nothing to worry about.  If it's gonna burn a lot of CPU
cycles, we'll need to use a workqueue marked CPU_INTENSIVE but I don't
think that's the case here.

Thank you.
  

On Tue, 2011-01-04 at 09:36 +0100, Tejun Heo wrote:
> Hello,
> 
> On Mon, Jan 03, 2011 at 07:54:56PM -0500, Andy Walls wrote:
> > 2. To prevent work items being handled by keventd/n from being delayed
> > too long, as the deferred work in question can involve a bit of sleeping
> > due to contention, the workload of the CX23418's MPEG encoding engine,
> > and the number of CX23418 devices in the system.
> > 
> > Will all the sleeping that can happen, is the move to a system wq, under
> > cmwq, going to have adverse affects on processing other work items in
> > the system?
> > 
> > I get the feeling it won't be a problem with cmwq, but I haven't paid
> > enough attention to be sure. 
> 
> It won't be a problem.  Now the system_wq supports parallel execution
> of multiple works and manages concurrency automatically.  Work items
> can sleep as necessary without worrying about other work items.

OK, that's what I thought.  I just wanted to ensure that busy CX23418
chips are not going to delay the processing of other work-events in the
rest the system down. 

> ...
> > It is not unusual for scheduled TV recording software to start nearly
> > simultaneous DTV TS, MPEG, and VBI or MPEG Index streams on multiple
> > cards.  So 3 CX23418 cards with 3 streams each.   Let's nominally
> > estimate the timing of the CX18_CPU_DE_SET_MDL commands per stream at
> > the PAL frame rate of 25 Hz; or 1 CX18_CPU_DE_SET_MDL mailbox command
> > per stream per 40 milliseconds.
> 
> IIUC, if they spend any significant amount of time executing, they'll
> be doing so by waiting for events (mutex, IRQ...), right?

Right.  As a CX23418 is expected to do more streaming by the host, the
mailbox mutex contention goes up and the latency of the CX23418
responding to mailbox commands also goes up so there is more wait()ing.


>   If so,
> there's nothing to worry about.  If it's gonna burn a lot of CPU
> cycles, we'll need to use a workqueue marked CPU_INTENSIVE but I don't
> think that's the case here.

Correct, CPU is not the concern here.


> Thank you.

Thanks.  I'll do a proper inspection of the patch tonight.  It looked OK
on cursory review.

Regards,
Andy

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

On Mon, 2011-01-03 at 14:49 +0100, Tejun Heo wrote:
> With cmwq, there's no reason to use separate out_work_queue.  Drop it
> and use system_wq instead.  The in_work_queue needs to be ordered so
> can't use one of the system wqs; however, as it isn't used to reclaim
> memory, allocate the workqueue with alloc_ordered_workqueue() without
> WQ_MEM_RECLAIM.
> 
> Signed-off-by: Tejun Heo <tj@kernel.org>
> Cc: Andy Walls <awalls@md.metrocast.net>
> Cc: linux-media@vger.kernel.org
> ---
> Only compile tested.  Please feel free to take it into the subsystem
> tree or simply ack - I'll route it through the wq tree.

Reviewed-by: Andy Walls <awalls@md.metrocast.net>
Acked-by: Andy Walls <awalls@md.metrocast.net>

Sorry it took so long for me to review.
Please route via your wq tree for the cx18 driver.

Thanks,
Andy

> Thanks.
> 
>  drivers/media/video/cx18/cx18-driver.c  |   24 ++----------------------
>  drivers/media/video/cx18/cx18-driver.h  |    3 ---
>  drivers/media/video/cx18/cx18-streams.h |    3 +--
>  3 files changed, 3 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/media/video/cx18/cx18-driver.c b/drivers/media/video/cx18/cx18-driver.c
> index df60f27..41c0822 100644
> --- a/drivers/media/video/cx18/cx18-driver.c
> +++ b/drivers/media/video/cx18/cx18-driver.c
> @@ -656,7 +656,7 @@ static int __devinit cx18_create_in_workq(struct cx18 *cx)
>  {
>  	snprintf(cx->in_workq_name, sizeof(cx->in_workq_name), "%s-in",
>  		 cx->v4l2_dev.name);
> -	cx->in_work_queue = create_singlethread_workqueue(cx->in_workq_name);
> +	cx->in_work_queue = alloc_ordered_workqueue(cx->in_workq_name, 0);
>  	if (cx->in_work_queue == NULL) {
>  		CX18_ERR("Unable to create incoming mailbox handler thread\n");
>  		return -ENOMEM;
> @@ -664,18 +664,6 @@ static int __devinit cx18_create_in_workq(struct cx18 *cx)
>  	return 0;
>  }
>  
> -static int __devinit cx18_create_out_workq(struct cx18 *cx)
> -{
> -	snprintf(cx->out_workq_name, sizeof(cx->out_workq_name), "%s-out",
> -		 cx->v4l2_dev.name);
> -	cx->out_work_queue = create_workqueue(cx->out_workq_name);
> -	if (cx->out_work_queue == NULL) {
> -		CX18_ERR("Unable to create outgoing mailbox handler threads\n");
> -		return -ENOMEM;
> -	}
> -	return 0;
> -}
> -
>  static void __devinit cx18_init_in_work_orders(struct cx18 *cx)
>  {
>  	int i;
> @@ -702,15 +690,9 @@ static int __devinit cx18_init_struct1(struct cx18 *cx)
>  	mutex_init(&cx->epu2apu_mb_lock);
>  	mutex_init(&cx->epu2cpu_mb_lock);
>  
> -	ret = cx18_create_out_workq(cx);
> -	if (ret)
> -		return ret;
> -
>  	ret = cx18_create_in_workq(cx);
> -	if (ret) {
> -		destroy_workqueue(cx->out_work_queue);
> +	if (ret)
>  		return ret;
> -	}
>  
>  	cx18_init_in_work_orders(cx);
>  
> @@ -1094,7 +1076,6 @@ free_mem:
>  	release_mem_region(cx->base_addr, CX18_MEM_SIZE);
>  free_workqueues:
>  	destroy_workqueue(cx->in_work_queue);
> -	destroy_workqueue(cx->out_work_queue);
>  err:
>  	if (retval == 0)
>  		retval = -ENODEV;
> @@ -1244,7 +1225,6 @@ static void cx18_remove(struct pci_dev *pci_dev)
>  	cx18_halt_firmware(cx);
>  
>  	destroy_workqueue(cx->in_work_queue);
> -	destroy_workqueue(cx->out_work_queue);
>  
>  	cx18_streams_cleanup(cx, 1);
>  
> diff --git a/drivers/media/video/cx18/cx18-driver.h b/drivers/media/video/cx18/cx18-driver.h
> index 77be58c..f7f71d1 100644
> --- a/drivers/media/video/cx18/cx18-driver.h
> +++ b/drivers/media/video/cx18/cx18-driver.h
> @@ -614,9 +614,6 @@ struct cx18 {
>  	struct cx18_in_work_order in_work_order[CX18_MAX_IN_WORK_ORDERS];
>  	char epu_debug_str[256]; /* CX18_EPU_DEBUG is rare: use shared space */
>  
> -	struct workqueue_struct *out_work_queue;
> -	char out_workq_name[12]; /* "cx18-NN-out" */
> -
>  	/* i2c */
>  	struct i2c_adapter i2c_adap[2];
>  	struct i2c_algo_bit_data i2c_algo[2];
> diff --git a/drivers/media/video/cx18/cx18-streams.h b/drivers/media/video/cx18/cx18-streams.h
> index 77412be..5837ffb 100644
> --- a/drivers/media/video/cx18/cx18-streams.h
> +++ b/drivers/media/video/cx18/cx18-streams.h
> @@ -41,8 +41,7 @@ static inline bool cx18_stream_enabled(struct cx18_stream *s)
>  /* Related to submission of mdls to firmware */
>  static inline void cx18_stream_load_fw_queue(struct cx18_stream *s)
>  {
> -	struct cx18 *cx = s->cx;
> -	queue_work(cx->out_work_queue, &s->out_work_order);
> +	schedule_work(&s->out_work_order);
>  }
>  
>  static inline void cx18_stream_put_mdl_fw(struct cx18_stream *s,


--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html