[PATCH] iwlwifi: dvm: convert create_singlethread_workqueue() to alloc_workqueue()

[PATCH] iwlwifi: dvm: convert create_singlethread_workqueue() to alloc_workqueue()

[Eva Rachel Retuya]

Use alloc_workqueue() to allocate the workqueue instead of
create_singlethread_workqueue() since the latter is deprecated and is scheduled
for removal.

There are work items doing related operations that shouldn't be swapped when
queued in a certain order hence preserve the strict execution ordering of a
single threaded (ST) workqueue by setting max_active to 1 and adding the
WQ_UNBOUND flag.

In addition, there are work items dealing with temperature throttling (tt_work,
ct_enter, ct_exit). Adding the WQ_HIGHPRI flag would place the work items in a
high priority workqueue.

Lastly, guarantee forward progress for work items depended upon during memory
reclaim by the addition of the WQ_MEM_RECLAIM flag.

Signed-off-by: Eva Rachel Retuya <eraretuya@gmail.com>
---
To the maintainers:
Just to confirm, are work items depended upon during memory reclaim? If not, the WQ_MEM_RECLAIM flag will be dropped. I added it just in case since create_singlethread_workqueue() also sets this flag.

 drivers/net/wireless/intel/iwlwifi/dvm/main.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/intel/iwlwifi/dvm/main.c b/drivers/net/wireless/intel/iwlwifi/dvm/main.c
index f62c2d7..37fa11c 100644
--- a/drivers/net/wireless/intel/iwlwifi/dvm/main.c
+++ b/drivers/net/wireless/intel/iwlwifi/dvm/main.c
@@ -1071,7 +1071,8 @@ static void iwl_bg_restart(struct work_struct *data)
 
 static void iwl_setup_deferred_work(struct iwl_priv *priv)
 {
-	priv->workqueue = create_singlethread_workqueue(DRV_NAME);
+	priv->workqueue = alloc_workqueue(DRV_NAME, WQ_HIGHPRI | WQ_UNBOUND |
+					  WQ_MEM_RECLAIM, 1);
 
 	INIT_WORK(&priv->restart, iwl_bg_restart);
 	INIT_WORK(&priv->beacon_update, iwl_bg_beacon_update);
-- 
1.9.1

Re: [PATCH] iwlwifi: dvm: convert create_singlethread_workqueue() to alloc_workqueue()

[Johannes Berg]

On Thu, 2016-03-17 at 20:37 +0800, Eva Rachel Retuya wrote:
> Use alloc_workqueue() to allocate the workqueue instead of
> create_singlethread_workqueue() since the latter is deprecated and is
> scheduled for removal.

Scheduled where?

>  static void iwl_setup_deferred_work(struct iwl_priv *priv)
>  {
> -	priv->workqueue = create_singlethread_workqueue(DRV_NAME);
> +	priv->workqueue = alloc_workqueue(DRV_NAME, WQ_HIGHPRI |
> WQ_UNBOUND |
> +					  WQ_MEM_RECLAIM, 1);

Seems like you should use alloc_ordered_workqueue() though? That also
gets you UNBOUND immediately, and the "1".

I'm not really sure HIGHPRI is needed either.

johannes

RE: [PATCH] iwlwifi: dvm: convert create_singlethread_workqueue() to alloc_workqueue()

[Grumbach, Emmanuel]

> 
> Use alloc_workqueue() to allocate the workqueue instead of
> create_singlethread_workqueue() since the latter is deprecated and is
> scheduled
> for removal.

I can't see any indication of that in the code of workqueue.
Can you share details about that?


> 
> There are work items doing related operations that shouldn't be swapped
> when
> queued in a certain order hence preserve the strict execution ordering of a
> single threaded (ST) workqueue by setting max_active to 1 and adding the
> WQ_UNBOUND flag.
> 
> In addition, there are work items dealing with temperature throttling
> (tt_work,
> ct_enter, ct_exit). Adding the WQ_HIGHPRI flag would place the work items
> in a
> high priority workqueue.
> 
> Lastly, guarantee forward progress for work items depended upon during
> memory
> reclaim by the addition of the WQ_MEM_RECLAIM flag.
> 
> Signed-off-by: Eva Rachel Retuya <eraretuya@gmail.com>
> ---
> To the maintainers:
> Just to confirm, are work items depended upon during memory reclaim? If
> not, the WQ_MEM_RECLAIM flag will be dropped. I added it just in case since
> create_singlethread_workqueue() also sets this flag.
> 
>  drivers/net/wireless/intel/iwlwifi/dvm/main.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/wireless/intel/iwlwifi/dvm/main.c
> b/drivers/net/wireless/intel/iwlwifi/dvm/main.c
> index f62c2d7..37fa11c 100644
> --- a/drivers/net/wireless/intel/iwlwifi/dvm/main.c
> +++ b/drivers/net/wireless/intel/iwlwifi/dvm/main.c
> @@ -1071,7 +1071,8 @@ static void iwl_bg_restart(struct work_struct *data)
> 
>  static void iwl_setup_deferred_work(struct iwl_priv *priv)
>  {
> -	priv->workqueue = create_singlethread_workqueue(DRV_NAME);
> +	priv->workqueue = alloc_workqueue(DRV_NAME, WQ_HIGHPRI |
> WQ_UNBOUND |
> +					  WQ_MEM_RECLAIM, 1);
> 
>  	INIT_WORK(&priv->restart, iwl_bg_restart);
>  	INIT_WORK(&priv->beacon_update, iwl_bg_beacon_update);
> --
> 1.9.1

Re: [PATCH] iwlwifi: dvm: convert create_singlethread_workqueue() to alloc_workqueue()

[tj-DgEjT+Ai2ygdnm+yROfE0A]

Hello,

On Thu, Mar 17, 2016 at 12:43:35PM +0000, Grumbach, Emmanuel wrote:
> > 
> > Use alloc_workqueue() to allocate the workqueue instead of
> > create_singlethread_workqueue() since the latter is deprecated and is
> > scheduled
> > for removal.
> 
> I can't see any indication of that in the code of workqueue.
> Can you share details about that?

create*_workqueue()'s have been deprecated and replaced by
alloc*_workqueue()'s for some years now.  I probably should note that
in the header file.

Thanks.

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

Re: [PATCH] iwlwifi: dvm: convert create_singlethread_workqueue() to alloc_workqueue()

[Tejun Heo]

Hello,

On Thu, Mar 17, 2016 at 01:43:22PM +0100, Johannes Berg wrote:
> On Thu, 2016-03-17 at 20:37 +0800, Eva Rachel Retuya wrote:
> > Use alloc_workqueue() to allocate the workqueue instead of
> > create_singlethread_workqueue() since the latter is deprecated and is
> > scheduled for removal.
> 
> Scheduled where?

They've been deprecated for years now.  I should note that in the
header.

> >  static void iwl_setup_deferred_work(struct iwl_priv *priv)
> >  {
> > -	priv->workqueue = create_singlethread_workqueue(DRV_NAME);
> > +	priv->workqueue = alloc_workqueue(DRV_NAME, WQ_HIGHPRI |
> > WQ_UNBOUND |
> > +					  WQ_MEM_RECLAIM, 1);
> 
> Seems like you should use alloc_ordered_workqueue() though? That also
> gets you UNBOUND immediately, and the "1".

Right, this one should have been alloc_ordered_workqueue().

> I'm not really sure HIGHPRI is needed either.

So, no WQ_MEM_RECLAIM either then, I suppose?  What are the latency
requirements here - what happens if a thermal management work gets
delayed?

Thanks.

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

RE: [PATCH] iwlwifi: dvm: convert create_singlethread_workqueue() to alloc_workqueue()

[Grumbach, Emmanuel]

> Hello,
> 
> On Thu, Mar 17, 2016 at 01:43:22PM +0100, Johannes Berg wrote:
> > On Thu, 2016-03-17 at 20:37 +0800, Eva Rachel Retuya wrote:
> > > Use alloc_workqueue() to allocate the workqueue instead of
> > > create_singlethread_workqueue() since the latter is deprecated and
> > > is scheduled for removal.
> >
> > Scheduled where?
> 
> They've been deprecated for years now.  I should note that in the header.
> 
> > >  static void iwl_setup_deferred_work(struct iwl_priv *priv)
> > >  {
> > > -	priv->workqueue = create_singlethread_workqueue(DRV_NAME);
> > > +	priv->workqueue = alloc_workqueue(DRV_NAME, WQ_HIGHPRI |
> > > WQ_UNBOUND |
> > > +					  WQ_MEM_RECLAIM, 1);
> >
> > Seems like you should use alloc_ordered_workqueue() though? That also
> > gets you UNBOUND immediately, and the "1".
> 
> Right, this one should have been alloc_ordered_workqueue().
> 
> > I'm not really sure HIGHPRI is needed either.
> 
> So, no WQ_MEM_RECLAIM either then, I suppose?  What are the latency
> requirements here - what happens if a thermal management work gets
> delayed?
> 

This worker is not supposed to free memory, so no WQ_MEM_RECLAIM needed. The latency is not critical.