[PATCH] ath6kl: avoid flush_scheduled_work() usage

[PATCH] ath6kl: avoid flush_scheduled_work() usage

[Tetsuo Handa]

Use local wq in order to avoid flush_scheduled_work() usage.

Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
Please see commit c4f135d643823a86 ("workqueue: Wrap flush_workqueue()
using a macro") for background.

This is a blind conversion, and is only compile tested.

 drivers/net/wireless/ath/ath6kl/usb.c | 29 +++++++++++++++++++++++----
 1 file changed, 25 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/ath/ath6kl/usb.c b/drivers/net/wireless/ath/ath6kl/usb.c
index 65e683effdcb..e3c65a671be1 100644
--- a/drivers/net/wireless/ath/ath6kl/usb.c
+++ b/drivers/net/wireless/ath/ath6kl/usb.c
@@ -21,6 +21,8 @@
 #include "debug.h"
 #include "core.h"
 
+static struct workqueue_struct *ath6kl_wq;
+
 /* constants */
 #define TX_URB_COUNT            32
 #define RX_URB_COUNT            32
@@ -478,7 +480,7 @@ static void ath6kl_usb_flush_all(struct ath6kl_usb *ar_usb)
 	 * Flushing any pending I/O may schedule work this call will block
 	 * until all scheduled work runs to completion.
 	 */
-	flush_scheduled_work();
+	flush_workqueue(ath6kl_wq);
 }
 
 static void ath6kl_usb_start_recv_pipes(struct ath6kl_usb *ar_usb)
@@ -544,7 +546,7 @@ static void ath6kl_usb_recv_complete(struct urb *urb)
 
 	/* note: queue implements a lock */
 	skb_queue_tail(&pipe->io_comp_queue, skb);
-	schedule_work(&pipe->io_complete_work);
+	queue_work(ath6kl_wq, &pipe->io_complete_work);
 
 cleanup_recv_urb:
 	ath6kl_usb_cleanup_recv_urb(urb_context);
@@ -579,7 +581,7 @@ static void ath6kl_usb_usb_transmit_complete(struct urb *urb)
 
 	/* note: queue implements a lock */
 	skb_queue_tail(&pipe->io_comp_queue, skb);
-	schedule_work(&pipe->io_complete_work);
+	queue_work(ath6kl_wq, &pipe->io_complete_work);
 }
 
 static void ath6kl_usb_io_comp_work(struct work_struct *work)
@@ -1234,7 +1236,26 @@ static struct usb_driver ath6kl_usb_driver = {
 	.disable_hub_initiated_lpm = 1,
 };
 
-module_usb_driver(ath6kl_usb_driver);
+static int __init ath6kl_init(void)
+{
+	int ret;
+
+	ath6kl_wq = alloc_workqueue("ath6kl_wq", 0, 0);
+	if (!ath6kl_wq)
+		return -ENOMEM;
+	ret = usb_register(&ath6kl_usb_driver);
+	if (ret)
+		destroy_workqueue(ath6kl_wq);
+	return ret;
+}
+module_init(ath6kl_init);
+
+static void __exit ath6kl_exit(void)
+{
+	usb_deregister(&ath6kl_usb_driver);
+	destroy_workqueue(ath6kl_wq);
+}
+module_exit(ath6kl_exit);
 
 MODULE_AUTHOR("Atheros Communications, Inc.");
 MODULE_DESCRIPTION("Driver support for Atheros AR600x USB devices");
-- 
2.18.4

Re: [PATCH] ath6kl: avoid flush_scheduled_work() usage

[Jeff Johnson]

On 6/10/2022 4:12 AM, Tetsuo Handa wrote:
> Use local wq in order to avoid flush_scheduled_work() usage.
> 
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> ---
> Please see commit c4f135d643823a86 ("workqueue: Wrap flush_workqueue()
> using a macro") for background.
> 
> This is a blind conversion, and is only compile tested.
> 
>   drivers/net/wireless/ath/ath6kl/usb.c | 29 +++++++++++++++++++++++----
>   1 file changed, 25 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/wireless/ath/ath6kl/usb.c b/drivers/net/wireless/ath/ath6kl/usb.c
[snip]
> -module_usb_driver(ath6kl_usb_driver);
> +static int __init ath6kl_init(void)
> +{
> +	int ret;
> +
> +	ath6kl_wq = alloc_workqueue("ath6kl_wq", 0, 0);
> +	if (!ath6kl_wq)
> +		return -ENOMEM;

this approach means the driver will always allocate a workqueue even if 
the associated hardware is never present.

did you consider instead having the allocation take place within the 
processing of ath6kl_usb_probe() and the destroy take place within the 
processing of ath6kl_usb_pm_remove()?

Re: [PATCH] ath6kl: avoid flush_scheduled_work() usage

[Jeff Johnson]

On 6/10/2022 12:05 PM, Jeff Johnson wrote:
> On 6/10/2022 4:12 AM, Tetsuo Handa wrote:
>> Use local wq in order to avoid flush_scheduled_work() usage.
>>
>> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
>> ---
>> Please see commit c4f135d643823a86 ("workqueue: Wrap flush_workqueue()
>> using a macro") for background.
>>
>> This is a blind conversion, and is only compile tested.
>>
>>   drivers/net/wireless/ath/ath6kl/usb.c | 29 +++++++++++++++++++++++----
>>   1 file changed, 25 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/net/wireless/ath/ath6kl/usb.c 
>> b/drivers/net/wireless/ath/ath6kl/usb.c
> [snip]
>> -module_usb_driver(ath6kl_usb_driver);
>> +static int __init ath6kl_init(void)
>> +{
>> +    int ret;
>> +
>> +    ath6kl_wq = alloc_workqueue("ath6kl_wq", 0, 0);
>> +    if (!ath6kl_wq)
>> +        return -ENOMEM;
> 
> this approach means the driver will always allocate a workqueue even if 
> the associated hardware is never present.
> 
> did you consider instead having the allocation take place within the 
> processing of ath6kl_usb_probe() and the destroy take place within the 
> processing of ath6kl_usb_pm_remove()?

typo: ath6kl_usb_pm_remove() => ath6kl_usb_remove()

Re: [PATCH] ath6kl: avoid flush_scheduled_work() usage

[Tetsuo Handa]

On 2022/06/11 4:10, Jeff Johnson wrote:
> On 6/10/2022 12:05 PM, Jeff Johnson wrote:
>>> +static int __init ath6kl_init(void)
>>> +{
>>> +    int ret;
>>> +
>>> +    ath6kl_wq = alloc_workqueue("ath6kl_wq", 0, 0);
>>> +    if (!ath6kl_wq)
>>> +        return -ENOMEM;
>>
>> this approach means the driver will always allocate a workqueue even if the associated hardware is never present.

Creating a WQ without WQ_MEM_RECLAIM flag consumes little resource.

>>
>> did you consider instead having the allocation take place within the processing of ath6kl_usb_probe() and the destroy take place within the processing of ath6kl_usb_pm_remove()?
> 
> typo: ath6kl_usb_pm_remove() => ath6kl_usb_remove()

Technically possible to use ath6kl_usb_create()/ath6kl_usb_destroy() if you prefer it.

Do you want ath6kl_wq be shared within this module (using a refcount), or be local to
each "struct ath6kl_usb" (adding a member and accessing via usb_get_intfdata()) ?

Re: [PATCH] ath6kl: avoid flush_scheduled_work() usage

[Tetsuo Handa]

On 2022/06/11 7:51, Tetsuo Handa wrote:
> On 2022/06/11 4:10, Jeff Johnson wrote:
>> On 6/10/2022 12:05 PM, Jeff Johnson wrote:
>>>> +static int __init ath6kl_init(void)
>>>> +{
>>>> +    int ret;
>>>> +
>>>> +    ath6kl_wq = alloc_workqueue("ath6kl_wq", 0, 0);
>>>> +    if (!ath6kl_wq)
>>>> +        return -ENOMEM;
>>>
>>> this approach means the driver will always allocate a workqueue even if the associated hardware is never present.
> 
> Creating a WQ without WQ_MEM_RECLAIM flag consumes little resource.

My understanding is that a loadable kernel module file is modprobe'd when
the kernel detected a hardware and some userspace program determined a .ko
file to use based on device IDs database). Thus, I think it is very likely
that an associated hardware presents if module's __init function is called.

If a module is built-in, that user is ready to tolerate memory footprint
wasted by non-present hardware. And I think memory wasted by keeping an
unused !WQ_MEM_RECLAIM workqueue is much smaller than memory wasted by
keeping that module.

Thus, I wonder who complains about creating possibly unused !WQ_MEM_RECLAIM
workqueue, unless that module is designed for tiny embedded environments.

> 
>>>
>>> did you consider instead having the allocation take place within the processing of ath6kl_usb_probe() and the destroy take place within the processing of ath6kl_usb_pm_remove()?
>>
>> typo: ath6kl_usb_pm_remove() => ath6kl_usb_remove()
> 
> Technically possible to use ath6kl_usb_create()/ath6kl_usb_destroy() if you prefer it.
> 
> Do you want ath6kl_wq be shared within this module (using a refcount), or be local to
> each "struct ath6kl_usb" (adding a member and accessing via usb_get_intfdata()) ?
> 

Anyway, here goes per "struct ath6kl_usb" version.


From 00c560307d72abffea29409328be8cd69abecc95 Mon Sep 17 00:00:00 2001
From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Date: Sun, 12 Jun 2022 10:38:03 +0900
Subject: [PATCH v2] ath6kl: avoid flush_scheduled_work() usage

Use local wq in order to avoid flush_scheduled_work() usage.

Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
Changes in v2:
  Use per "struct ath6kl_usb" workqueue instead of per module workqueue.

Please see commit c4f135d643823a86 ("workqueue: Wrap flush_workqueue()
using a macro") for background.

This is a blind conversion, and is only compile tested.

 drivers/net/wireless/ath/ath6kl/usb.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/ath/ath6kl/usb.c b/drivers/net/wireless/ath/ath6kl/usb.c
index 65e683effdcb..5220809841a6 100644
--- a/drivers/net/wireless/ath/ath6kl/usb.c
+++ b/drivers/net/wireless/ath/ath6kl/usb.c
@@ -71,6 +71,7 @@ struct ath6kl_usb {
 	u8 *diag_cmd_buffer;
 	u8 *diag_resp_buffer;
 	struct ath6kl *ar;
+	struct workqueue_struct *wq;
 };
 
 /* usb urb object */
@@ -478,7 +479,7 @@ static void ath6kl_usb_flush_all(struct ath6kl_usb *ar_usb)
 	 * Flushing any pending I/O may schedule work this call will block
 	 * until all scheduled work runs to completion.
 	 */
-	flush_scheduled_work();
+	flush_workqueue(ar_usb->wq);
 }
 
 static void ath6kl_usb_start_recv_pipes(struct ath6kl_usb *ar_usb)
@@ -544,7 +545,7 @@ static void ath6kl_usb_recv_complete(struct urb *urb)
 
 	/* note: queue implements a lock */
 	skb_queue_tail(&pipe->io_comp_queue, skb);
-	schedule_work(&pipe->io_complete_work);
+	queue_work(pipe->ar_usb->wq, &pipe->io_complete_work);
 
 cleanup_recv_urb:
 	ath6kl_usb_cleanup_recv_urb(urb_context);
@@ -579,7 +580,7 @@ static void ath6kl_usb_usb_transmit_complete(struct urb *urb)
 
 	/* note: queue implements a lock */
 	skb_queue_tail(&pipe->io_comp_queue, skb);
-	schedule_work(&pipe->io_complete_work);
+	queue_work(pipe->ar_usb->wq, &pipe->io_complete_work);
 }
 
 static void ath6kl_usb_io_comp_work(struct work_struct *work)
@@ -619,6 +620,7 @@ static void ath6kl_usb_destroy(struct ath6kl_usb *ar_usb)
 
 	kfree(ar_usb->diag_cmd_buffer);
 	kfree(ar_usb->diag_resp_buffer);
+	destroy_workqueue(ar_usb->wq);
 
 	kfree(ar_usb);
 }
@@ -631,9 +633,15 @@ static struct ath6kl_usb *ath6kl_usb_create(struct usb_interface *interface)
 	int status = 0;
 	int i;
 
+	/* ath6kl_usb_destroy() needs ar_usb != NULL && ar_usb->wq != NULL. */
 	ar_usb = kzalloc(sizeof(struct ath6kl_usb), GFP_KERNEL);
 	if (ar_usb == NULL)
-		goto fail_ath6kl_usb_create;
+		return NULL;
+	ar_usb->wq = alloc_workqueue("ath6kl_wq", 0, 0);
+	if (!ar_usb->wq) {
+		kfree(ar_usb);
+		return NULL;
+	}
 
 	usb_set_intfdata(interface, ar_usb);
 	spin_lock_init(&(ar_usb->cs_lock));
-- 
2.18.4

Re: [PATCH] ath6kl: avoid flush_scheduled_work() usage

[Kalle Valo]

Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> writes:

> On 2022/06/11 7:51, Tetsuo Handa wrote:
>> On 2022/06/11 4:10, Jeff Johnson wrote:
>>> On 6/10/2022 12:05 PM, Jeff Johnson wrote:
>>>>> +static int __init ath6kl_init(void)
>>>>> +{
>>>>> +    int ret;
>>>>> +
>>>>> +    ath6kl_wq = alloc_workqueue("ath6kl_wq", 0, 0);
>>>>> +    if (!ath6kl_wq)
>>>>> +        return -ENOMEM;
>>>>
>>>> this approach means the driver will always allocate a workqueue even if the associated hardware is never present.
>> 
>> Creating a WQ without WQ_MEM_RECLAIM flag consumes little resource.
>
> My understanding is that a loadable kernel module file is modprobe'd when
> the kernel detected a hardware and some userspace program determined a .ko
> file to use based on device IDs database). Thus, I think it is very likely
> that an associated hardware presents if module's __init function is called.
>
> If a module is built-in, that user is ready to tolerate memory footprint
> wasted by non-present hardware. And I think memory wasted by keeping an
> unused !WQ_MEM_RECLAIM workqueue is much smaller than memory wasted by
> keeping that module.
>
> Thus, I wonder who complains about creating possibly unused !WQ_MEM_RECLAIM
> workqueue, unless that module is designed for tiny embedded
> environments.

I complain, it's still wrong. We have patches which save few bytes
everywhere we can, we shouldn't deliberately increase the kernel size.

>>From 00c560307d72abffea29409328be8cd69abecc95 Mon Sep 17 00:00:00 2001
> From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Date: Sun, 12 Jun 2022 10:38:03 +0900
> Subject: [PATCH v2] ath6kl: avoid flush_scheduled_work() usage
>
> Use local wq in order to avoid flush_scheduled_work() usage.
>
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> ---
> Changes in v2:
>   Use per "struct ath6kl_usb" workqueue instead of per module workqueue.

Please don't embed patches into email, patchwork doesn't see those:

https://patchwork.kernel.org/project/linux-wireless/list/

Please submit v3.

> Please see commit c4f135d643823a86 ("workqueue: Wrap flush_workqueue()
> using a macro") for background.
>
> This is a blind conversion, and is only compile tested.

This is good information to have, please include that to the commit log
so that it's stored to git.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

[PATCH v3] ath6kl: avoid flush_scheduled_work() usage

[Tetsuo Handa]

As per commit c4f135d643823a86 ("workqueue: Wrap flush_workqueue() using
a macro") says, use per "struct ath6kl_usb" workqueue.

Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
Changes in v3:
  Update patch description.

Changes in v2:
  Use per "struct ath6kl_usb" workqueue instead of per module workqueue.

This is a blind conversion, and is only compile tested.

 drivers/net/wireless/ath/ath6kl/usb.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/ath/ath6kl/usb.c b/drivers/net/wireless/ath/ath6kl/usb.c
index 65e683effdcb..5220809841a6 100644
--- a/drivers/net/wireless/ath/ath6kl/usb.c
+++ b/drivers/net/wireless/ath/ath6kl/usb.c
@@ -71,6 +71,7 @@ struct ath6kl_usb {
 	u8 *diag_cmd_buffer;
 	u8 *diag_resp_buffer;
 	struct ath6kl *ar;
+	struct workqueue_struct *wq;
 };
 
 /* usb urb object */
@@ -478,7 +479,7 @@ static void ath6kl_usb_flush_all(struct ath6kl_usb *ar_usb)
 	 * Flushing any pending I/O may schedule work this call will block
 	 * until all scheduled work runs to completion.
 	 */
-	flush_scheduled_work();
+	flush_workqueue(ar_usb->wq);
 }
 
 static void ath6kl_usb_start_recv_pipes(struct ath6kl_usb *ar_usb)
@@ -544,7 +545,7 @@ static void ath6kl_usb_recv_complete(struct urb *urb)
 
 	/* note: queue implements a lock */
 	skb_queue_tail(&pipe->io_comp_queue, skb);
-	schedule_work(&pipe->io_complete_work);
+	queue_work(pipe->ar_usb->wq, &pipe->io_complete_work);
 
 cleanup_recv_urb:
 	ath6kl_usb_cleanup_recv_urb(urb_context);
@@ -579,7 +580,7 @@ static void ath6kl_usb_usb_transmit_complete(struct urb *urb)
 
 	/* note: queue implements a lock */
 	skb_queue_tail(&pipe->io_comp_queue, skb);
-	schedule_work(&pipe->io_complete_work);
+	queue_work(pipe->ar_usb->wq, &pipe->io_complete_work);
 }
 
 static void ath6kl_usb_io_comp_work(struct work_struct *work)
@@ -619,6 +620,7 @@ static void ath6kl_usb_destroy(struct ath6kl_usb *ar_usb)
 
 	kfree(ar_usb->diag_cmd_buffer);
 	kfree(ar_usb->diag_resp_buffer);
+	destroy_workqueue(ar_usb->wq);
 
 	kfree(ar_usb);
 }
@@ -631,9 +633,15 @@ static struct ath6kl_usb *ath6kl_usb_create(struct usb_interface *interface)
 	int status = 0;
 	int i;
 
+	/* ath6kl_usb_destroy() needs ar_usb != NULL && ar_usb->wq != NULL. */
 	ar_usb = kzalloc(sizeof(struct ath6kl_usb), GFP_KERNEL);
 	if (ar_usb == NULL)
-		goto fail_ath6kl_usb_create;
+		return NULL;
+	ar_usb->wq = alloc_workqueue("ath6kl_wq", 0, 0);
+	if (!ar_usb->wq) {
+		kfree(ar_usb);
+		return NULL;
+	}
 
 	usb_set_intfdata(interface, ar_usb);
 	spin_lock_init(&(ar_usb->cs_lock));
-- 
2.18.4

Re: [PATCH v3] ath6kl: avoid flush_scheduled_work() usage

[Kalle Valo]

Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> wrote:

> As per commit c4f135d643823a86 ("workqueue: Wrap flush_workqueue() using
> a macro") says, use per "struct ath6kl_usb" workqueue.
> 
> This is a blind conversion, and is only compile tested.
> 
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com>

Patch applied to ath-next branch of ath.git, thanks.

62ebaf2f9261 ath6kl: avoid flush_scheduled_work() usage

-- 
https://patchwork.kernel.org/project/linux-wireless/patch/f78ddbdc-8989-a1a7-2234-ce9ec3894625@I-love.SAKURA.ne.jp/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches